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.

Sign up for GitHub

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

Removed root browsing context from constellation #17077

Merged

bors-servo merged 1 commit into servo:master from asajeffrey:constellation-no-root-browsing-context

Jun 8, 2017

Member

asajeffrey commented May 29, 2017 •

edited by nox

Removed the special root browsing context from the constellation.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix [multi window] Constellation should be able to handle multiple root frame #13994
These changes do not require tests because this isn't visible from user code

This change is

asajeffrey added the A-constellation label

highfive assigned Manishearth

highfive commented May 29, 2017

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/document.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs
@KiChjang: components/script/dom/document.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs

highfive commented May 29, 2017

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

highfive added the S-awaiting-review label

Member Author

asajeffrey commented May 29, 2017

r? @paulrouget
cc @cbrewster

highfive assigned paulrouget and unassigned Manishearth

Contributor

paulrouget commented May 30, 2017

I'm looking at this. It will take me a day or two. I'd like to see how close we are to support tabs with this.

Member Author

asajeffrey commented May 30, 2017

Thanks! Let me know if there's anything else needed for a minimal one-window-but-multiple-tabs browser.

Contributor

paulrouget commented May 31, 2017

So now, if I'm not mistaken, the compositor has a pipeline tree while the constellation has a pipeline forest. That means the compositor will get plenty of messages from or for pipeline it doesn't know about. I think it brings us back to #15934 (Create a dedicated channel for constellation -> embedder communication).

Can we land support for multiple browsing contexts without fixing #15934 first?

paulrouget mentioned this pull request

Create a dedicated channel for constellation -> embedder communication #15934

Closed

Contributor

paulrouget commented Jun 5, 2017

@asajeffrey I managed to get tabs working. Thank you! The only issue is what I mentioned in my previous comment, but I will take care of that later.

