Clean up task sources and make all tasks cancellable #12404

cbrewster · 2016-07-12T05:59:46Z

This makes it so each task is a thin wrapper over a runnable and whenever a task is queued, it is automatically wrapped by the window's runnable_wrapper.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
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


        Add event runnables

Make tasks a wrapper over runnables

highfive · 2016-07-12T05:59:49Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmltextareaelement.rs, components/script/dom/document.rs, components/script/task_source/user_interaction.rs, components/script/dom/storage.rs, components/script/network_listener.rs, components/script/dom/event.rs, components/script/dom/htmldetailselement.rs, components/script/dom/htmlformelement.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/htmlimageelement.rs, components/script/script_thread.rs, components/script/dom/htmlinputelement.rs, components/script/task_source/dom_manipulation.rs, components/script/task_source/mod.rs

highfive · 2016-07-12T05:59:50Z

Warning

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

nox · 2016-07-12T08:43:15Z

@bors-servo r+

bors-servo · 2016-07-12T08:43:16Z

📌 Commit 6b0ce8e has been approved by nox

bors-servo · 2016-07-12T10:42:17Z

⌛ Testing commit 6b0ce8e with merge 0049ced...


        Auto merge of #12404 - ConnorGBrewster:task_source_cleanup, r=nox

Clean up task sources and make all tasks cancellable  This makes it so each task is a thin wrapper over a runnable and whenever a task is queued, it is automatically wrapped by the window's `runnable_wrapper`. ---  - [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 - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12404)

bors-servo · 2016-07-12T11:35:52Z

💔 Test failed - linux-rel

jdm · 2016-07-12T15:37:16Z

All the webstorage tests now time out, suggesting that something went wrong here :)

cbrewster · 2016-07-12T16:42:29Z

Woops, CancellableRunnable needed to implement the new Runnable methods to call the proper method on their inner runnable.

asajeffrey · 2016-07-12T23:14:16Z

May fix #11703?

asajeffrey · 2016-07-13T15:47:02Z

Mostly this looks good, and yay for one less intermittent! My only question is about the amount of boxing we're doing, can we replace Box<T> by T?

Reviewed 1 of 9 files at r1, 1 of 3 files at r2, 12 of 13 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/script_thread.rs, line 177 [r5] (raw file):

impl RunnableWrapper {
    pub fn wrap_runnable<T: Runnable + Send + 'static>(&self, runnable: Box<T>) -> Box<Runnable + Send> {

Why are we taking a Box<T> rather than a T as argument?

components/script/task_source/mod.rs, line 16 [r5] (raw file):

pub trait TaskSource {
    fn queue<T: Runnable + Send + 'static>(&self, msg: Box<T>, window: &Window) -> Result<(), ()>;

Why are we taking a Box<T> as an argument rather than a T?

Comments from Reviewable

cbrewster · 2016-07-13T15:56:37Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/script_thread.rs, line 177 [r5] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Why are we taking a Box<T> rather than a T as argument?

This doesn't end up boxing any more than usual, it just requires boxing the runnable before it reaches this point (as before this method boxed the runnable). I do think it is cleaner to have this method do the boxing rather than requiring the boxing to be done when the runnable is created.

components/script/task_source/mod.rs, line 16 [r5] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Why are we taking a Box<T> as an argument rather than a T?

Same as above.

Comments from Reviewable


        Make all task source runnables cancellable

Implement all Runnable methods on CancellableRunnable to redirect to their inner runnable

asajeffrey · 2016-07-13T16:10:42Z

Review status: 5 of 16 files reviewed at latest revision, 2 unresolved discussions.

components/script/script_thread.rs, line 177 [r5] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

This doesn't end up boxing any more than usual, it just requires boxing the runnable before it reaches this point (as before this method boxed the runnable). I do think it is cleaner to have this method do the boxing rather than requiring the boxing to be done when the runnable is created.

I meant why are we doing any boxing?

Comments from Reviewable

KiChjang · 2016-07-13T16:21:27Z

@asajeffrey Because Runnable is a trait, and you cannot have a trait as an argument type.


        Move boxing to runnable initialization


        Enable change_parentage test

asajeffrey · 2016-07-13T17:17:23Z

Reviewed 10 of 11 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/script_thread.rs, line 177 [r5] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

I meant why are we doing any boxing?

IRC conversation: http://logs.glob.uno/?c=mozilla%23servo&s=13+Jul+2016&e=13+Jul+2016#c479392

Comments from Reviewable

asajeffrey · 2016-07-13T17:18:12Z

@bors-servo r+

bors-servo · 2016-07-13T17:18:13Z

📌 Commit 862dc00 has been approved by asajeffrey

bors-servo · 2016-07-13T17:18:16Z

⌛ Testing commit 862dc00 with merge 3011d4b...


        Auto merge of #12404 - ConnorGBrewster:task_source_cleanup, r=asajeffrey

Clean up task sources and make all tasks cancellable  This makes it so each task is a thin wrapper over a runnable and whenever a task is queued, it is automatically wrapped by the window's `runnable_wrapper`. ---  - [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="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12404)

bors-servo · 2016-07-13T18:15:51Z

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

Add event runnables

afc0ccb

Make tasks a wrapper over runnables

highfive assigned asajeffrey Jul 12, 2016

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

cbrewster mentioned this pull request Jul 12, 2016

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

Closed

3 of 5 tasks complete

highfive assigned nox and unassigned asajeffrey Jul 12, 2016

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

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

highfive added S-awaiting-review and removed S-tests-failed labels Jul 12, 2016

cbrewster force-pushed the cbrewster:task_source_cleanup branch from 8f565d8 to 714215c Jul 12, 2016

asajeffrey self-assigned this Jul 13, 2016

Make all task source runnables cancellable

5f7324a

Implement all Runnable methods on CancellableRunnable to redirect to their inner runnable

cbrewster force-pushed the cbrewster:task_source_cleanup branch from 2b42b8f to 523cd73 Jul 13, 2016

cbrewster added 2 commits Jul 13, 2016

Move boxing to runnable initialization

ad30275

Enable change_parentage test

Loading status checks…

862dc00

cbrewster force-pushed the cbrewster:task_source_cleanup branch from 523cd73 to 862dc00 Jul 13, 2016

highfive unassigned nox Jul 13, 2016

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

bors-servo merged commit 862dc00 into servo:master Jul 13, 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 13, 2016

metajack mentioned this pull request Jul 13, 2016

intermittent solutions meta bug #12424

Open

8 of 18 tasks complete

servo / servo

Clean up task sources and make all tasks cancellable #12404

Clean up task sources and make all tasks cancellable #12404

cbrewster commented Jul 12, 2016 •

edited

highfive commented Jul 12, 2016

highfive commented Jul 12, 2016

nox commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

jdm commented Jul 12, 2016

cbrewster commented Jul 12, 2016

asajeffrey commented Jul 12, 2016

asajeffrey commented Jul 13, 2016

cbrewster commented Jul 13, 2016

asajeffrey commented Jul 13, 2016

KiChjang commented Jul 13, 2016

asajeffrey commented Jul 13, 2016

asajeffrey commented Jul 13, 2016

bors-servo commented Jul 13, 2016

bors-servo commented Jul 13, 2016

bors-servo commented Jul 13, 2016

servo / servo

Join GitHub today

Clean up task sources and make all tasks cancellable #12404

Clean up task sources and make all tasks cancellable #12404

Conversation

cbrewster commented Jul 12, 2016 • edited

highfive commented Jul 12, 2016

highfive commented Jul 12, 2016

nox commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

bors-servo commented Jul 12, 2016

jdm commented Jul 12, 2016

cbrewster commented Jul 12, 2016

asajeffrey commented Jul 12, 2016

asajeffrey commented Jul 13, 2016

cbrewster commented Jul 13, 2016

asajeffrey commented Jul 13, 2016

KiChjang commented Jul 13, 2016

asajeffrey commented Jul 13, 2016

asajeffrey commented Jul 13, 2016

bors-servo commented Jul 13, 2016

bors-servo commented Jul 13, 2016

bors-servo commented Jul 13, 2016

cbrewster commented Jul 12, 2016 •

edited