Added a TopLevelBrowsingContextId type. #16916

asajeffrey · 2017-05-17T20:59:47Z

Added a TopLevelBrowsingContextId type, which is a subtype of BrowsingContextId that only refers to top-level browsing contexts.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes do not require tests because refactoring

This change is

highfive · 2017-05-17T20:59:51Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/dom/windowproxy.rs, components/script/dom/node.rs, components/script_traits/script_msg.rs and 6 more
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script/dom/windowproxy.rs, components/script/dom/node.rs, components/script_traits/script_msg.rs and 6 more

highfive · 2017-05-17T20:59:53Z

Warning

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

asajeffrey · 2017-05-17T21:00:54Z

This is the next step in the effort to allow more than one root browsing context. cc @cbrewster @paulrouget

asajeffrey · 2017-05-17T21:01:31Z

r? @cbrewster

cbrewster

Looking good! +1 for more type safety

cbrewster · 2017-05-18T16:51:59Z

components/constellation/constellation.rs

@@ -700,8 +685,8 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
    /// Get an iterator for browsing contexts. Specify self.root_browsing context_id to
    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


Update to state that this can only iterate a sub-tree

cbrewster · 2017-05-18T16:52:46Z

components/constellation/constellation.rs

@@ -710,11 +695,20 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    /// Get an iterator for browsing contexts. Specify self.root_browsing context_id to
+    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


This can only iterate an entire tree.

cbrewster · 2017-05-18T16:52:52Z

components/constellation/constellation.rs

+    {
+        self.fully_active_descendant_browsing_contexts_iter(BrowsingContextId::from(top_level_browsing_context_id))
+    }
+
    /// Get an iterator for browsing contexts. Specify self.root_browsing_context_id to
    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


Same as above

cbrewster · 2017-05-18T16:53:01Z

components/constellation/constellation.rs

@@ -723,28 +717,37 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    /// Get an iterator for browsing contexts. Specify self.root_browsing_context_id to
+    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


Same as above

cbrewster · 2017-05-18T17:02:47Z

components/constellation/constellation.rs

@@ -1862,17 +1892,20 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        // and pass the event to that script thread.
        // If the pipeline lookup fails, it is because we have torn down the pipeline,
        // so it is reasonable to silently ignore the event.
-        let browsing_context_id = self.pipelines.get(&pipeline_id).map(|pipeline| pipeline.browsing_context_id);
+        let top_level_browsing_context_id = self.pipelines.get(&pipeline_id)


Nice catch!

Yes, this is one of those odd cases that the extra typechecking helped with. Yay types!

cbrewster · 2017-05-18T17:22:58Z

components/layout_thread/lib.rs

