Create a dedicated channel for constellation -> embedder communication

[paulrouget]

This issue is part of an effort to improve Servo's embedding story. See https://github.com/paulrouget/servoshell/projects/2

Follow up of #15795.

As of now, Servo communicates with the embedder by calling the WindowMethods, in the main thread. These methods are called from the compositor, and the main thread is woken up via the compositor proxy.

A subset of these methods are relevant to the compositor (framebuffer_size(), window_rect(), …) and some are not (set_page_title(), status(), …).

Also, some of these calls need to be synchronous (framebuffer_size()) some don't (set_cursor()).

As Alan noted in #15795:

this seems to be conflating two (at least conceptually) different threads: the compositor, and the embedding application"

So to make the embedding API more sensible (separation of concerns), it would be great to have a dedicated mechanism for the constellation to communicate with the embedder.

This channel should be between the constellation and the embedder.

We would end up with 2 channels between the servo and the embedder: constellation -> embedder and compositor -> embedder. With the embedder code running in the main thread.

Ideally, both channels would follow the same pattern: sync calls would be methods, async calls would be events, and the embedder would provide a mechanism to both channels to wake up the main thread.

We would need to identify in the current WindowMethods what falls under compositor and constellation concerns.

[gterzian]

Ok, please add the "assigned" label to this one as well, I'll take it on as well...

[gterzian]

Here is my understanding of the situation, to be confirmed by feedback, followed by an implementation plan based thereon:

As it currently stands:

1. In the default implementation in ports::servo, ports::glutin::window::Window; is the "embedder".
2. Window is responsible for creating the compositor sender and receiver channel, via window.create_compositor_channel, which returns a pair of GlutinCompositorProxy(implementing CompositorProxy) and a CompositorReceiver.
3. The compositor only communicates with Window through self.window, using the WindowMethods.
4. The constellation communicates with the compositor via the GlutinCompositorProxy created by Window, which is cloned and passed to the constellation upon instantiation here
5. Webrender also uses the CompositorReceiver, again created initially by Window.

The problem I see with the above, is that it appears that the embedder Window is responsible for creating the communication channel which is used by servo internals.

If that is correct, I propose that this issue also involves making the GlutinCompositorProxy as independent from Window, the "embedder", as possible, as well as creating another channel for "constellation -> embedder" communication, and keeping the "compositor -> embedder" communication via WindowMethods where appropriate.

The result would be:

A. A new "constellation -> embedder" channel, that bypasses the compositor. Candidates for that are a bunch of messages currently handled inside compositor.handle_browser_messages. and are currently just forwarded to Window via WindowMethods.
B. "compositor -> embedder" communicate via compositor.window. I think most stuff happening in handle_window_messages are good candidates for that.
C. "constellation -> compositor -> webrender" communicate via a new proxy, created by Browser, that essentially is the same as the current GlutinCompositorProxy except that it isn't coupled to the embedder anymore.

So I propose to implement the following solution:

1. Have servo::lib::Browser create the communication channel to be used for "constellation -> compositor" and "compositor -> webrender" communication. It would be an implementation of CompositorProxy and CompositorReceiver(Maybe BrowserCompositorProxy?) and would replace the functionality of the current GlutinCompositorProxy.
2. Have Window have a method returning a Option<glutin::WindowProxy>, the window proxy would be stored by, and used, the proxy described under 1 to wake up the event loop. That way we reduce coupling from the embedder to the internal communication channel to the need to wake up the main thread, via the window proxy(Do we really need to wake up the main thread for all communication between constellation and compositor? If not, this is perhaps unnecessary).
3. Have Window create a channel to receive messages directly from the constellation. It could essentially be the same thing as the current GlutinCompositorProxy, which the constellation would store, and send the appropriate messages on, and a CompositorReceicer, which Window would store on a port field and listen on messages on, as well as handle those messages, perhaps calling its own WindowMethods as a result. I'm wondering if we can just use the compositor specific types like compositor_thread::Msg and CompositorProxy, or if we should have embedder specific ones...

Please let me know what you think. @paulrouget @asajeffrey

[gterzian]

After some additional consideration, I'm wondering if 1 and 2 in the implementation plan above are really necessary, from this comment I can tell that the coupling of the client/embedder with the channel/event loop is intentional, in which case we could focus on 3 in this issue.

To 3 I would like to add the need for the embedder to be aware in some way of the shutdown state of the compositor. Perhaps the constellation should forward messages pertaining to this to the embedder?

[paulrouget]

The problem I see with the above, is that it appears that the embedder Window is responsible for creating the communication channel which is used by servo internals.

If I'm not mistaken, GlutinCompositorProxy is only used to wake up glutin's event loop. It's just a proxy. Glutin blocks the main thread until user interaction happens (triggering events). But sometimes, Servo performs action that need to wake up the main thread (from where, in the embedder, we call handle_events(events). That's what the proxy is used for.

This is confusing. Ideally, this notion of CompositorProxy would disappear and the embedder would simply provide a "EventLoopRiser" thread safe object that could be used from both the compositor and the constellation.

Here is how I see it:

1. Communication Servo -> Embedder is done via WindowMethods. Here, 2 kind of communications happen: gl/window/compositor related messages (window size, present, …) and browser/constellation related messages (title change, url change, …).

2. Communication Embedder -> Servo is done via the handle_events(Vec<WindowEvents>), which again, are of 2 different kind: window stuff (Resize, MouseWindowEventClass, Scroll, …) and browser/constellation stuff (LoadUrl, Navigation, Reload…).

1 is done via compositor.window.foo where foo is a WindowMethod.
2 is done via compositor.handle_events.

So even for non-compositor stuff, we always go through the compositor, because it's the only way we have to communicate back and forth with the emebedder.

So we need another mechanism for the constellation.

I am not providing any solution to the initial problem here, just describing how things work. Let me know what you think.

Also - do not hesitate to consider getting rid of some existing concepts. For example, I don't think we have to keep WindowMethods and CompositorProxy.

[paulrouget]

PS: when you create a link to a line of code, press y before copying the link. It will add the commit hash to the url, making sure the link is always valid even if the file changes.

[gterzian]

Thanks for the additional info and the github protip. 😄

If I'm not mistaken, GlutinCompositorProxy is only used to wake up glutin's event loop. It's just a proxy.

I agree with the above, and would also like to state the obvious which is that the messages that are being proxied are currently all like "constellation -> compositor <- webrender", with the compositor sitting in between all communication, and sometimes forwarding some messages to the embedder via the window. As this issues has pointed out, some of those messages actually belong in a separate constellation -> embedder channel.

I can't prove it, but I have a feeling that the create_compositor_channel -> (Box<CompositorProxy + Send>, Box<CompositorReceiver>) is partly responsible for the problem of all communication going through the compositor regardless of whether it relates to it.

create_compositor_channel is one of WindowMethods, yet it requires the embedder to implement a method that returns a (Box<CompositorProxy + Send>, Box<CompositorReceiver>), for which the embedder has to use compositing::compositor_thread::{self, CompositorProxy, CompositorReceiver}; as well as compositor_thread::Msg.

Perhaps the above might have promoted something along the lines of "window is creating this compositor communication channel, so we can put everything through it, both related to compositor and window"?

As a first step towards better separation of communication channels, I would like to propose the following:

1. replace create_compositor_channel with a simple wake_up_event_loop(&self), in WindowMethods.
2. Have Browser implement CompositorProxy for a BrowserCompositorProxy struct, which would have a window field, and a send(&self, msg: compositor_thread::Msg) method that would simply call self.window.wake_up_event_loop() when necessary.

The benefits of this refactoring?

• it becomes suddenly very weird to send messages that belong in a direct communication channel between the constellation and the embedder, through this BrowserCompositorProxy.
• We achieve better separation of concerns: Browser is responsible for creating the communication glue between the various servo internal components, and window is only responsible for waking up it's main event loop as appropriate.

Once we have the above, we can start thinking about the best structure for the "constellation -> embedder" channel(s) of communication. I can think of the following elements that could be needed:

• something along the lines of a WindowMsg enum, to be added to compositing::windowing
• a Sender<WindowMsg>, to be used by the constellation.
• a Receiver<WindowMsg>, to be used by window to listen for messages, calling it's own WindowMethods when appropriate.
• a create_servo_channel -> Box<Sender<WindowMsg> + Send>, to be added to WindowMethods, used by Browser to pass the Sender<WindowMsg> along to the constellation.
• if we want to send messages from the embedder to constellation, in lieu of using handle_events, window could have a new Sender<ConstellationMsg> field.

[paulrouget]

replace create_compositor_channel with a simple wake_up_event_loop(&self), in WindowMethods.

Will you be able to call this function ever? At that point the main thread is blocked.

[paulrouget]

We need a function that returns a thread safe object that can wake up the event loop from any thread. Maybe something like this:

fn create_event_loop_riser(&self) -> Box<EventLoopRiser>;

pub trait EventLoopRiser {
    fn clone(&self) -> Box<EventLoopRiser + Send>;
    fn rise(&self);
}

[paulrouget]

Also - I agree that the first step should be to move the CompositorProxy-related things to Browser.

[gterzian]

@paulrouget thanks, I think it's time to write a first draft of this replacement of CompositorProxy, and see what the compiler has to say about the design...

[paulrouget]

Once #17068 lands, this is what I think should happen next:

1. in the compositor, all the pipeline work that is not related to compositing need to be striped down to its most simple form. The compositor code in these cases should just be used to forward messages to the constellation. For example, the Reload message handler work that way today:

