Redesign ScriptMsg to be more specific to DOMManipulationTaskSource #9217

KiChjang · 2016-01-09T11:03:58Z

This is a large-ish PR that contains the following:

A new directory is created under components/script/ called task_source, which houses all the stuff for different task sources. Note that the ones that I have now aren't exhaustive - there are more task sources than just the generic ones.
A DOMManipulationTaskMsg which eliminates some usage of Runnables to fire events. Instead, they send event information to the DOMManipulationTaskSource and lets the ScriptTask handle all the event firing.
Re-added fn script_chan, since I can't think of any other way to give Trusted values an appropriate sender.
Rewrote step 7 of the end to make use of the DOMManipulationTaskSource

Partial #7959

KiChjang · 2016-01-09T11:53:18Z

KiChjang · 2016-01-09T11:55:16Z

components/script/dom/bindings/global.rs

+    pub fn dom_manipulation_task_source(&self) -> Box<TaskSource<DOMManipulationTaskMsg> + Send> {
+        match *self {
+            GlobalRef::Window(ref window) => window.dom_manipulation_task_source(),
+            GlobalRef::Worker(_) => unimplemented!(),


This is unimplemented because I have no idea how the Worker handles script channels.

jdm · 2016-01-09T13:28:32Z

In my own rebased PRs I've been using the MainThreadScriptChan(window.main_thread_script_chan().clone()).clone() for the target of Trusted operations.

jdm · 2016-01-09T13:42:40Z

My main objection with these changes as they stand is that the DocumentLoadsComplete and SendStorageNotification messages are now divorced from the related code, and the implementation of document loading is already complex. It may be worth reinstating the runnable concept for the DOM manipulation task source in order to accommodate this.

KiChjang · 2016-01-09T20:26:34Z

Re-added the Runnable stuff, but with a twist. I dislike the fact that we are making a generic RunnableMsg for all different kinds of tasks, so instead I have created new messages that better reflect what the purpose of each Runnable is.

bors-servo · 2016-01-10T12:01:54Z

☔ The latest upstream changes (presumably #9214) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-01-11T20:48:47Z

☔ The latest upstream changes (presumably #9257) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-01-12T05:01:24Z

☔ The latest upstream changes (presumably #9261) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-03-09T22:34:30Z

Great work! There's only one issue that still needs to be addressed, as far as I can tell. I would expect several tests in tests/wpt/web-platform-tests/workers/ to start failing with the current changes.
-S-awaiting-review +S-needs-code-changes

Reviewed 4 of 4 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

KiChjang · 2016-03-10T02:07:18Z

Yeah... I'll have to figure out a way to fix that last remaining issue.

Use DOMManipulationTaskSource whenever possible

KiChjang · 2016-03-10T06:07:21Z

Comments addressed.

jdm · 2016-03-10T20:01:25Z

@bors-servo: r+
Whee! This is a nice improvement to the status quo.

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

bors-servo · 2016-03-10T20:01:26Z

📌 Commit 3f2cbf0 has been approved by jdm

bors-servo · 2016-03-10T20:08:27Z

⌛ Testing commit 3f2cbf0 with merge 740965e...

Redesign ScriptMsg to be more specific to DOMManipulationTaskSource This is a large-ish PR that contains the following: * A new directory is created under `components/script/` called `task_source`, which houses all the stuff for different task sources. Note that the ones that I have now aren't exhaustive - there are more task sources than just the generic ones. * A `DOMManipulationTaskMsg` which eliminates some usage of `Runnable`s to fire events. Instead, they send event information to the `DOMManipulationTaskSource` and lets the `ScriptTask` handle all the event firing. * Re-added `fn script_chan`, since I can't think of any other way to give `Trusted` values an appropriate sender. * Rewrote step 7 of [the end](https://html.spec.whatwg.org/multipage/syntax.html#the-end) to make use of the `DOMManipulationTaskSource` Partial #7959  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9217)

bors-servo · 2016-03-10T21:23:56Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 9, 2016

KiChjang mentioned this pull request Jan 9, 2016

Implement HTMLDetailsElement #9216

Closed

KiChjang force-pushed the dom-manipulation-msg branch from ff54a69 to 9bb1a2e Compare January 9, 2016 11:31

highfive assigned jdm Jan 9, 2016

KiChjang reviewed Jan 9, 2016
View reviewed changes

KiChjang force-pushed the dom-manipulation-msg branch 6 times, most recently from 4759e53 to 2c4055e Compare January 9, 2016 19:06

highfive added the S-needs-rebase There are merge conflict errors. label Jan 10, 2016

KiChjang force-pushed the dom-manipulation-msg branch 2 times, most recently from 0596b11 to 67d0b5d Compare January 10, 2016 16:03

KiChjang removed the S-needs-rebase There are merge conflict errors. label Jan 10, 2016

KiChjang force-pushed the dom-manipulation-msg branch 4 times, most recently from 86c871f to 1c62bab Compare January 10, 2016 19:17

highfive added the S-needs-rebase There are merge conflict errors. label Jan 11, 2016

KiChjang force-pushed the dom-manipulation-msg branch from 1c62bab to 3a3d012 Compare January 12, 2016 00:23

KiChjang removed the S-needs-rebase There are merge conflict errors. label Jan 12, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Jan 12, 2016

KiChjang force-pushed the dom-manipulation-msg branch from 599c9cc to 764c209 Compare March 9, 2016 20:38

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 9, 2016

KiChjang added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Mar 9, 2016

KiChjang force-pushed the dom-manipulation-msg branch from 764c209 to 8f6b234 Compare March 9, 2016 21:17

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 9, 2016

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 9, 2016

KiChjang force-pushed the dom-manipulation-msg branch from 8f6b234 to 3f2cbf0 Compare March 10, 2016 06:05

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 10, 2016

Add task_source directory

3f2cbf0

Use DOMManipulationTaskSource whenever possible

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 Mar 10, 2016

bors-servo merged commit 3f2cbf0 into servo:master Mar 10, 2016

This was referenced Mar 10, 2016

Implement origin concept and browsing contextless documents. #8658

Merged

Implement Worker#terminate() (fixes #4427). #9688

Merged

[WIP] Implement the end, script execution, and everything. #9335

Closed

KiChjang deleted the dom-manipulation-msg branch March 10, 2016 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign ScriptMsg to be more specific to DOMManipulationTaskSource #9217

Redesign ScriptMsg to be more specific to DOMManipulationTaskSource #9217

KiChjang commented Jan 9, 2016

KiChjang commented Jan 9, 2016

KiChjang Jan 9, 2016

jdm commented Jan 9, 2016

jdm commented Jan 9, 2016

KiChjang commented Jan 9, 2016

bors-servo commented Jan 10, 2016

bors-servo commented Jan 11, 2016

bors-servo commented Jan 12, 2016

jdm commented Mar 9, 2016

KiChjang commented Mar 10, 2016

KiChjang commented Mar 10, 2016

jdm commented Mar 10, 2016

bors-servo commented Mar 10, 2016

bors-servo commented Mar 10, 2016

bors-servo commented Mar 10, 2016

Redesign ScriptMsg to be more specific to DOMManipulationTaskSource #9217

Redesign ScriptMsg to be more specific to DOMManipulationTaskSource #9217

Conversation

KiChjang commented Jan 9, 2016

KiChjang commented Jan 9, 2016

KiChjang Jan 9, 2016

Choose a reason for hiding this comment

jdm commented Jan 9, 2016

jdm commented Jan 9, 2016

KiChjang commented Jan 9, 2016

bors-servo commented Jan 10, 2016

bors-servo commented Jan 11, 2016

bors-servo commented Jan 12, 2016

jdm commented Mar 9, 2016

KiChjang commented Mar 10, 2016

KiChjang commented Mar 10, 2016

jdm commented Mar 10, 2016

bors-servo commented Mar 10, 2016

bors-servo commented Mar 10, 2016

bors-servo commented Mar 10, 2016