Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[multi window] Constellation should not assume there's one and only one compositor #13993

Open
Tracked by #13997
paulrouget opened this issue Oct 31, 2016 · 14 comments
Open
Tracked by #13997
Labels
C-assigned There is someone working on resolving the issue

Comments

@paulrouget
Copy link
Contributor

paulrouget commented Oct 31, 2016

The constellation assume there's one and only one compositor. When Servo will support multiple windows, this won't be true anymore.

We also want to be able to start Servo with no compositor at all. A compositor requires a gl buffer, so Servo can be loaded only once a gl buffer is available. This makes the work of the embedder a bit difficult.

Things like pixel density, window size, screen size, … can be different depending on the window.

This is a required step for multi window support.

I'd imagine that we would need a hashmap of top level frames <-> compositors.

@aneeshusa
Copy link
Contributor

Something that I don't run into often, but that is very annoying when it does happen, is plugging in an external monitor with a different DPI and Firefox forcing one scaling factor for all windows. Would love to see this work properly in Servo!

@gterzian
Copy link
Member

gterzian commented Apr 23, 2017

I'd imagine that we would need a hashmap of top level frames <-> compositors.

Would it be somewhat correct to say that what we need is some combination of Tabs and BrowserManager?

The way I see it in your example in #13994 (comment), Tabs is essentially a list of top level frames, and BrowserManager is the glue between Constellation(of which I assume there will always be one only?) and a compositor. Whereas the servo field on Tabs is basically the 1 to 1 relationship "top level frame <-> compositor".

So I guess one way to look at it, in the context of your example, is that constellation would need something like a Vec<Tabs>(ignoring for a moment that Tabs act like the embedder in your example)?

@paulrouget
Copy link
Contributor Author

paulrouget commented Apr 24, 2017

I'd imagine that we would need a hashmap of top level frames <-> compositors.
Would it be somewhat correct to say that what we need is some combination of Tabs and BrowserManager?

warning: "Tabs" and "BrowserManager" are just temporary names I used in that prototype. "Tabs" are not a concept that we have in Servo. And I don't think we want that to ever happen. We talk about top level frames. "BrowserManager" is a fancy name for the constellation from the point of view of the embedder. My example assume only one window (one compositor).

We want only one constellation.

The way I see it in your example in #13994 (comment), Tabs is essentially a list of top level frames

Yes.

and BrowserManager is the glue between Constellation(of which I assume there will always be one only?)

BrowserManager is the constellation. Even though in this example it's also a link to the compositor.
Think "BrowserManager == Constellation".

and a compositor. Whereas the servo field on Tabs is basically the 1 to 1 relationship "top level frame <-> compositor".

The constellation will hold references to multiple compositors.
The constellation will hold references to multiple top level frames.
The constellation can tell a compositor/WR instance to render a top level frame (its frame tree).
A compositor can request information to the embedder (size of the framebuffer for example) via WindowMethods.
The constellation will be able to talk back to the embedder (via methods or events) without relying on the compositor (#15934).

So I guess one way to look at it, in the context of your example, is that constellation would need a HashMap(not taking into account that Tabs act like the embedder in your example)?

Tabs is really just a temporary struct I use in the embedder. Servo doesn't know about Tab and Tabs.

@gterzian
Copy link
Member

@paulrouget Ok thanks a lot for the clarification. Can we add the "assigned" level, and say I am taking on this one? I will check with @glennw re how his work on allowing multiple WR's is/can affect this issue and how I can best start to work on it...

@paulrouget paulrouget added the C-assigned There is someone working on resolving the issue label Apr 25, 2017
@paulrouget
Copy link
Contributor Author

Done.

@paulrouget
Copy link
Contributor Author

@gterzian now that #15934, do you want to work on this? I'll be happy to take care of this if you're busy with some other things.

@paulrouget
Copy link
Contributor Author

Multiple WR instances works correctly now.

It might be a bit tedious to have a hashmap instead of a direct link to the compositor as we will have to send events to the relevant compositor. Many events, like Scroll for example, don't have a TopLevelBrowsingContextId yet.

I'm also wondering how the API to create and destroy a compositor will look like.

Also, the SelectBrowser message and ToggleWebRenderDebug will need a way to refer to a compositor. So I guess we will need to introduce the notion of a CompositorId to the embedder (the same way we did with TopLevelBrowsingContextId).

Maybe events like Scroll & co could use CompositorId instead of TopLevelBrowsingContextId.

@gterzian
Copy link
Member

Ok, I can leave this one to you @paulrouget, as I was planning on first taking a first pass at the http cache. Happy to pick up some subset of this issue later on, and off course continue working on other issues with implementing window.open in mind...

@nox
Copy link
Contributor

nox commented Oct 6, 2017

@paulrouget Are you still working on this?

@paulrouget
Copy link
Contributor Author

Not at the moment, still on my todo list though.
It's ok if someone wants to work on it, I'd be happy to help.

@paulrouget
Copy link
Contributor Author

Just started to work on this.

@paulrouget
Copy link
Contributor Author

Need #19679

@paulrouget
Copy link
Contributor Author

Priorities changed. I haven't got a chance to finish this.

@paulrouget paulrouget changed the title [multi window] Constellation should not assume there's only one compositor [multi window] Constellation should not assume there's one and only one compositor Sep 5, 2018
@paulrouget
Copy link
Contributor Author

I think we should implement this in this order:

  1. separate compositor thread and embedder thread: Do not run embedding code in the same thread as the compositor code #19679
  2. make compositor optional
  3. support multiple compositors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-assigned There is someone working on resolving the issue
Projects
Status: To do
Development

No branches or pull requests

4 participants