Embedder send WindowEvent::Reload to compositor.
Compositor find the pipeline to reload and its browsing context.
Compositor send ConstellationMsg::Reload(browsing_context) to constellation.

What we want is either:

Embedder send WindowEvent::Reload(browsing_context) to compositor.
Compositor forward to constellation.

… or:

Embedder send WindowEvent::Reload to compositor.
Compositor forward to constellation.
Constellation find browsing context.

2. remove all the forwarding code from compositor and use a dedicated mechanism.

Where basically the compositor doesn't need to forward anymore.

I don't know how to do 2) yet. 1) needs to happen soon as we want to land #17077.

Greg, if that works for you, I'd like to take care of 1). I let you handle 2).

[gterzian]

@paulrouget ok, I'll take care of 2)

[paulrouget]

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

[gterzian]

In the light of the current 'proof of concept' at #17269 I'm starting to think that 'libservo/Browser' could become the main piece of glue between the embedder and other servo internals, this could also facilitate further improvements like multiple compositors and/or multiple "tabs"(Browser currently has one compositor, so I guess that could be the place to keep track of several ones) .

Regarding embedder => constellation communication, perhaps 'WindowMethods' and 'WindowEvents' is still the way to go, with the main change being:

• it would be 'Browser' that would call the 'WindowMethods'(in response to EmbedderMsg, which in fact are just repurposed compositor_thread::Msg that don't really deal with compositing), see https://github.com/gterzian/servo/blob/887289e2628f714de8c5787a0c72c527957502ba/components/servo/lib.rs#L270
• and it would also be 'Browser' that would handle 'WindowEvents' sent by the embedder. See https://github.com/gterzian/servo/blob/887289e2628f714de8c5787a0c72c527957502ba/components/servo/lib.rs#L253

@paulrouget I'm still looking into your Tabs prototype at paulrouget@6f3bdef and so far it seems that we're thinking of something similar, with the difference being that I would just put everything into Browser(and perhaps indeed rename it Servo), essentially doing what you are doing with window.get_servo_events and servo.new_browser directly inside Browser/Servo.

I guess once this would be in place, it would move us one step further to offering some sort of 'Tabs' to the embedder by way of an API, perhaps using "window.open"?

[gterzian]

@paulrouget There was a very good point of @cbrewster here #17269 (comment) about whether we should have a new type of event for the stuff that isn't handled by the compositor, which made me think of the BrowserEvent that you introduced in your prototype, and also made me realize a bit more what I was trying to write above:

Taking the "tabs prototype" found at paulrouget@6f3bdef as a starting point:

1. If you look at BrowserEvent::SetUrl(ctx, url) you can see it is created in response to a call to history_changed(&self, ctx: TopLevelBrowsingContextId, history: Vec<LoadData>, current: usize), which is called by the compositor, in response to receiving a Msg::HistoryChanged(ctx, entries, current) from the constellation. In the end, the tabs prototype uses the ctx to find the appropriate tab, and updates the url.
2. The "constellation => compositor => window" communication pattern is a good example of what we are trying to remove in this issue.
3. The additional "window => tabs" communication pattern, by way of an additional event loop, could be made unnecessary if 1) Browser would handle the initial Msg::HistoryChanged from the constellation directly(the purpose of this issue, by removing the compositor), and 2) Browser would have the capabilities that the tabs prototype currently has(essentially the ability to handle some WindowEvent directly, and something like a Vec<<ServoUrl, TopLevelBrowsingContextId>>).

Making those observations, I am getting more convinced of the need for Browser(perhaps renamed as Servo) to be the main glue between the embedder and servo internals. So in fact, instead of going for a "constellation -> embedder" type of communication, it could be "constellation -> browser -> embedder", where:

• Browser handles all WindowEvents, and in response either calls compositor methods(or sends messages to the compositor), or WindowMethods.
• Browser handles messages directly from constellation, those messages are essentially constellation_thread::Msg which don't related to compositing, and can be re-purposed into a new EmbedderMsg. Again browser can call compositor methods, send compositor messages, and/or WindowMethods in response to these messages.
• WindowEvents remain the sole mean of communication for embedder => Browser, and WindowMethods the sole mean of communication fro Browser => embedder.

The result, going back to the history changed example, would allow us to do something like:

• constellation sends a Msg::HistoryChanged
• Browser handles that message, updating it's internal data of ServoUrl, TopLevelBrowsingContextId pairs.
• Browser lets the embedder know about the change via window.history_changed.

Other operations initiated by the embedder, like 'opening a new tab' or 'selecting a new tab' would be communicated via WindowEvents, again handled by Browser, which could send messages to constellation, call compositor methods(or send messages on a clone of compositor_proxy), and so on...

Lastly, it could be a really good idea to rename Browser into Servo, otherwise potential embedders will for eternity think in terms of browsers with tabs... 😄

