Remove compositor forwarding code and use dedicated mechanism #17269

gterzian · 2017-06-11T13:36:47Z

@paulrouget Here is a first sketch of the proposed "design", handling a first message as a proof of concept, if you could please already take a look before I move all the messages(or method calls) across... thanks

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

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

This change is

highfive · 2017-06-11T13:36:51Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs

paulrouget · 2017-06-12T03:38:18Z

Can you add a change to ports/ to show how this endup being used? Maybe you could rewrite the title update mechanism (the title of the current page is displayed in the glutin window).

paulrouget · 2017-06-12T04:08:12Z

Two examples where the compositor should not been involved:

Title of the pages changes. Embedder needs to update the window title (script to constellation to embedder).
Page reload when the user press Cmd-R. (embedder to constellation to script)

bors-servo · 2017-06-12T05:18:00Z

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

gterzian · 2017-06-12T05:48:46Z

@paulrouget I've just added a commit to support changing the page title, tested it locally and "it works". It doesn't require any changes in ports, however it does require reaching for the root pipeline inside the compositor.

gterzian · 2017-06-12T07:18:11Z

@paulrouget Ok now also added support for:

Title of the pages changes. Embedder needs to update the window title (script to constellation to embedder).
Page reload when the user press Cmd-R. (embedder to constellation to script)

cbrewster

Initial review pass

cbrewster · 2017-06-13T03:19:59Z

components/compositing/compositor_thread.rs

+pub enum EmbedderMsg {
+    /// A status message to be displayed by the browser chrome.
+    Status(Option<String>),
+    /// Alerts the compositor that the current page has changed its title.


s/compositor/embedder/

cbrewster · 2017-06-13T03:19:59Z

components/compositing/compositor_thread.rs

@@ -30,6 +30,42 @@ pub trait EventLoopWaker : 'static + Send {
 }

 /// Sends messages to the compositor.


s/compositor/embedder/

cbrewster · 2017-06-13T03:19:59Z

components/compositing/compositor_thread.rs

+    }
+}
+
+/// The port that the compositor receives messages on.


s/compositor/embedder/

cbrewster · 2017-06-13T03:19:59Z

components/constellation/constellation.rs