r=me, but you need a proper review though (I'm not a reviewer).

Once this has landed, this is what I will work on:

by-pass the compositor for non-compositor message. See my plan here: Create a dedicated channel for constellation -> embedder communication #15934 (comment)
expose TopLevelBrowsingContext to embedder
implement API to create and switch browsing context
add a browsing context parameter to the WindowEvents (when it makes sense)
update the visibility API status based on the active compositor pipeline tree

I got most of this working here: https://github.com/paulrouget/servo/tree/tabs (just prototyping, code is horrible).

Contributor

paulrouget commented Jun 5, 2017

@asajeffrey don't you think we should clear IOCompositor::pipeline_details when the frame tree is set?

Member Author

asajeffrey commented Jun 5, 2017

highfive assigned cbrewster and unassigned paulrouget

cbrewster suggested changes

View reviewed changes

Contributor

cbrewster left a comment

Looks good other than a couple nits/questions!

components/constellation/constellation.rs Outdated

-                      self.close_browsing_context(root_browsing_context_id, ExitPipelineMode::Normal);
+                      // Close the top-level browsing contexts
+                      let top_level_browsing_context_ids: Vec<TopLevelBrowsingContextId> = self.browsing_contexts.values()
+                          .filter(|browsing_context| browsing_context.is_top_level())

Contributor

cbrewster Jun 5, 2017

nit: .filter(BrowsingContext::is_top_level)

Member Author

asajeffrey Jun 6, 2017

We wish. It's got the wrong type (&BrowsingContext vs BrowsingContext).

Contributor

cbrewster Jun 7, 2017

Tis a shame

components/constellation/constellation.rs Outdated

+                          .collect();
+                      for top_level_browsing_context_id in top_level_browsing_context_ids {
+                          debug!("Removing top-level browsing context {}.", top_level_browsing_context_id);
+                          let browsing_context_id = BrowsingContextId::from(top_level_browsing_context_id);

Contributor

cbrewster Jun 5, 2017

If we know the browsing contexts are top level, can't we just get their id rather than the top_level_id to avoid this conversion?

Member Author

asajeffrey Jun 6, 2017

Fixed.

components/constellation/constellation.rs

@@ @@ -1495,6 +1490,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF> @@
                       };
                       // Create the new pipeline, attached to the parent and push to pending changes
+                      self.handle_load_start_msg(load_info.info.new_pipeline_id);

Contributor

cbrewster Jun 5, 2017

If we need to be calling self.handle_load_start_msg before we push a pending change, it might make sense to add a new method for adding pending changes that way we don't forget to call self.handle_load_start_msg.

This could be saved for another PR since it isn't directly related to these changes. Possible E-Easy.

Member Author

asajeffrey Jun 6, 2017

Yes, there's quite a bit of boilerplate we could DRY. This should be a separate PR though. Not sure anything in the constellation is E-Easy :)

components/constellation/constellation.rs

                               if let Err(e) = result {
                                   self.handle_send_error(parent_pipeline_id, e);
                               }
-                              Some(source_id)
+                              None

Contributor

cbrewster Jun 5, 2017

I'm not very familiar with webdriver, but its the only user of the pipeline id returned from this method. Does this change break anything there?

Member Author

asajeffrey Jun 6, 2017

Er, I don't think so. webdriver isn't very tested though :/

components/constellation/constellation.rs Outdated

+                              return None;
+                          }
+                      };
+                      let (top_level_id, window_size, timestamp) = match self.browsing_contexts.get(&browsing_context_id) {

Contributor

cbrewster Jun 5, 2017

These values are only used in the None branch of the match, can we move them there?

Member Author

asajeffrey Jun 6, 2017

Fixed.

components/constellation/constellation.rs Outdated

-                      let (evicted_id, new_context, navigated, location_changed) = if let Some(instant) = change.replace_instant {
+                      let url = change.load_data.url.clone();
+                      let (evicted_id, new_context, navigated) = if let Some(instant) = change.replace_instant {

Contributor

cbrewster Jun 5, 2017

Yay one less item in this tuple!

Member Author

asajeffrey Jun 6, 2017

Yay!

cbrewster added S-awaiting-answer S-needs-code-changes and removed S-awaiting-review labels

highfive added S-awaiting-review and removed S-needs-code-changes labels

asajeffrey commented

View reviewed changes

Member Author

asajeffrey left a comment

Pushed changes.

components/constellation/constellation.rs Outdated

-                      self.close_browsing_context(root_browsing_context_id, ExitPipelineMode::Normal);
+                      // Close the top-level browsing contexts
+                      let top_level_browsing_context_ids: Vec<TopLevelBrowsingContextId> = self.browsing_contexts.values()
+                          .filter(|browsing_context| browsing_context.is_top_level())

Member Author

asajeffrey Jun 6, 2017

We wish. It's got the wrong type (&BrowsingContext vs BrowsingContext).

components/constellation/constellation.rs Outdated

+                          .collect();
+                      for top_level_browsing_context_id in top_level_browsing_context_ids {
+                          debug!("Removing top-level browsing context {}.", top_level_browsing_context_id);
+                          let browsing_context_id = BrowsingContextId::from(top_level_browsing_context_id);

Member Author

asajeffrey Jun 6, 2017

Fixed.

components/constellation/constellation.rs

@@ @@ -1495,6 +1490,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF> @@
                       };
                       // Create the new pipeline, attached to the parent and push to pending changes
+                      self.handle_load_start_msg(load_info.info.new_pipeline_id);

Member Author

asajeffrey Jun 6, 2017

Yes, there's quite a bit of boilerplate we could DRY. This should be a separate PR though. Not sure anything in the constellation is E-Easy :)

components/constellation/constellation.rs Outdated

+                              return None;
+                          }
+                      };
+                      let (top_level_id, window_size, timestamp) = match self.browsing_contexts.get(&browsing_context_id) {

Member Author

asajeffrey Jun 6, 2017

Fixed.

components/constellation/constellation.rs

                               if let Err(e) = result {
                                   self.handle_send_error(parent_pipeline_id, e);
                               }
-                              Some(source_id)
+                              None

Member Author

asajeffrey Jun 6, 2017

Er, I don't think so. webdriver isn't very tested though :/

components/constellation/constellation.rs Outdated

-                      let (evicted_id, new_context, navigated, location_changed) = if let Some(instant) = change.replace_instant {
+                      let url = change.load_data.url.clone();
+                      let (evicted_id, new_context, navigated) = if let Some(instant) = change.replace_instant {

Member Author

asajeffrey Jun 6, 2017

Yay!

asajeffrey removed the S-awaiting-answer label

cbrewster approved these changes

View reviewed changes

Contributor

cbrewster left a comment

LGTM. squash and r=me

cbrewster added S-needs-squash and removed S-awaiting-review labels

asajeffrey force-pushed the constellation-no-root-browsing-context branch from 8047f6a to 293c157 Compare

June 7, 2017 15:03

highfive added the S-awaiting-review label

highfive added S-awaiting-review and removed S-tests-failed labels

asajeffrey force-pushed the constellation-no-root-browsing-context branch from 11ad7cf to 86c09ac Compare

June 7, 2017 20:24

Member Author

asajeffrey commented Jun 7, 2017

IRC conversation with @cbrewster: http://logs.glob.uno/?c=mozilla%23servo&s=7+Jun+2017&e=7+Jun+2017#c691847

@bors-servo r=cbrewster

Contributor

bors-servo commented Jun 7, 2017

📌 Commit 86c09ac has been approved by cbrewster

highfive added S-awaiting-merge and removed S-awaiting-review labels

Contributor

bors-servo commented Jun 7, 2017

⌛ Testing commit 86c09ac with merge abb6ce3...

bors-servo pushed a commit that referenced this pull request


          Auto merge of #17077 - asajeffrey:constellation-no-root-browsing-cont…

abb6ce3

…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 -->

Contributor

bors-servo commented Jun 7, 2017

💔 Test failed - linux-dev

highfive added S-tests-failed and removed S-awaiting-merge labels

Contributor

cbrewster commented Jun 7, 2017

Checking files for tidiness...

./components/constellation/constellation.rs:2891: Line is longer than 120 characters

cbrewster added S-fails-tidy and removed S-tests-failed labels

Member Author

asajeffrey commented Jun 7, 2017 via email

Oh for pity's sake...


          Removed root browsing context from constellation.

2fd925b

asajeffrey force-pushed the constellation-no-root-browsing-context branch from 86c09ac to 2fd925b Compare

June 8, 2017 03:59

highfive added the S-awaiting-review label

Member Author

asajeffrey commented Jun 8, 2017

@bors-servo r=cbrewster

Contributor

bors-servo commented Jun 8, 2017

📌 Commit 2fd925b has been approved by cbrewster

highfive added S-awaiting-merge and removed S-awaiting-review labels

Contributor

bors-servo commented Jun 8, 2017

⌛ Testing commit 2fd925b with merge caa8343...

bors-servo pushed a commit that referenced this pull request


          Auto merge of #17077 - asajeffrey:constellation-no-root-browsing-cont…

caa8343

…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 -->

Contributor

bors-servo commented Jun 8, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: cbrewster
Pushing caa8343 to master...

bors-servo merged commit 2fd925b into servo:master

highfive removed the S-awaiting-merge label

paulrouget mentioned this pull request

Tabs support: add methods to create additional top level browsing context #17226

Closed

cbrewster mentioned this pull request

bhtml no longer showing navigation/url bar #17246

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment