Separate script and layout messages, issue #8843 #8958

jkachmar · 2015-12-13T19:09:42Z

Separated layout-specific messages to the constellation out from the ScriptMsg enum into a LayoutMsg enum within script_traits/script_msg.rs, addresses #8843.

I initially tried to move LayoutMsg into layout_traits/lib.rs, but this introduced a cyclic dependency: layout_traits depends on script_traits for the LayoutTaskFactory implementation, and script_traits/script_task.rs now depends on LayoutMsg for new layout channels in InitialScriptState and ScriptTask.

highfive · 2015-12-13T19:09:46Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

highfive · 2015-12-13T19:09:48Z

Warning

These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

bors-servo · 2015-12-15T19:50:09Z

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

Manishearth · 2015-12-16T04:20:16Z

r? @KiChjang

Manishearth · 2015-12-16T04:35:12Z

components/script/dom/bindings/trace.rs

@@ -306,6 +307,13 @@ impl JSTraceable for ConstellationChan<ScriptMsg> {
    }
 }

+impl JSTraceable for ConstellationChan<LayoutMsg> {


I think we should just use no_jsmanaged_fields! on ConstellationChan<T>?

Manishearth · 2015-12-16T04:39:47Z

components/layout_traits/Cargo.toml

@@ -22,6 +22,9 @@ path = "../net_traits"
 [dependencies.profile_traits]
 path = "../profile_traits"

+[dependencies.style_traits]
+path = "../style_traits"


Why is this necessary?

Manishearth · 2015-12-16T04:40:06Z

components/script/Cargo.toml

@@ -27,6 +27,9 @@ path = "../net_traits"
 [dependencies.profile_traits]
 path = "../profile_traits"

+[dependencies.layout_traits]
+path = "../layout_traits"


Why is this necessary? LayoutMsg is defined in script_traits

Manishearth · 2015-12-16T04:40:46Z

@bors-servo delegate=KiChjang

Looks good to me (modulo nits) at a glance; Keith can do a more thorough review

bors-servo · 2015-12-16T04:40:47Z

✌️ @KiChjang can now approve this pull request

KiChjang · 2015-12-16T05:58:18Z

-S-awaiting-review -S-needs-rebase +S-needs-code-changes

Great job so far! I've left comments in places where care must be taken when dealing with passing messages to the constellation.

The most important idea here is to recognize that the script objects should only send ScriptMsgs to the constellation - it should not, in anyway, need a ConstellationChan<LayoutMsg>. That's reserved for layout objects.

Secondly, both script and layout objects should never need to know what kind of messages (i.e. ScriptMsgand LayoutMsg) that the underlying channel accepts - all they need to know is that they are, in fact, communicating with the constellation, hence the ScriptMsg as ConstellationMsg in script files and LayoutMsg as ConstellationMsg in layout files.

Reviewed 11 of 19 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

components/compositing/constellation.rs, line 102 [r2] (raw file):
nit: Should follow the comment style of previous receivers, i.e. "Receives messages from the layout task".

components/compositing/pipeline.rs, line 85 [r2] (raw file):
This is not actually true; it's a channel for the layout task to send messages to the constellation.

components/script/dom/document.rs, line 1163 [r2] (raw file):
This sounds very wrong to me. A document is a script object, and yet it is sending a LayoutMsg to the constellation. We should instead make two copies of the ChangeRunningAnimationsState message: one for LayoutMsgs, and the other for ScriptMsgs.

components/script/dom/window.rs, line 199 [r2] (raw file):
Again, wrong idea here. It's a channel to communicate with the constellation, but given the comment I left on document.rs, we really should remove this channel altogether.

components/script/layout_interface.rs, line 21 [r2] (raw file):
LayoutMsg as ConstellationMsg

components/script/lib.rs, line 51 [r2] (raw file):
Shouldn't be necessary anymore, since the original plan of moving LayoutMsg to layout_traits was scrapped.

components/script_traits/lib.rs, line 262 [r2] (raw file):
"from layout"; but we really should remove this, since this is from the script, and scripts should only ever send ScriptMsgs to the constellation.

Comments from the review on Reviewable.io

bors-servo · 2015-12-17T12:07:34Z

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

KiChjang · 2015-12-23T04:40:10Z

-S-awaiting-review -S-needs-rebase +S-needs-code-changes

Reviewed 13 of 16 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

components/layout/layout_task.rs, line 692 [r3] (raw file):
Since there's no other type of ConstellationChan, we can just leave it named as constellation_chan.

components/script/layout_interface.rs, line 259 [r3] (raw file):
I'm confused, why is there a ConstellationChan<ScriptMsg> here?

components/script/script_task.rs, line 1203 [r3] (raw file):
There's absolutely no need for the layout to have a ConstellationChan<ScriptMsg> at all. I don't find it necessary to add a new layout_to_constellation_chan field. You can just change the message type of constellation_chan to be LayoutMsg.

Comments from the review on Reviewable.io


        Separate script and layout messages, issue #8843

KiChjang · 2015-12-26T21:54:37Z

-S-awaiting-review

@bors-servo r+

Thanks for doing this!

Reviewed 5 of 11 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

Comments from the review on Reviewable.io

bors-servo · 2015-12-26T21:54:37Z

📌 Commit 655268d has been approved by KiChjang

bors-servo · 2015-12-26T21:54:39Z

⌛ Testing commit 655268d with merge 941653d...


        Auto merge of #8958 - jkachmar:separate-layout-msg, r=KiChjang

Separate script and layout messages, issue #8843 Separated layout-specific messages to the constellation out from the `ScriptMsg` enum into a `LayoutMsg` enum within `script_traits/script_msg.rs`, addresses [#8843](#8843). I initially tried to move `LayoutMsg` into `layout_traits/lib.rs`, but this introduced a cyclic dependency: `layout_traits` depends on `script_traits` for the `LayoutTaskFactory` implementation, and `script_traits/script_task.rs` now depends on `LayoutMsg` for new layout channels in `InitialScriptState` and `ScriptTask`.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8958)

bors-servo · 2015-12-26T22:42:22Z

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

highfive added the S-awaiting-review label Dec 13, 2015

highfive added the S-needs-rebase label Dec 15, 2015

highfive assigned KiChjang Dec 16, 2015

Manishearth reviewed Dec 16, 2015
View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Dec 16, 2015

highfive added S-needs-rebase S-awaiting-review and removed S-needs-code-changes labels Dec 17, 2015

KiChjang added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Dec 23, 2015

Separate script and layout messages, issue #8843

Loading status checks…

655268d

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 26, 2015

KiChjang removed the S-awaiting-review label Dec 26, 2015

highfive added the S-awaiting-merge label Dec 26, 2015

KiChjang mentioned this pull request Dec 27, 2015

Separate script and layout messages that are sent to the constellation #8843

Closed

servo / servo

Separate script and layout messages, issue #8843 #8958

Separate script and layout messages, issue #8843 #8958

jkachmar commented Dec 13, 2015

highfive commented Dec 13, 2015

highfive commented Dec 13, 2015

bors-servo commented Dec 15, 2015

Manishearth commented Dec 16, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Dec 16, 2015

bors-servo commented Dec 16, 2015

KiChjang commented Dec 16, 2015

bors-servo commented Dec 17, 2015

KiChjang commented Dec 23, 2015

KiChjang commented Dec 26, 2015

bors-servo commented Dec 26, 2015

bors-servo commented Dec 26, 2015

bors-servo commented Dec 26, 2015

servo / servo

Join GitHub today

Separate script and layout messages, issue #8843 #8958

Separate script and layout messages, issue #8843 #8958

Conversation

jkachmar commented Dec 13, 2015

highfive commented Dec 13, 2015

highfive commented Dec 13, 2015

bors-servo commented Dec 15, 2015

Manishearth commented Dec 16, 2015

This comment has been minimized.

Manishearth Dec 16, 2015

This comment has been minimized.

Manishearth Dec 16, 2015

This comment has been minimized.

Manishearth Dec 16, 2015

Manishearth commented Dec 16, 2015

bors-servo commented Dec 16, 2015

KiChjang commented Dec 16, 2015

bors-servo commented Dec 17, 2015

KiChjang commented Dec 23, 2015

KiChjang commented Dec 26, 2015

bors-servo commented Dec 26, 2015

bors-servo commented Dec 26, 2015

bors-servo commented Dec 26, 2015