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

Update Constellation to track each browser's focused browsing context #22051

Merged
merged 3 commits into from Nov 5, 2018

Conversation

Projects
None yet
6 participants
@mandreyel
Contributor

mandreyel commented Oct 30, 2018

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.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17401 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Oct 30, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: components/constellation/constellation.rs
@mandreyel

This comment has been minimized.

Contributor

mandreyel commented Oct 30, 2018

@jdm jdm assigned cbrewster and unassigned avadacatavra Oct 30, 2018

@cbrewster

Looking good, I would suggest us storing a map of TopLevelBrowsingContextId -> BrowsingContextId for tracking the active browsing context for each "tab". And then just use the active_browser_id as the key for this map when determining the focused browsing context for key events. This alleviates the need for the extra bookkeeping of focused_browsing_context_id.

Also I agree about merging these HashMaps for TopLevelBrowsingContextId. I think that would be a good followup.

/// focused browsing context so that we can restore the
/// `focused_browsing_context_id` to point to the active browser's focused
/// browsing context.
browser_ids: HashMap<TopLevelBrowsingContextId, BrowsingContextId>,

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

It would be good to change the name here to something more along the lines of focused_browsing_contexts

/// pipeline is the current entry of the focused browsing context. This
/// field is always updated to be the focused browsing context of the
/// current active browser.
focused_browsing_context_id: Option<BrowsingContextId>,

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

This could store the active/focused top level browsing context: TopLevelBrowsingContextId. With focused_browsing_contexts you can use this as the key and retrieve the focused BrowsingContextId for key events to go to. (This will all need to be reworked with multi-window support, but for now this will do).

Possibly we could just reuse active_browser_id as it seems like it would be the equivalent.

This comment has been minimized.

@mandreyel

mandreyel Nov 2, 2018

Contributor

I was thinking about this too, but I left it in because I wasn't sure of the additional overhead that always looking up the related browsing context in the hash map would introduce in handle_key_events (since from what I understand this might be a hot path). That is, I viewed focused_browsing_context as a sort of cache. Do you think this is valid or just premature optimization?

This comment has been minimized.

@mandreyel

mandreyel Nov 2, 2018

Contributor

But I agree that it makes the logic more confusing, so it's perhaps worth removing either way.

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

Right, doing an extra lookup would add a little extra overhead here. I don't imagine it will be noticeable though (if I am wrong, we can switch back to this caching method).

// Only update `focused_browsing_context_id` if the browsing context for
// which the focus was requested is in the current active browser.
if Some(top_level_browsing_context_id) == self.active_browser_id {

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

With the adjustments mentioned above we can just reduce this to always doing:

self.focused_browsing_contexts.insert(top_level_browsing_context_id, browsing_context_id);
@@ -1644,6 +1649,9 @@ where
let is_private = false;
let is_visible = true;
// Register this new top-level browsing context id as a browser and set
// its focused browsing context to be itself.
self.browser_ids.insert(top_level_browsing_context_id, browsing_context_id);
if self.focused_browsing_context_id.is_none() {

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

This can be removed.

This comment has been minimized.

@mandreyel

mandreyel Nov 2, 2018

Contributor

Because of the suggestion above to always do an insert when we get a focus message? I think it's cleaner if we insert only during browser creation and subsequently only do updates in handle_focus_msg, i.e.

self.browsers.entry(top_level_browsing_context_id)
    .focused_browsing_context_id = browsing_context_id;

or something similar.

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

Yes this sounds right, I was referring to setting focused_browsing_context_id

@@ -3893,9 +3923,20 @@ where
/// Send the current frame tree to compositor
fn send_frame_tree(&mut self, top_level_browsing_context_id: TopLevelBrowsingContextId) {
self.active_browser_id = Some(top_level_browsing_context_id);

This comment has been minimized.

@cbrewster

cbrewster Nov 2, 2018

Member

I believe with the above changes, this should be reverted.

@mandreyel

This comment has been minimized.

Contributor

mandreyel commented Nov 2, 2018

@cbrewster Thanks for the review, I'll implement the changes shortly.
Btw, auxiliaries need to have an entry in the hash map for browser related bookkeeping, correct? Since afaik, auxiliaries are used for new browsers created with window.open, but please correct me if this is wrong. I'm asking because I forgot this from my previous commit.

@cbrewster

This comment has been minimized.

Member

cbrewster commented Nov 2, 2018

@mandreyel Yes auxiliary BCs will need to be included in the bookkeeping as they are always top-level browsing contexts: https://html.spec.whatwg.org/multipage/#auxiliary-browsing-context

@mandreyel mandreyel force-pushed the mandreyel:track-focused-bc-of-each-tlbc branch from 1da4792 to 147f565 Nov 2, 2018

@mandreyel mandreyel force-pushed the mandreyel:track-focused-bc-of-each-tlbc branch from 147f565 to 0ed9b68 Nov 2, 2018

@cbrewster

This comment has been minimized.

Member

cbrewster commented Nov 4, 2018

LGTM, thank you!
@bors-servo r+

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 4, 2018

📋 Looks like this PR is still in progress, ignoring approval

@cbrewster

This comment has been minimized.

Member

cbrewster commented Nov 4, 2018

Oops, please remove the WIP in the title if you are ready for this to be merged

@mandreyel mandreyel changed the title from [WIP] Update Constellation to track each browser's focused browsing context to Update Constellation to track each browser's focused browsing context Nov 5, 2018

@mandreyel

This comment has been minimized.

Contributor

mandreyel commented Nov 5, 2018

@cbrewster Pardon, wasn't near a terminal yesterday. I've removed it now!

@jdm

This comment has been minimized.

Member

jdm commented Nov 5, 2018

@bors-servo r=cbrewster

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 5, 2018

📌 Commit 0ed9b68 has been approved by cbrewster

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 5, 2018

⌛️ Testing commit 0ed9b68 with merge ddc8509...

bors-servo added a commit that referenced this pull request Nov 5, 2018

Auto merge of #22051 - mandreyel:track-focused-bc-of-each-tlbc, r=cbr…
…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 -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 5, 2018

@bors-servo bors-servo merged commit 0ed9b68 into servo:master Nov 5, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment