Consolidate runnable variants on DOMManipulationTask into a single Runnable variant. #12292

cbrewster · 2016-07-06T16:50:43Z

Consolidates the runnable variants on DOMManipulationTask into a single Runnable variant.
Also combines MainThreadRunnable into the Runnable trait.

I plan on filing a few E-Easy issues after this lands to implement name on each of the structs that implement Runnable.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because refactoring

This change is


        Combined DOMManipulationTask runnable variants into a single variant

highfive · 2016-07-06T16:50:45Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/storage.rs, components/script/dom/htmldetailselement.rs, components/script/dom/htmlformelement.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/htmlimageelement.rs, components/script/script_thread.rs, components/script/task_source/dom_manipulation.rs

highfive · 2016-07-06T16:50:47Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-07-06T17:14:08Z

components/script/script_thread.rs

-
-pub trait MainThreadRunnable {
-    fn handler(self: Box<Self>, script_thread: &ScriptThread);
+    fn name(&self) -> &'static str { "" }


I was thinking of "generic runnable" instead of the empty string.


        Add name method to Runnable trait


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12296)


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12296)


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12296)


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12296)


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12296)


        Auto merge of #12296 - ConnorGBrewster:dommanip_intermittent, r=<try>

[Do Not Merge] Make DOM Manipulation events cancellable  I can't reproduce the intermittent failure on #11703 so I am running some tests using buildbot. If this does fix the intermittent, this is blocked on #12292. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11703 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12296)

KiChjang · 2016-07-07T07:36:28Z

@bors-servo r+

bors-servo · 2016-07-07T07:36:29Z

📌 Commit 54cf3af has been approved by KiChjang

bors-servo · 2016-07-07T08:16:42Z

⌛ Testing commit 54cf3af with merge 783fe8f...


        Auto merge of #12292 - ConnorGBrewster:runnable_variant, r=KiChjang

Consolidate runnable variants on DOMManipulationTask into a single Runnable variant.  Consolidates the runnable variants on `DOMManipulationTask` into a single `Runnable` variant. Also combines `MainThreadRunnable` into the `Runnable` trait. I plan on filing a few E-Easy issues after this lands to implement `name` on each of the structs that implement `Runnable`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12292)

bors-servo · 2016-07-07T09:11:48Z

💔 Test failed - linux-rel

highfive · 2016-07-07T09:12:31Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /</span><span class="stdout">css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

KiChjang · 2016-07-07T09:31:49Z

@bors-servo retry

#11574

bors-servo · 2016-07-07T10:43:32Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, windows are reusable. Rebuilding only linux-rel, mac-rel-css, mac-rel-wpt...

bors-servo · 2016-07-07T11:26:13Z

💔 Test failed - mac-rel-css

KiChjang · 2016-07-07T11:31:32Z

STDERR: Could not terminate process: 21086
  ▶ ERROR [expected OK] /css-transforms-1_dev/html/css-transform-property-existence.htm
  └   → Traceback (most recent call last):
  File "/Users/servo/buildbot/slave/mac-rel-css/build/tests/wpt/harness/wptrunner/executors/base.py", line 149, in run_test
    result = self.do_test(test)
  File "/Users/servo/buildbot/slave/mac-rel-css/build/tests/wpt/harness/wptrunner/executors/executorservo.py", line 133, in do_test
    self.proc.kill()
  File "/Users/servo/buildbot/slave/mac-rel-css/build/python/_virtualenv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 749, in kill
    self.proc.kill(sig=sig)
  File "/Users/servo/buildbot/slave/mac-rel-css/build/python/_virtualenv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 165, in kill
    send_sig(signal.SIGTERM)
  File "/Users/servo/buildbot/slave/mac-rel-css/build/python/_virtualenv/lib/python2.7/site-packages/mozprocess/processhandler.py", line 152, in send_sig
    os.killpg(pid, sig)
OSError: [Errno 1] Operation not permitted

larsbergstrom · 2016-07-07T19:18:34Z

@bors-servo retry

#12314

bors-servo · 2016-07-07T21:26:45Z

⌛ Testing commit 54cf3af with merge fdfde1c...


        Auto merge of #12292 - ConnorGBrewster:runnable_variant, r=KiChjang

Consolidate runnable variants on DOMManipulationTask into a single Runnable variant.  Consolidates the runnable variants on `DOMManipulationTask` into a single `Runnable` variant. Also combines `MainThreadRunnable` into the `Runnable` trait. I plan on filing a few E-Easy issues after this lands to implement `name` on each of the structs that implement `Runnable`. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12292)

bors-servo · 2016-07-07T22:22:11Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

Combined DOMManipulationTask runnable variants into a single variant

d6f4dc0

highfive assigned Manishearth Jul 6, 2016

highfive added the S-awaiting-review label Jul 6, 2016

cbrewster changed the title ~~Runnable variant~~ Consolidate runnable variants on DOMManipulationTask into a single Runnable variant. Jul 6, 2016

KiChjang reviewed Jul 6, 2016
View changes

Add name method to Runnable trait

Loading status checks…

54cf3af

cbrewster force-pushed the cbrewster:runnable_variant branch from b86b123 to 54cf3af Jul 6, 2016

cbrewster mentioned this pull request Jul 6, 2016

[Do Not Merge] Make DOM Manipulation events cancellable #12296

Closed

3 of 5 tasks complete

highfive assigned KiChjang and unassigned Manishearth Jul 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 7, 2016

highfive added the S-tests-failed label Jul 7, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 7, 2016

bors-servo merged commit 54cf3af into servo:master Jul 7, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 7, 2016

servo / servo

Consolidate runnable variants on DOMManipulationTask into a single Runnable variant. #12292

Consolidate runnable variants on DOMManipulationTask into a single Runnable variant. #12292

cbrewster commented Jul 6, 2016 •

edited by larsbergstrom

highfive commented Jul 6, 2016

highfive commented Jul 6, 2016

This comment has been minimized.

KiChjang commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

highfive commented Jul 7, 2016

KiChjang commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

KiChjang commented Jul 7, 2016

larsbergstrom commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

servo / servo

Join GitHub today

Consolidate runnable variants on DOMManipulationTask into a single Runnable variant. #12292

Consolidate runnable variants on DOMManipulationTask into a single Runnable variant. #12292

Conversation

cbrewster commented Jul 6, 2016 • edited by larsbergstrom

highfive commented Jul 6, 2016

highfive commented Jul 6, 2016

This comment has been minimized.

KiChjang Jul 6, 2016

KiChjang commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

highfive commented Jul 7, 2016

KiChjang commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

KiChjang commented Jul 7, 2016

larsbergstrom commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

cbrewster commented Jul 6, 2016 •

edited by larsbergstrom