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 be able to handle multiple root frame #13994

Closed
Tracked by #13997
paulrouget opened this issue Oct 31, 2016 · 15 comments · Fixed by #17077
Closed
Tracked by #13997

[multi window] Constellation should be able to handle multiple root frame #13994

paulrouget opened this issue Oct 31, 2016 · 15 comments · Fixed by #17077
Assignees

Comments

@paulrouget
Copy link
Contributor

At the moment, a constellation has only one root frame.

When Servo will support multiple windows, we would need multiple root frames (and multiple compositors, see #13993). We might also want to have multiple root frames for one compositor (non-mozbrowser based embedder will have one root frame per tab).

@paulrouget paulrouget changed the title [multi window] Constellation should be able to handle multiple top frame [multi window] Constellation should be able to handle multiple root frame Oct 31, 2016
@paulrouget
Copy link
Contributor Author

@asajeffrey I've put together an example of how tab management could work: https://github.com/paulrouget/servo/tree/tabs_hooks (I ended up using the glutin port to make things easier). This branch includes the hooks for all the keyboards shortcuts, and the tabs are printed in the terminal. This is not functional as there is only one top level root frame, but I hope the comments will help you understand the kind of architecture I have in mind.

I recommend starting to look at the main loop.

Important: this branch is just for experimenting. I'm not planning to land this code.

Note: when I refer to "tab", "top level browsing context", "browser", "root frame" or "top level frame", I mean the same thing.

So we need a "browser factory". A singleton. Equivalent of the constellation. Called BrowserManager in my branch.

This browser factory creates browsers on demand: BrowserManager::new_browser(LoadData) -> FrameId. Which will create a new top level frame. See here.

To reference a browser, we can use FrameId, but I'm not super happy about that, as a FrameId is not necessarily a top level frame. Do we want to introduce a new type?

When the embedder send events to Servo, when relevant, we will need to also tell which FrameId we want to target. See for example how the embedder ask Servo to reload a tab.

When Servo send back information to the embedder, the embedder will expect a FrameId argument. See for example how the url of a tab is updated here and here.

Finally, we will need a way to "switch tabs". Which requires 2 things: 1) let WR know which frame tree to render 2) update the DOM visibility API (similar work has been done here)

@paulrouget
Copy link
Contributor Author

Here is a more precise description of my expectations:

  1. constellation.rs supports multiple top level frames
  2. component/servo/lib.rs exposes an API (two methods) to a) create new top level frames b) tell WR which one to display and update the DOM visibility API
  3. compositor.rs call the WindowMethods with the relevant FrameId as an argument
  4. all the relevant WindowEvent in windowing.rs include a FrameId argument

With all of that, we will be able to build tabs.

@asajeffrey - let me know if you have any questions

@asajeffrey
Copy link
Member

There are some other places that a top-level frame id would be a useful concept. In servo as it currently stands, there's a 1-1 correspondence between top-level browsing context and unit of related browsing contexts (because we don't treat the opener as reachable). Do we want to continue this? Make it up to the embedder?

As a naming point, I'm not sure about using "browser" as synonymous with "top-level browsing context", most people would use "browser" to mean the whole user agent, which may present multiple tabs.

In the API, I'm not sure about servo returning a "can continue" boolean. Shouldn't exiting the app be the embedder's decision, not servo's? (Oh joy, shutdown again.)

@paulrouget
Copy link
Contributor Author

In servo as it currently stands, there's a 1-1 correspondence between top-level browsing context and unit of related browsing contexts (because we don't treat the opener as reachable). Do we want to continue this? Make it up to the embedder?

You know better. Eventually, from the embedder, we want to be able to create a new "tab" and tell Servo that its opener is this other "tab". For example, when the the first tab use window.open. Is the new frame still a top level frame?

As a naming point, I'm not sure about using "browser" as synonymous with "top-level browsing context", most people would use "browser" to mean the whole user agent, which may present multiple tabs.

Ok!

In the API, I'm not sure about servo returning a "can continue" boolean. Shouldn't exiting the app be the embedder's decision, not servo's? (Oh joy, shutdown again.)

You're right. But this is out of the scope of this work for now I think.

@paulrouget
Copy link
Contributor Author

So how would you call a "tab"? Is it always a unique top level browsing context (see my question in the previous comment)? If so, we can call it like that. Or should we just call that a "TopFrame"?

@paulrouget
Copy link
Contributor Author

@asajeffrey let me know if I can make my requirements clearer.

@asajeffrey
Copy link
Member

OK, worklets PR submitted, time to think about this again!

@paulrouget
Copy link
Contributor Author

@asajeffrey what's going to be the type of a "top frame"? WindowProxy? TopWindowProxy? BrowsingContext? TopBrowserContext? TopFrameId?

@asajeffrey
Copy link
Member

TopLevelBrowsingContextId.

@asajeffrey
Copy link
Member

Top-level browsing context ids are #16916.

@asajeffrey
Copy link
Member

Doing an audit of the places we're using the root browsing context, most of them it's pretty obvious how to remove them. There are a few annoying cases though:

  • Generating crash reports for non-pipeline panics. In general we probably want the embedding application to handle logging and crash reporting, not servo.
  • Where should key events be routed if there's no focused pipeline?
  • There are some places where the embedding API should tell us which top-level browsing context to target, for example handling reload events.

@jdm
Copy link
Member

jdm commented May 18, 2017

I'm not clear how we would end up with no focused pipeline.

@asajeffrey
Copy link
Member

Key events that arrive before the Focus message? It's probably OK to just discard such key events.

@paulrouget
Copy link
Contributor Author

Generating crash reports for non-pipeline panics. In general we probably want the embedding application to handle logging and crash reporting, not servo.

Agreed.

Where should key events be routed if there's no focused pipeline?

Within any top level browsing context, there is one focused pipeline.
As for which top level browsing context is focused, it's a notion that is up to the embedder. The embedder decides to which top level browsing context to send the key events to.

There are some places where the embedding API should tell us which top-level browsing context to target, for example handling reload events.

Wouldn't that be for most of the places?

I would expect any call to Servo to come with a top level browsing context id.

@asajeffrey
Copy link
Member

Another tricky case is webdriver, which assumes there's a root browsing context. Looking at the spec, I can't really tell what's meant to happen when a new session is created. https://www.w3.org/TR/webdriver/#new-session doesn't say how to initialize the top-level browsing context of the session. @jgraham?

bors-servo pushed a commit that referenced this issue Jun 7, 2017
…ext, r=cbrewster

Removed root browsing context from constellation

<!-- Please describe your changes on the following line: -->

Removed the special root browsing context from the constellation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13994
- [X] These changes do not require tests because this isn't visible from user code

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17077)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 7, 2017
…ext, r=cbrewster

Removed root browsing context from constellation

<!-- Please describe your changes on the following line: -->

Removed the special root browsing context from the constellation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13994
- [X] These changes do not require tests because this isn't visible from user code

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17077)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 8, 2017
…ext, r=cbrewster

Removed root browsing context from constellation

<!-- Please describe your changes on the following line: -->

Removed the special root browsing context from the constellation.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13994
- [X] These changes do not require tests because this isn't visible from user code

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17077)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants