cleanup embedder/compositor/constellation/script messages #17425

paulrouget · 2017-06-20T07:52:05Z

This is work in progress. Some tests still fail.
I'd like to get early feedback as it's a pretty large PR.

There is nothing fundamentally new. Basically, I added TopLevelBrowsingContrextId to the relevant messages between the embedder, the compositor and the constellation, and enforced the PipelineId to be attached to each ScriptMsg (see #17201).

I unaliased all the ScriptMsg. It was getting difficult to understand the nature of the message as ScriptMsg was used aliased CompositorMsg sometimes (CompositorMsg is an actually type of message already). I renamed constellation_chan to script_to_constellation_chan, again, for clarification.

This cleanup code is necessary for #15934 and for tabs support.

/cc @asajeffrey can I ask you to look at this? No need for a formal review, I need feedback at this stage.

This change is

highfive · 2017-06-20T07:52:10Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs
@KiChjang: components/script/dom/dissimilaroriginwindow.rs, components/script/dom/workletglobalscope.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs and 18 more
@fitzgen: components/script/dom/dissimilaroriginwindow.rs, components/script/dom/workletglobalscope.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs and 18 more
@emilio: components/script/dom/webglrenderingcontext.rs

highfive · 2017-06-20T07:52:11Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

paulrouget · 2017-06-20T08:17:02Z

/cc @gterzian

asajeffrey

Yay, this looks a lot better! The addition of TopLevelBrowserIds seems to have cleaned the code up a lot!

asajeffrey · 2017-06-22T19:16:24Z

components/compositing/compositor.rs

-                self.on_key_event(ch, key, state, modifiers);
+                // FIXME(paul): this should be a message sent from embedder
+                // Steal a few key events for webrender debug options.
+                if modifiers.contains(CONTROL) && state == KeyState::Pressed {


It would be really nice if we didn't need this sort of hand-crafted code in servo itself, can we find a way to move it into the embedding app?

Will happen later. Filed #17647

asajeffrey · 2017-06-22T19:20:16Z

components/constellation/constellation.rs

        self.pending_changes.push(change);
    }

    /// Handles loading pages, navigation, and granting access to the compositor
    #[allow(unsafe_code)]
    fn handle_request(&mut self) {
        enum Request {
-            Script(FromScriptMsg),
+            Script((PipelineId, FromScriptMsg)),


Nit: Script(PipelineId, FromScriptMsg) rather than Script((PipelineId, FromScriptMsg))?

This is not consistent with NetworkListener. Also, it's going to make the following select map a bit more complex.

paulrouget · 2017-06-23T05:30:08Z

Thank you. I'll go ahead and finalize this PR and then ask for a formal review.

paulrouget · 2017-07-10T04:33:18Z

@gterzian can you give me an informal review of this PR? I'd like to get your input before asking for a review to Alan.

paulrouget · 2017-07-10T04:37:32Z

I still need to figure out what to do in handle_load_start_msg.

Also, I'd very much like to rename top_level_browsing_context_id to browser_id.

gterzian · 2017-07-10T13:45:06Z

@paulrouget Ok here are my comments.

Have you considered:

Replacing the 'ad hoc' Window.set_browser_id with a new WindowMethod, that would also reflect the fact that new browsers could be created on an ongoing basis(for example if the JS calls Window.open), maybe something like new_browser_created(browser_id)?
Adding a new WindowEvent::SelectBrowser(browser_id)?
Adding a new WindowEvent::NewBrowser(target_url).

Those proposals could seen as candidates to replace the current dance taking place in servo/main, simply by making WindowEvent::NewBrowser(target_url) the initial event that is sent through(replacing vec![WindowEvent::InitializeCompositing] with vec![WindowEvent::NewBrowser(target_url)]), to which Servo could respond by calling it's own create_browser method, followed by calling self.compositor.initialize_compositing() as well as self.compositor.window.new_browser_created(browser_id) once constellation has responded with a browser_id(You could even go full circle by not showing anything until window sends a WindowEvent::SelectBrowser(browser_id) in response to the new_browser_created call, after having checked internally that this was indeed the first browser that was created and none are already "selected")?

By the way this is a good example of why it could be useful for Servo to handle WindowEvents directly, as opposed to just passing those on to compositor. So the above changes should perhaps instead be made in #17269 after this one has merged...

paulrouget · 2017-07-10T14:03:32Z

Adding a new WindowEvent::SelectBrowser(browser_id)?

Agreed.

Adding a new WindowEvent::NewBrowser(target_url).

Agreed.

Replacing the 'ad hoc' Window.set_browser_id with a new WindowMethod, that would also reflect the fact that new browsers could be created on an ongoing basis(for example if the JS calls Window.open), maybe something like new_browser_created(browser_id)?

I don't think we want servo to have the ability to create new browsers. What we want is Servo to tell the embedder to handle a new url, and then up to the embedder to create a new browser.

To get the browser_id back, we would use this mechanism instead:

WindowEvent::NewBrowser(ServoUrl, IpcSender<BrowserId>)

What do you think?

Those proposals could seen as candidates to replace the current dance taking place in servo/main, simply by making WindowEvent::NewBrowser(target_url) the initial event that is sent through(replacing vec![WindowEvent::InitializeCompositing] with vec![WindowEvent::NewBrowser(target_url)])

Ok

to which Servo could respond by calling it's own create_browser method, followed by calling self.compositor.initialize_compositing()

This should actually disappear. initialize_compositing does nothing.

as well as self.compositor.window.new_browser_created(browser_id) once constellation has responded with a browser_id

Let's not use a callback for this but an IpcSender as I suggested earlier. Because we want to know which browser_id correspond to which NewBrowser request.

Think about this case:

send(NewBrowser(url1));
send(NewBrowser(url2));

…

fn new_browser_created(browser_id) {
// here we don't know which browser_id correspond to request number 1 and request number 2.
}

(You could even go full circle by not showing anything until window sends a WindowEvent::SelectBrowser(browser_id) in response to the new_browser_created call, after having checked internally that this was indeed the first browser that was created and none are already "selected")?

I think that's already the case, isn't it?

By the way this is a good example of why it could be useful for Servo to handle WindowEvents directly, as opposed to just passing those on to compositor. So the above changes should perhaps instead be made in #17269 after this one has merged...

I think we should at least use WindowEvents instead of new Servo:: methods. It makes more sense.

gterzian · 2017-07-11T02:13:13Z

@paulrouget

To get the browser_id back, we would use this mechanism instead: WindowEvent::NewBrowser(ServoUrl, IpcSender)

Yes makes sense.

I don't think we want servo to have the ability to create new browsers. What we want is Servo to tell the embedder to handle a new url, and then up to the embedder to create a new browser.

Ok I see, so servo would ask the embedder to "handle a url", to which the embedder could respond by sending a WindowEvent::NewBrowser(ServoUrl, IpcSender<BrowserId>)...

not showing anything until window sends a WindowEvent::SelectBrowser(browser_id). I think that's already the case, isn't it?

Yes it's the case, although it's servo that calls it's own select_browser method, so in the light of the above it could be instead servo handling a WindowEvent::SelectBrowser(browser_id) event. (So would require some new code in the glutin window, in response to receiving on a IpcSender<BrowserId>.

paulrouget · 2017-07-11T06:26:19Z

@gterzian Can you take a look at the 2 last commits?

Using WindowEvent instead of methods is more consistent, but it's cumbersome. I believe we should keep it that way for now for consistency, but eventually, we might want to move from messages to methods.

gterzian · 2017-07-12T02:22:30Z

I see.

Perhaps some of the cumbersomeness can be removed later by finding a way to get rid of the initial handle_events(vec![WindowEvent::NewBrowser(target_url, sender)]);, and simply have servo handle incoming events, where window would, as part of it's own initialization, fire a WindowEvent::NewBrowser(target_url, sender). In fact one could get rid of window.set_browser_id(browser_id);, since window could internally receive on the sender, and simply store the browser_id by itself...

gterzian · 2017-07-12T02:25:45Z

Maybe one way to get rid of the initial vec![WindowEvent::NewBrowser(target_url, sender)], is by adding a open_new_browser(target_url WindowMethod, which could be called in servo/main, to which window could respond by firing the initial WindowEvent::NewBrowser(target_url, sender), and storing the browser_id after having received it from the sender, which window could receive on internally, since it would be firing the event. Finally window could fire the SelectBrowser(browser_id) event...

gterzian · 2017-07-12T02:28:58Z

I guess maybe_open_new_browser(target_url) is a better name, since it would be up to the embedder to allow it or not...

paulrouget · 2017-07-12T08:52:08Z

We'll see later about the cosmetic changes. As for now, I'll keep your initial suggestion and use an event. For consistency.

Later, we will take the time to re-consider the decision to use messages for embedder -> libservo communication, and callbacks for libservo -> embedder communication.

paulrouget · 2017-07-13T07:54:57Z

Depends on ~~#17709, #17686, #17685, #17726~~

paulrouget · 2017-08-15T06:29:08Z

I made 2 changes:

better commit messages (I'm wondering if it's better to squash everything…)
fixed the test (wrong pipeline_id was passed along the PostMessage message

paulrouget · 2017-08-15T07:02:41Z

@bors-servo r=asajeffrey

bors-servo · 2017-08-15T07:02:42Z

📌 Commit 10b5266 has been approved by asajeffrey

bors-servo · 2017-08-15T07:20:12Z

⌛ Testing commit 10b5266 with merge 7455899...

@asajeffrey

cleanup embedder/compositor/constellation/script messages Fix: #17226 #17200 #17201 This is work in progress. Some tests still fail. I'd like to get early feedback as it's a pretty large PR. There is nothing fundamentally new. Basically, I added TopLevelBrowsingContrextId to the relevant messages between the embedder, the compositor and the constellation, and enforced the PipelineId to be attached to each ScriptMsg (see #17201). I unaliased all the ScriptMsg. It was getting difficult to understand the nature of the message as ScriptMsg was used aliased CompositorMsg sometimes (CompositorMsg is an actually type of message already). I renamed constellation_chan to script_to_constellation_chan, again, for clarification. This cleanup code is necessary for #15934 and for tabs support. /cc @asajeffrey can I ask you to look at this? No need for a formal review, I need feedback at this stage.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17425)

bors-servo · 2017-08-15T08:28:16Z

☀️ 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 7455899 to master...

make sure proper frame tree is sent when iframes change We ran into a hittest issue because the frame tree was not correctly updated after #17425. The frame tree should be updated on tab change (via send_frame_tree) or when the frame sub tree changes and it's the active one (via update_frame_tree_if_active). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #18101 (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/18149)

highfive assigned cbrewster Jun 20, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 20, 2017

paulrouget force-pushed the attach-pipeline-2 branch from 6b1a4b6 to cff0a33 Compare June 20, 2017 08:16

asajeffrey reviewed Jun 22, 2017

View reviewed changes

paulrouget force-pushed the attach-pipeline-2 branch from cff0a33 to 5bca2c7 Compare July 10, 2017 04:30

paulrouget force-pushed the attach-pipeline-2 branch 2 times, most recently from 45676b0 to 983dc5d Compare July 10, 2017 05:56

paulrouget force-pushed the attach-pipeline-2 branch from 983dc5d to 72ac293 Compare July 11, 2017 06:20

paulrouget force-pushed the attach-pipeline-2 branch from 72ac293 to 5e4e948 Compare July 11, 2017 07:03

paulrouget force-pushed the attach-pipeline-2 branch from 5e4e948 to fd3dbad Compare July 12, 2017 08:25

paulrouget force-pushed the attach-pipeline-2 branch 2 times, most recently from 59bead6 to 9e568f2 Compare July 13, 2017 03:02

paulrouget force-pushed the attach-pipeline-2 branch 2 times, most recently from c5cd31f to 94184f4 Compare July 13, 2017 08:08

jdm added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Jul 13, 2017

paulrouget force-pushed the attach-pipeline-2 branch from 8e0b67a to 7490396 Compare August 15, 2017 06:19

paulrouget added 4 commits August 15, 2017 08:22

make use of ScriptToConstellationChan

d241389

rename Browser to Servo as we support multiple browsers

6876e42

Event to make a browser visible

d9e7bdd

Use BrowserId and rename Browser to Servo in ports/

10b5266

paulrouget force-pushed the attach-pipeline-2 branch from 7490396 to 10b5266 Compare August 15, 2017 06:22

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

bors-servo merged commit 10b5266 into servo:master Aug 15, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 15, 2017

This was referenced Aug 15, 2017

Attach PipelineId to all ScriptMsg #17201

Closed

Remove pipeline non-compositing-logic code from compositor #17200

Closed

Tabs support: add methods to create additional top level browsing context #17226

Closed

jdm mentioned this pull request Aug 17, 2017

Link hitboxes do not move when scrolling page #18101

Closed

This was referenced Aug 19, 2017

make sure proper frame tree is sent when iframes change #18149

Merged

Clicking Browser.html interface buttons after loading a website causes Servo to become unresponsive #18098

Closed

paulrouget mentioned this pull request Sep 14, 2017

Panic during layout when encountering an iframe with no parent node and no browsing context #17482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup embedder/compositor/constellation/script messages #17425

cleanup embedder/compositor/constellation/script messages #17425

paulrouget commented Jun 20, 2017 •

edited by larsbergstrom

highfive commented Jun 20, 2017

highfive commented Jun 20, 2017

paulrouget commented Jun 20, 2017

asajeffrey left a comment

asajeffrey Jun 22, 2017

paulrouget Jul 10, 2017

asajeffrey Jun 22, 2017

paulrouget Jul 10, 2017

paulrouget commented Jun 23, 2017

paulrouget commented Jul 10, 2017

paulrouget commented Jul 10, 2017 •

edited

gterzian commented Jul 10, 2017

paulrouget commented Jul 10, 2017

gterzian commented Jul 11, 2017

paulrouget commented Jul 11, 2017

gterzian commented Jul 12, 2017

gterzian commented Jul 12, 2017 •

edited

gterzian commented Jul 12, 2017

paulrouget commented Jul 12, 2017

paulrouget commented Jul 13, 2017 •

edited

paulrouget commented Aug 15, 2017

paulrouget commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

cleanup embedder/compositor/constellation/script messages #17425

cleanup embedder/compositor/constellation/script messages #17425

Conversation

paulrouget commented Jun 20, 2017 • edited by larsbergstrom

highfive commented Jun 20, 2017

highfive commented Jun 20, 2017

paulrouget commented Jun 20, 2017

asajeffrey left a comment

Choose a reason for hiding this comment

asajeffrey Jun 22, 2017

Choose a reason for hiding this comment

paulrouget Jul 10, 2017

Choose a reason for hiding this comment

asajeffrey Jun 22, 2017

Choose a reason for hiding this comment

paulrouget Jul 10, 2017

Choose a reason for hiding this comment

paulrouget commented Jun 23, 2017

paulrouget commented Jul 10, 2017

paulrouget commented Jul 10, 2017 • edited

gterzian commented Jul 10, 2017

paulrouget commented Jul 10, 2017

gterzian commented Jul 11, 2017

paulrouget commented Jul 11, 2017

gterzian commented Jul 12, 2017

gterzian commented Jul 12, 2017 • edited

gterzian commented Jul 12, 2017

paulrouget commented Jul 12, 2017

paulrouget commented Jul 13, 2017 • edited

paulrouget commented Aug 15, 2017

paulrouget commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

bors-servo commented Aug 15, 2017

paulrouget commented Jun 20, 2017 •

edited by larsbergstrom

paulrouget commented Jul 10, 2017 •

edited

gterzian commented Jul 12, 2017 •

edited

paulrouget commented Jul 13, 2017 •

edited