Remove compositor forwarding code and use dedicated mechanism by gterzian · Pull Request #17269 · servo/servo

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 Create a dedicated channel for constellation -> embedder communication #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:05:03Z

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:05:41Z

components/compositing/compositor_thread.rs

@@ -29,6 +29,42 @@ pub trait EventLoopWaker : 'static + Send {
    fn wake(&self);
 }



s/compositor/embedder/

cbrewster · 2017-06-13T03:06:02Z

components/compositing/compositor_thread.rs

+        }
+    }
+}
+


s/compositor/embedder/

cbrewster · 2017-06-13T03:07:07Z

components/constellation/constellation.rs

    /// 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:07:11Z

components/constellation/constellation.rs


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


nit: Add doc comment

cbrewster · 2017-06-13T03:17:00Z

components/servo/lib.rs

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


nit: indentation

cbrewster · 2017-06-13T03:17:05Z

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:18:40Z

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.

bors-servo · 2017-08-28T11:05:08Z

⌛ Testing commit 6bca340 with merge 1657ad4815034e5c27cf130f7ef77adfb1030ba3...

bors-servo · 2017-08-28T11:09:53Z

💔 Test failed - mac-rel-wpt2

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

@bors-servo: retry

servo-mac7 is constantly having git errors #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

servo-mac7 is constantly having git errors #18228

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

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

@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

servo-mac7 is constantly having git errors #18228

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

⌛ Testing commit 6bca340 with merge ef401dd...

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 There is new code that needs to be reviewed. label Jun 11, 2017

gterzian force-pushed the remove_compositor_forwarding_code_and_use_dedicated_mechanism branch from e9c224a to 88fa2fc Compare June 11, 2017 13:41

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

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

gterzian mentioned this pull request Jun 13, 2017

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

Closed

cbrewster suggested changes Jun 13, 2017

View reviewed changes

cbrewster 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 Jun 13, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Jun 14, 2017

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 Jun 17, 2017

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

highfive added the S-needs-rebase There are merge conflict errors. label Jun 21, 2017

gterzian force-pushed the remove_compositor_forwarding_code_and_use_dedicated_mechanism branch from d4e1022 to e79db2e Compare June 22, 2017 03:44

highfive assigned asajeffrey and unassigned cbrewster Aug 28, 2017

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. S-needs-rebase There are merge conflict errors. labels Aug 28, 2017

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 Aug 28, 2017

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

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 Aug 29, 2017

highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 29, 2017

bors-servo mentioned this pull request Aug 29, 2017

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

Merged

5 tasks

paulrouget mentioned this pull request Sep 12, 2017

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

Open

		@@ -29,6 +29,42 @@ pub trait EventLoopWaker : 'static + Send {
		fn wake(&self);
		}

Uh oh!

Conversation

gterzian commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 11, 2017

Uh oh!

paulrouget commented Jun 12, 2017

Uh oh!

paulrouget commented Jun 12, 2017

Uh oh!

bors-servo commented Jun 12, 2017

Uh oh!

gterzian commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gterzian commented Jun 12, 2017

Uh oh!

cbrewster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Jun 14, 2017

Uh oh!

gterzian commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gterzian commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gterzian commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulrouget commented Jun 19, 2017

Uh oh!

gterzian commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

bors-servo commented Aug 28, 2017

Uh oh!

bors-servo commented Aug 28, 2017

Uh oh!

jdm commented Aug 28, 2017

Uh oh!

bors-servo commented Aug 29, 2017

Uh oh!

bors-servo commented Aug 29, 2017

Uh oh!

paulrouget commented Aug 29, 2017

Uh oh!

jdm commented Aug 29, 2017

Uh oh!

bors-servo commented Aug 29, 2017

Uh oh!

bors-servo commented Aug 29, 2017

gterzian commented Jun 11, 2017 •

edited

Loading

gterzian commented Jun 12, 2017 •

edited

Loading

gterzian Jun 13, 2017 •

edited

Loading

gterzian commented Jun 16, 2017 •

edited

Loading

gterzian commented Jun 17, 2017 •

edited

Loading

gterzian commented Jun 19, 2017 •

edited

Loading

gterzian commented Jun 19, 2017 •

edited

Loading