Skip to content

/ servo

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

Jump to bottom

Constellation: there should be one focused pipeline by top level browsing context id #17401

Closed
paulrouget opened this issue Jun 19, 2017 · 9 comments
Closed

Constellation: there should be one focused pipeline by top level browsing context id #17401

paulrouget opened this issue Jun 19, 2017 · 9 comments
Labels
C-assigned

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 19, 2017

At the moment, there is one global focused pipeline.

The actual focused pipeline being the focused pipeline under the last top level browsing context sent to the compositor.
@paulrouget paulrouget mentioned this issue Jul 15, 2017
Closed
@paulrouget paulrouget mentioned this issue Aug 8, 2017
Merged
@mandreyel
Copy link
Contributor

@mandreyel mandreyel commented Oct 6, 2018

How has #21713 affected this issue? Can you please give a few more details?
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Oct 8, 2018 •
edited

I'm not sure. I haven't looked at this for a long time, so this might not be relevant.

Looking at the PR, that might just work as long as focused_browsing_context_id is a top level browsing context.

The idea is this: there are multiple top level browsing contexts and only one is "active" (see active_browser_id). Let's say top level context A is active and a descendant pipeline a is focused. When switching to top level context B (by calling send_frame_tree) a descendant pipeline of B is focused. Switching back to A, we need to make sure pipeline a is the one getting the focus.
@mandreyel
Copy link
Contributor

@mandreyel mandreyel commented Oct 18, 2018

@paulrouget I'm sorry, I completely missed that you responded!

Hmm, I think I may be lacking some context to fully understand the issue. Since we're now tracking focused browsing contexts (introduced in the PR), which means their current pipeline is also the focused pipeline, is your proposed invariant not already ensured?
@cbrewster
Copy link
Member

@cbrewster cbrewster commented Oct 18, 2018

The issue here is tracking the focused browsing context per top-level browsing context. In other words, we need to track the active browsing context per browser tab. This would mean there should be multiple focused browsing contexts at once (one per top-level browsing context/browser tab).
@mandreyel
Copy link
Contributor

@mandreyel mandreyel commented Oct 19, 2018

Ah, I wasn't taking tabs into account (though I had a suspicion), since afaik it's not yet supported by Servo. So am I right that this will only become relevant when #21363 progresses?
@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Oct 19, 2018 •
edited

@mandreyel Tabs are internally supported. But there's no UI for it. Every time the embedder sends a NewBrowser message, a new top level browsing context is created. We switch between "tabs" by sending the right frame tree to WR. This happens with the SelectBrowser message.
@mandreyel
Copy link
Contributor

@mandreyel mandreyel commented Oct 19, 2018

Ah, that makes sense. Thanks for clearing it up. This is indeed an unsolved issue, I'll look into it later.
@mandreyel
Copy link
Contributor

@mandreyel mandreyel commented Oct 26, 2018

@highfive assign me
@highfive highfive added the C-assigned label Oct 26, 2018
@highfive
Copy link

@highfive highfive commented Oct 26, 2018

Hey @mandreyel! Thanks for your interest in working on this issue. It's now assigned to you!
@mandreyel mandreyel mentioned this issue Oct 30, 2018
Merged
3 of 5 tasks complete
bors-servo added a commit that referenced this issue Nov 5, 2018
@bors-servo
Auto merge of #22051 - mandreyel:track-focused-bc-of-each-tlbc, r=cbr…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
ddc8509 
…ewster

Update Constellation to track each browser's focused browsing context

Since there may be multiple browsers (top-level browsing contexts), each has a focused browsing context. However, we were not keeping track of each browser's focused browsing context, so e.g when switching tabs the `Constellation::focused_browsing_context_id` would not be set to the switched-to browser's focused browsing context.

This PR introduces a `browser_ids` `HashMap` in constellation, that maps each of the top-level browsing context's ids to their currently focused browsing context's id, so that when the active browser is changed with the `SelectBrowser` message, we can look up and restore the selected browser's focused browsing context.

This is a wip. For one, I'm not a fan of adding another hash map to constellation, and since there already is a hash map for keeping track of a browser's joint session history, we could introduce some `Browser` struct to hold data for a browser like its session history and focused browsing context (and possibly more later). But wanted to implement the bare-bones logic to first ensure correctness and will refactor later. Also, we may need new tests but I'm not sure.

---
<!-- 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 #17401 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/22051)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-assigned
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Update Constellation to track each browser's focused browsing context
4 participants
@paulrouget @highfive @cbrewster @mandreyel
You can’t perform that action at this time.