Clean up task sources and make all tasks cancellable by cbrewster · Pull Request #12404 · servo/servo

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 Intermittent crash in /html/semantics/embedded-content/the-iframe-element/change_parentage.html (Option::unwrap on a None value) #11703 (github issue number if applicable).

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

This change is

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...

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

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.

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...

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 There is new code that needs to be reviewed. label Jul 12, 2016

cbrewster mentioned this pull request Jul 12, 2016

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

Closed

5 tasks

highfive assigned nox and unassigned asajeffrey Jul 12, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 12, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 12, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 12, 2016

cbrewster force-pushed the task_source_cleanup branch from 8f565d8 to 714215c Compare July 12, 2016 19:58

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 task_source_cleanup branch from 2b42b8f to 523cd73 Compare July 13, 2016 16:00

cbrewster added 2 commits July 13, 2016 11:10

Move boxing to runnable initialization

ad30275

Enable change_parentage test

862dc00

cbrewster force-pushed the task_source_cleanup branch from 523cd73 to 862dc00 Compare July 13, 2016 17:10

highfive unassigned nox Jul 13, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 13, 2016

bors-servo merged commit 862dc00 into servo:master Jul 13, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 13, 2016

metajack mentioned this pull request Jul 13, 2016

intermittent solutions meta bug #12424

Open

18 tasks

Uh oh!

Conversation

cbrewster commented Jul 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jul 12, 2016

Uh oh!

highfive commented Jul 12, 2016

Uh oh!

nox commented Jul 12, 2016

Uh oh!

bors-servo commented Jul 12, 2016

Uh oh!

bors-servo commented Jul 12, 2016

Uh oh!

bors-servo commented Jul 12, 2016

Uh oh!

jdm commented Jul 12, 2016

Uh oh!

cbrewster commented Jul 12, 2016

Uh oh!

asajeffrey commented Jul 12, 2016

Uh oh!

asajeffrey commented Jul 13, 2016

Uh oh!

cbrewster commented Jul 13, 2016

Uh oh!

asajeffrey commented Jul 13, 2016

Uh oh!

KiChjang commented Jul 13, 2016

Uh oh!

asajeffrey commented Jul 13, 2016

Uh oh!

asajeffrey commented Jul 13, 2016

Uh oh!

bors-servo commented Jul 13, 2016

Uh oh!

bors-servo commented Jul 13, 2016

Uh oh!

bors-servo commented Jul 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cbrewster commented Jul 12, 2016 •

edited

Loading