@@ -244,7 +244,6 @@ impl LayoutThreadFactory for LayoutThread {

    /// Spawns a new layout thread.
    fn create(id: PipelineId,
-              top_level_browsing_context_id: Option<BrowsingContextId>,


I remembered why we wanted this, which is for crash reporting, so that if a layout panic happens we know which top-level browsing context to blame.

cbrewster · 2017-05-18T17:23:28Z

components/constellation/pipeline.rs

@@ -104,7 +104,7 @@ pub struct InitialPipelineState {
    pub browsing_context_id: BrowsingContextId,

    /// The ID of the top-level browsing context that contains this Pipeline.
-    pub top_level_browsing_context_id: BrowsingContextId,
+    pub top_level_browsing_context_id: TopLevelBrowsingContextId,


Is this still needed since the layout thread no longer needs it?

asajeffrey

Changes made.

asajeffrey · 2017-05-19T01:47:00Z

components/constellation/constellation.rs

@@ -710,11 +695,20 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    /// Get an iterator for browsing contexts. Specify self.root_browsing context_id to
+    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


asajeffrey · 2017-05-19T01:47:08Z

components/constellation/constellation.rs

+    {
+        self.fully_active_descendant_browsing_contexts_iter(BrowsingContextId::from(top_level_browsing_context_id))
+    }
+
    /// Get an iterator for browsing contexts. Specify self.root_browsing_context_id to
    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


asajeffrey · 2017-05-19T01:47:15Z

components/constellation/constellation.rs

@@ -723,28 +717,37 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        }
    }

+    /// Get an iterator for browsing contexts. Specify self.root_browsing_context_id to
+    /// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.


asajeffrey · 2017-05-19T01:48:06Z

components/constellation/constellation.rs

@@ -1862,17 +1892,20 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        // and pass the event to that script thread.
        // If the pipeline lookup fails, it is because we have torn down the pipeline,
        // so it is reasonable to silently ignore the event.
-        let browsing_context_id = self.pipelines.get(&pipeline_id).map(|pipeline| pipeline.browsing_context_id);
+        let top_level_browsing_context_id = self.pipelines.get(&pipeline_id)


Yes, this is one of those odd cases that the extra typechecking helped with. Yay types!

asajeffrey · 2017-05-19T01:54:24Z

components/layout_thread/lib.rs

@@ -244,7 +244,6 @@ impl LayoutThreadFactory for LayoutThread {

    /// Spawns a new layout thread.
    fn create(id: PipelineId,
-              top_level_browsing_context_id: Option<BrowsingContextId>,


I remembered why we wanted this, which is for crash reporting, so that if a layout panic happens we know which top-level browsing context to blame.

cbrewster · 2017-05-19T17:17:03Z

components/constellation/constellation.rs

@@ -1783,7 +1803,8 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        // Send to the explicitly focused pipeline (if it exists), or the root
        // browsing context's current pipeline. If neither exist, fall back to sending to
        // the compositor below.
-        let root_pipeline_id = self.browsing_contexts.get(&self.root_browsing_context_id)
+        let root_browsing_context_id = BrowsingContextId::from(self.root_browsing_context_id);
+        let root_pipeline_id = self.browsing_contexts.get(&root_browsing_context_id)


I wonder if we could replace these occurrences with:

self.browsing_contexts.get(&self.root_browsing_context_id.into())

since From<T> for U implies Into<U> for T

I prefer T::from(x) over x.into() just because I find it easier to read. x.into() is rather mysterious, it converts from one unnamed type to another unnamed type. YMMV.

cbrewster · 2017-05-19T17:38:12Z

You can decide whether to switch to .into() or keep using ::from. Other than that this looks good, so you can squash and r=me

asajeffrey · 2017-05-19T17:47:24Z

Squashed. @bors-servo r=cbrewster

bors-servo · 2017-05-19T17:47:24Z

📌 Commit 273c474 has been approved by cbrewster

bors-servo · 2017-05-20T02:50:17Z

⌛ Testing commit 273c474 with merge 09d18dd...

…ntexts, r=cbrewster Added a TopLevelBrowsingContextId type.  Added a `TopLevelBrowsingContextId` type, which is a subtype of `BrowsingContextId` that only refers to top-level browsing contexts. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16916)

bors-servo · 2017-05-20T03:29:41Z

💔 Test failed - mac-rel-wpt1

cbrewster · 2017-05-20T17:03:19Z

  ▶ Unexpected subtest result in /html/semantics/embedded-content/the-iframe-element/iframe-synchronously-discard.html:
  │ FAIL [expected PASS] IFrame discards are processed synchronously
  │   → assert_equals: grandchild should be discarded expected null but got object "[object Window]"
  │ 
  │ child.onload</grandchild.onload<@http://web-platform.test:8000/html/semantics/embedded-content/the-iframe-element/iframe-synchronously-discard.html:24:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

asajeffrey · 2017-05-22T13:51:13Z

Well the good news is that I can replicate this locally. Not entirely sure how I broke synchronous iframe discarding, off to do some digging...

asajeffrey · 2017-05-22T14:26:22Z

Figured it out...

$ git diff
diff --git a/components/script/dom/htmliframeelement.rs b/components/script/dom/htmliframeelement.rs
index 1c00f3e..0075521 100644
--- a/components/script/dom/htmliframeelement.rs
+++ b/components/script/dom/htmliframeelement.rs
@@ -806,9 +806,11 @@ impl VirtualMethods for HTMLIFrameElement {
         // when the `PipelineExit` message arrives.
         for exited_pipeline_id in exited_pipeline_ids {
             if let Some(exited_document) = ScriptThread::find_document(exited_pipeline_id) {
+                debug!("Discarding browsing context for pipeline {}", exited_pipeline_id);
                 exited_document.window().window_proxy().discard_browsing_context();
                 for exited_iframe in exited_document.iter_iframes() {
-                    exited_iframe.pipeline_id.set(None);
+                    debug!("Discarding nested browsing context");
+                    exited_iframe.destroy_nested_browsing_context();
                 }
             }
         }

When we were synchronously discarding a browsing context, we were calling exited_iframe.pipeline_id.set(None);, which used to do the job, but now should be exited_iframe.destroy_nested_browsing_context();.

asajeffrey · 2017-05-22T14:27:37Z

@bors-servo r=cbrewster

bors-servo · 2017-05-22T14:27:38Z

📌 Commit 4257736 has been approved by cbrewster

bors-servo · 2017-05-22T20:13:43Z

⌛ Testing commit 4257736 with merge a0d6d68...

…ntexts, r=cbrewster Added a TopLevelBrowsingContextId type.  Added a `TopLevelBrowsingContextId` type, which is a subtype of `BrowsingContextId` that only refers to top-level browsing contexts. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16916)

bors-servo · 2017-05-22T21:46:26Z

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

highfive assigned pcwalton May 17, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 17, 2017

asajeffrey added the A-constellation Involves the constellation label May 17, 2017

highfive assigned cbrewster and unassigned pcwalton May 17, 2017

asajeffrey mentioned this pull request May 18, 2017

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

Closed

cbrewster suggested changes May 18, 2017

View reviewed changes

cbrewster added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 18, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 19, 2017

asajeffrey commented May 19, 2017

View reviewed changes

cbrewster reviewed May 19, 2017

View reviewed changes

asajeffrey force-pushed the constellation-top-level-browsing-contexts branch from 53174cb to 273c474 Compare May 19, 2017 17:45

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 19, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 20, 2017

Added a TopLevelBrowsingContextId type.

4257736

asajeffrey force-pushed the constellation-top-level-browsing-contexts branch from 273c474 to 4257736 Compare May 22, 2017 14:27

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels May 22, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 22, 2017

asajeffrey mentioned this pull request May 22, 2017

Webdriver browsing contexts not pipelines #16997

Merged

3 tasks

bors-servo merged commit 4257736 into servo:master May 22, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 22, 2017

bors-servo mentioned this pull request May 22, 2017

Replace initial about:blank on navigation #16848

Closed

5 tasks

		@@ -700,8 +685,8 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
		/// Get an iterator for browsing contexts. Specify self.root_browsing context_id to
		/// iterate the entire tree, or a specific browsing context id to iterate only that sub-tree.

Added a TopLevelBrowsingContextId type. #16916

Added a TopLevelBrowsingContextId type. #16916

Conversation

asajeffrey commented May 17, 2017 • edited by larsbergstrom

highfive commented May 17, 2017

highfive commented May 17, 2017

asajeffrey commented May 17, 2017

asajeffrey commented May 17, 2017

cbrewster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrewster commented May 19, 2017

asajeffrey commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 20, 2017

bors-servo commented May 20, 2017

cbrewster commented May 20, 2017

asajeffrey commented May 22, 2017

asajeffrey commented May 22, 2017

asajeffrey commented May 22, 2017

bors-servo commented May 22, 2017

bors-servo commented May 22, 2017

bors-servo commented May 22, 2017

asajeffrey commented May 17, 2017 •

edited by larsbergstrom