[paulrouget]

I agree with what you're saying. So we could rename "Browser" to "Constellation" or "Servo".

How would we create a new browser (TopLevelBrowsingContext)? And how would that look like from the point of view of the embedder?

This is what I'd like the final API to look like, more or less: https://gist.github.com/paulrouget/248c0d7bd308242c43e05648b1245fab

Look at the example at the end of the file and tell me what you think.

[paulrouget]

So what we want to do for this issue is:

• by-pass the compositor when it makes sense
• move the window object to servo/lib.rs
• maybe rename browser to servo (or constellation)

[paulrouget]

Browser handles that message, updating it's internal data of ServoUrl, TopLevelBrowsingContextId pairs.

I disagree with that step. Browser should not have any internal state. Nothing to update, just redirect messages.

The embedder will have a list of (ServoUrl, TopLevelBrowsingContextId) pairs, not Browser.

[gterzian]

@paulrouget thanks.

First of all, I would like to confirm my understanding of the "event loop", to make sure I am not missing something in the context of this discussion. Is the following correct regarding the current Glutin implemention:

• When the embedder receives a Glutin::Event, it is 'translated' to a windowing::WindowEvent, and passed along libservo::Browser.handle_event. Glutin events can either come from the OS window system, or from calling EventLoopWaker.wake from inside servo. In both cases, the "event loop" is kicked awake.
• Browser.handle_event simply passes incoming events on to compositor, via compositor.handle_events.
• Inside compositor.handle_events, the compositor will first receive any incoming messages on the compositor port. After that, compositor will handle the window events. The compositor channel requires an EventLoopWaker, because in order to "receive" messages, compositor.handle_events needs to be called in response to a new window event.

If the above is correct, whatever we do, we will still need to call compositor.handle_events everytime for every window event, otherwise the compositor channel will never receive messages. (So we can't completely by pass the compositor when handling window events)

[gterzian]

By the way, it seems the answer to my question above is found in #15734
It seems that the event that is sent when the embedder event loop is kicked awake is WindowEvent::Idle (the compositor doesn't do anything specific in response to this event, but receiving it gives it a chance to receive messages on it's port)

[gterzian]

ok @paulrouget I think I now understand better(again!) what it is that I am trying to propose here, after having read about your proposed perform_update method in #15734 and seen it used in your proposed api.

Essentially, what I had in mind is the 'reverse' of you: have Browser handle WindowEvent::Idle internally, and call WindowMethods and/or compositor methods as appropriate, wherease you have the embedder handle WindowEvent::Idle, and pull all events from browser, calling WindowMethods in response to those events.

However, I think the part of your proposal that has the embedder call perform_updates is better, because it makes "making updates" explicit, as opposed to mixing it with window event handling.

And I also think we could consider hiding the compositor from the embedder, passing the native_window along with the call to perform_updates, and also keep 'listening' for window events as they come in.

I would propose the following changes to your api, assuming we also rename Browser to Servo in libservo:

• When a WindowEvent::Idle event comes in, the embedder calls servo.perform_updates(native_window)(note we're passing the window now). Internally Servo can receive all it's internal messages, and then call WindowMethods on the native window passed along by the embedder. I think this would remove the need for the additional servo => embedder event loop found in your proposal (instead of having the embedder call fn get_events() -> Vec<EventFromBrowser>; and call window methods on it's native window, servo can call those methods directly).

Example: Constellation sends a EmbedderMsg::ChangePageTitle(pipeline_id, title) on the embedder proxy => embedder receives WindowEvent::Idle and calls servo.perform_updates(native_window) => servo receive the change title message from constellation and calls native_window.set_page_title(title).

• We still have servo listen for events coming from the embedder, and call appropriate compositor methods, or send messages to constellation, in response. Otherwise wouldn't we still end up mixing the two concepts? And wouldn't we only act on something like a WindowEvent::Reload after a WindowEvent::Idle(If we only pull events by calling window.get_events inside browser.perform_updates as you suggested in Can we make it so the embedder doesn't need to call Browser::handle_event() when there's no event? #15734 (comment))?

Example: The embedder sends a WindowEvent::Reload(which would include the window id?) => servo receives the event, gets the top_level_browsing_context_id from a compositor, and sends a ConstellationMsg::Reload(top_level_browsing_context_id).

The result is that we really separate "handling window events from the embedder" from "pulling internal messages in response to a event loop wake up", making the later an explicit perform_updates call from the embedder on servo, while still listening on 'real' window events and reacting to those as they come in. We also don't include compositor in the embedder api, and we don't introduce another event loop to communicate back to the embedder.

[paulrouget]

I'll respond to your comments a bit later this week.

I'm working on #17201 and #17200, and these might have an impact on the current architecture.

[paulrouget]

Thank you @gterzian