@@ -171,6 +171,8 @@ pub struct Constellation<Message, LTF, STF> {
    /// A channel for the constellation to receive messages from the compositor thread.
    compositor_receiver: Receiver<FromCompositorMsg>,

+    embedder_proxy: EmbedderProxy,


nit: Add doc comment

cbrewster · 2017-06-13T03:19:59Z

components/constellation/constellation.rs

@@ -299,6 +301,8 @@ pub struct Constellation<Message, LTF, STF> {

 /// State needed to construct a constellation.
 pub struct InitialConstellationState {
+    pub embedder_proxy: EmbedderProxy,


nit: Add doc comment

cbrewster · 2017-06-13T03:19:59Z

components/servo/lib.rs

+    (EmbedderProxy {
+         sender: sender,
+         event_loop_waker: event_loop_waker,
+        },


nit: indentation

cbrewster · 2017-06-13T03:19:59Z

components/servo/lib.rs

+                (_, _) => {},
+            }
+        }
+        for event in events.clone() {


Since the variants of WindowEvent handled by the embedder and the compositor are disjoint, it would be better to have two separate types. Maybe the window should store events for the compositor and events for the embedder?

Good point. I would almost be tempted to think that all window events should be handled by Browser, which could call the appropriate methods of compositor, or handle those differently(such as in the cases where the compositor isn't involved). Maybe this could also help support multiple compositors down the line? @paulrouget

So in other words, all window events could be handled here, and Browser could either call a method of 'window', or 'compositor', or both, in response to each event.

cbrewster · 2017-06-13T03:19:59Z

components/servo/lib.rs

+    fn handle_window_event(&mut self, event: WindowEvent) {
+        match event {
+            WindowEvent::Reload => {
+                let top_level_browsing_context_id = match self.compositor.root_pipeline {


Is the plan to eventually allow the embedder to handle the current top_level_browsing_context_id?

cc @paulrouget

Eventually, the embedder will have a list of top_level_browsing_context_id (tabs).

So asking the compositor for the root pipeline to get the context id will be be a temporary thing.

bors-servo · 2017-06-14T15:34:09Z

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

gterzian · 2017-06-16T10:42:38Z

@cbrewster thanks for the review so far, I'm on standby for a bit more "big picture" guidance from @paulrouget...

gterzian · 2017-06-17T06:26:58Z

OK so I've done one more thing to just follow along the current line, and learned some stuff along the way:

I moved all WindowEvent handling to Browser, which calls compositor methods in response.
The one catch is when we are shutting down. If you look at the current (on master) implementation of compositor.handle_browser_messages, you can see that the default is to return true, while in the case that the compositor has or is about to shutdown, it returns false. There shouldn't be any more WindowEvent handling or receiving of messages(either by the compositor or a dedicated embedder channel) after that. I'm currently handling this here. This will also be relevant if we move towards a 'pull' model of event handling from the window(through something like window.get_events)...
One thing worth noting is that I tried to only have the compositor and browser receive messages in response to the WindowEvent::Idle and WindowEvent::InitializeCompositing, and that would freeze up the browser. This could be relevant if we move to an explicit browser.perform_updates method call by the embedder, as it would mean that doing so only in response to WindowEvent::Idle wouldn't be "enough". (to be confirmed, maybe there was some other reason for the freeze-up)

My conclusion so far on this experiment:

Compositor doesn't need to be handling window events directly.
A "browser" or "servo" could do all the event handling, either by waiting on events like now, or in response to a call to perform_updates from the embedder.
A "browser" or "servo" could also receive messages directly from constellation, entirely by-passing the compositor where appropriate.
A "browser" or "servo" could also tell compositor to receive messages(or "perform updates").
A "browser" or "servo" could essentially be the only embedder facing component, by handling window events, and calling window methods, as well as providing the main entry point for the event loop through a perform_updates method.
I speculate that having a "browser" or "servo" doing all of the above, rather then the current setup where compositor is essentially doing it, would facilitate having multiple windows and/or multiple compositor.

gterzian · 2017-06-19T07:16:28Z

@paulrouget Ok just to continue the current experiment, I made an attempt to move all messages from constellation that don't relate to the compositor to the dedicated "embedder" channel, and I've also moved all window event handling from compositor to browser. I think the first part is the stated goal of this issue, and I'm wondering if we shouldn't include moving all window event handling as I did as well.

Worth noting is that the way the compositor was previously handling it's "own" messages, as well as window events, was intertwined with the ShutDownState of the compositor. So I've tried to replicate the logic in browser.handle_events, which required breaking up compositor.handle_events into compositor.perform_updates and compositor.receive_messages...

When I run servo, I keep getting a called `Option::unwrap()` on a `None` value (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }, at src/libcore/option.rs:335) just before shutdown(which doesn't seem to crash servo in it's entirety), which I haven't been able to solve yet.

Also when I tried to run some tests, it seemed very slow, and I'm not sure if it's just my machine or if I introduce something that slows down the event loop or something else...

(note: the PR still contains debug messages).

paulrouget · 2017-06-19T07:40:28Z

@gterzian your work and my work may conflict. It's work in progress, but you might want to take a look: master...paulrouget:attach-pipeline-wip

gterzian · 2017-06-19T11:14:44Z

@paulrouget There will defenitely be a huge "conflict" in terms of git 😄 , however the direction you're moving to is very compatibly with what I am trying to do here, since it seems to be moving logic out of the compositor. If "browser/servo" handles WindowEvent, as in this PR, it would simply be "browser/servo" that would be sending ConstellationMsg, and inside constelation we would obbiously have to import them as FromEmbedderMsg, as opposed to FromCompositorMsg.

bors-servo · 2017-06-21T16:30:34Z

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

jdm · 2017-08-28T22:52:10Z

@bors-servo: retry

#18228

bors-servo · 2017-08-29T03:13:47Z

⌛ Testing commit 6bca340 with merge 9307f29805768714a3dee54a32b0e9b672ccfd92...

bors-servo · 2017-08-29T03:49:58Z

💔 Test failed - mac-rel-wpt2

paulrouget · 2017-08-29T04:36:23Z

Same error multiple times in a row. What does it mean?

exceptions.Exception: Actual commit (f3f80c0dca7408863cfa1a6276862f0927b7947a) differs from requested commit (9307f29805768714a3dee54a32b0e9b672ccfd92)

jdm · 2017-08-29T14:49:27Z

@bors-servo retry

#18228

bors-servo · 2017-08-29T16:25:57Z

⌛ Testing commit 6bca340 with merge 026872b...


        Auto merge of #17269 - gterzian:remove_compositor_forwarding_code_and…

@paulrouget

…_use_dedicated_mechanism, r=asajeffrey Remove compositor forwarding code and use dedicated mechanism  @paulrouget Here is a first sketch of the proposed "design", handling a first message as a proof of concept, if you could please already take a look before I move all the messages(or method calls) across... thanks ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #15934 (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/17269)

bors-servo · 2017-08-29T16:26:11Z

💔 Test failed - mac-rel-wpt1

jdm · 2017-08-29T16:28:34Z

@bors-servo: retry

#18228

bors-servo · 2017-08-29T17:58:00Z

⌛ Testing commit 6bca340 with merge ef401dd...


        Auto merge of #17269 - gterzian:remove_compositor_forwarding_code_and…

@paulrouget

…_use_dedicated_mechanism, r=asajeffrey Remove compositor forwarding code and use dedicated mechanism  @paulrouget Here is a first sketch of the proposed "design", handling a first message as a proof of concept, if you could please already take a look before I move all the messages(or method calls) across... thanks ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #15934 (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/17269)

bors-servo · 2017-08-29T19:10:03Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: asajeffrey
Pushing ef401dd to master...

gterzian · 2017-08-30T15:12:41Z

Thank you all for your help.

highfive assigned cbrewster Jun 11, 2017

highfive added the S-awaiting-review label Jun 11, 2017

gterzian force-pushed the gterzian:remove_compositor_forwarding_code_and_use_dedicated_mechanism branch from e9c224a to 88fa2fc Jun 11, 2017

highfive added the S-needs-rebase label Jun 12, 2017

gterzian force-pushed the gterzian:remove_compositor_forwarding_code_and_use_dedicated_mechanism branch 2 times, most recently from 88657cf to b0ffab7 Jun 12, 2017

gterzian mentioned this pull request Jun 13, 2017

Create a dedicated channel for constellation -> embedder communication #15934

Closed

cbrewster requested changes Jun 13, 2017

View changes

cbrewster added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Jun 13, 2017

highfive added the S-needs-rebase label Jun 14, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 17, 2017

gterzian force-pushed the gterzian:remove_compositor_forwarding_code_and_use_dedicated_mechanism branch from 01c4eaf to 186139c Jun 17, 2017

highfive added the S-needs-rebase label Jun 21, 2017

gterzian force-pushed the gterzian:remove_compositor_forwarding_code_and_use_dedicated_mechanism branch from d4e1022 to e79db2e Jun 22, 2017

highfive added the S-tests-failed label Aug 28, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 29, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 29, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 29, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 29, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 29, 2017

bors-servo merged commit 6bca340 into servo:master Aug 29, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 29, 2017

bors-servo mentioned this pull request Aug 29, 2017

Enable screen.width/height/availWidth/availHeight #18183

Merged

4 of 5 tasks complete

gterzian deleted the gterzian:remove_compositor_forwarding_code_and_use_dedicated_mechanism branch Aug 30, 2017

paulrouget mentioned this pull request Sep 12, 2017

Move the callback-to-events translation to servo paulrouget/servoshell#39

Open

servo / servo

Remove compositor forwarding code and use dedicated mechanism #17269

Remove compositor forwarding code and use dedicated mechanism #17269

gterzian commented Jun 11, 2017 •

edited

highfive commented Jun 11, 2017

paulrouget commented Jun 12, 2017

paulrouget commented Jun 12, 2017

bors-servo commented Jun 12, 2017

gterzian commented Jun 12, 2017 •

edited

gterzian commented Jun 12, 2017

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jun 14, 2017

gterzian commented Jun 16, 2017 •

edited

gterzian commented Jun 17, 2017 •

edited

gterzian commented Jun 19, 2017 •

edited

paulrouget commented Jun 19, 2017

gterzian commented Jun 19, 2017 •

edited

bors-servo commented Jun 21, 2017

jdm commented Aug 28, 2017

bors-servo commented Aug 29, 2017

bors-servo commented Aug 29, 2017

paulrouget commented Aug 29, 2017

jdm commented Aug 29, 2017

bors-servo commented Aug 29, 2017

bors-servo commented Aug 29, 2017

jdm commented Aug 29, 2017

bors-servo commented Aug 29, 2017

bors-servo commented Aug 29, 2017

gterzian commented Aug 30, 2017

		@@ -30,6 +30,42 @@ pub trait EventLoopWaker : 'static + Send {
		}

		/// Sends messages to the compositor.

servo / servo

Join GitHub today

Remove compositor forwarding code and use dedicated mechanism #17269

Remove compositor forwarding code and use dedicated mechanism #17269

Conversation

gterzian commented Jun 11, 2017 • edited

highfive commented Jun 11, 2017

paulrouget commented Jun 12, 2017

paulrouget commented Jun 12, 2017

bors-servo commented Jun 12, 2017

gterzian commented Jun 12, 2017 • edited

gterzian commented Jun 12, 2017

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Jun 13, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jun 14, 2017

gterzian commented Jun 16, 2017 • edited

gterzian commented Jun 17, 2017 • edited

gterzian commented Jun 19, 2017 • edited

paulrouget commented Jun 19, 2017

gterzian commented Jun 19, 2017 • edited

bors-servo commented Jun 21, 2017

jdm commented Aug 28, 2017

bors-servo commented Aug 29, 2017

bors-servo commented Aug 29, 2017

paulrouget commented Aug 29, 2017

jdm commented Aug 29, 2017

bors-servo commented Aug 29, 2017

bors-servo commented Aug 29, 2017

jdm commented Aug 29, 2017

bors-servo commented Aug 29, 2017

bors-servo commented Aug 29, 2017

gterzian commented Aug 30, 2017

gterzian commented Jun 11, 2017 •

edited

gterzian commented Jun 12, 2017 •

edited

gterzian Jun 13, 2017 •

edited

gterzian commented Jun 16, 2017 •

edited

gterzian commented Jun 17, 2017 •

edited

gterzian commented Jun 19, 2017 •

edited

gterzian commented Jun 19, 2017 •

edited