Implement dissimilar-origin window.parent and window.top #15799

asajeffrey · 2017-03-02T16:25:08Z

This PR implements window.parent and window.top for dissimilar-origin windows.

This PR builds on #15536, only the last commit is part of this PR.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14996 and fix #11660.
These changes do not require tests because there's already a parentage test in mozilla/cross-origin-objects/cross-origin-objects.html.

This change is

highfive · 2017-03-02T16:25:20Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/location.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script/devtools.rs, components/script/dom/browsingcontext.rs, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/dom/webidls/Document.webidl, components/script/dom/dissimilaroriginwindow.rs, components/script/dom/window.rs, components/script/dom/document.rs, components/script/dom/webidls/Location.webidl, components/script/dom/htmliframeelement.rs
@KiChjang: components/script/dom/location.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/net_traits/pub_domains.rs, components/net_traits/pub_domains.rs, components/script/script_thread.rs, components/script/devtools.rs, components/script/dom/browsingcontext.rs, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/dom/webidls/Document.webidl, components/script/dom/dissimilaroriginwindow.rs, components/script/dom/window.rs, components/script/dom/document.rs, components/script/dom/webidls/Location.webidl, components/script/dom/htmliframeelement.rs

highfive · 2017-03-02T16:25:24Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.
These commits modify unsafe code. Please review it carefully!

asajeffrey · 2017-03-02T16:25:58Z

cc @jdm @Ms2ger

KiChjang · 2017-03-02T19:49:30Z

r? @Ms2ger

asajeffrey · 2017-03-02T20:18:04Z

Accidentally fixes #11660.

Ms2ger · 2017-03-03T09:55:52Z

Is there anyone else who could review this?

asajeffrey · 2017-03-03T14:39:32Z

The obvious candidate is @jdm, but he's away next week. @nox?

nox · 2017-03-03T14:40:26Z

I will as soon as #15536 lands and this gets rebased on top of master.

asajeffrey · 2017-03-03T14:42:55Z

@nox: thanks! If you wanted to take a look at #15536 too, that would be great, it's already gone through quite a bit of review by @jdm and @Ms2ger.

bors-servo · 2017-03-15T16:17:38Z

☔ The latest upstream changes (presumably #15536) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2017-03-15T19:16:08Z

@nox: ready when you are!

bors-servo · 2017-03-15T20:37:53Z

☔ The latest upstream changes (presumably #15679) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2017-03-15T21:25:55Z

Rebased.

bors-servo · 2017-03-16T23:20:15Z

☔ The latest upstream changes (presumably #15906) made this pull request unmergeable. Please resolve the merge conflicts.

nox

Some questions.

nox · 2017-03-17T08:16:48Z

components/script/script_thread.rs

+        result_receiver.recv().expect("Failed to get frame id from constellation.")
+    }
+
+    fn remote_browsing_context(&self,


What does this do?

Gets a browsing context for a pipeline in another script thread. I'll add a comment.

nox · 2017-03-17T08:16:48Z

components/script/script_thread.rs

+        Some(browsing_context)
+    }
+
+    fn local_browsing_context(&self,


What does this do?

Gets a browsing context for a pipeline in this script thread. I'll add a comment.

nox · 2017-03-17T08:16:48Z

components/script/script_thread.rs

+                              -> Root<BrowsingContext>
+    {
+        if let Some(browsing_context) = self.browsing_contexts.borrow().get(&frame_id) {
+            browsing_context.set_currently_active(&*window);


The browsing context already exists, but another window is currently active.

I don't get it. Why does getting a browsing context for a pipeline in this script thread should change the active window?

nox · 2017-03-17T08:16:48Z

components/constellation/constellation.rs

-
+            FromScriptMsg::GetFrameId(pipeline_id, sender) => {
+                let result = self.pipelines.get(&pipeline_id).map(|pipeline| pipeline.frame_id);
+                let _ = sender.send(result);


No warn! on send error?

nox · 2017-03-17T08:16:48Z

components/constellation/constellation.rs

+            }
+            FromScriptMsg::GetParentInfo(pipeline_id, sender) => {
+                let result = self.pipelines.get(&pipeline_id).and_then(|pipeline| pipeline.parent_info);
+                let _ = sender.send(result);


No warn! on send error?

asajeffrey

Thanks for the review!

asajeffrey · 2017-03-17T14:53:26Z

components/constellation/constellation.rs

-
+            FromScriptMsg::GetFrameId(pipeline_id, sender) => {
+                let result = self.pipelines.get(&pipeline_id).map(|pipeline| pipeline.frame_id);
+                let _ = sender.send(result);


asajeffrey · 2017-03-17T14:53:26Z

components/constellation/constellation.rs

+            }
+            FromScriptMsg::GetParentInfo(pipeline_id, sender) => {
+                let result = self.pipelines.get(&pipeline_id).and_then(|pipeline| pipeline.parent_info);
+                let _ = sender.send(result);


asajeffrey · 2017-03-17T14:53:26Z

components/script/script_thread.rs

+        result_receiver.recv().expect("Failed to get frame id from constellation.")
+    }
+
+    fn remote_browsing_context(&self,


Gets a browsing context for a pipeline in another script thread. I'll add a comment.

asajeffrey · 2017-03-17T14:53:26Z

components/script/script_thread.rs

+        Some(browsing_context)
+    }
+
+    fn local_browsing_context(&self,


Gets a browsing context for a pipeline in this script thread. I'll add a comment.

asajeffrey · 2017-03-17T14:53:26Z

components/script/script_thread.rs

+                              -> Root<BrowsingContext>
+    {
+        if let Some(browsing_context) = self.browsing_contexts.borrow().get(&frame_id) {
+            browsing_context.set_currently_active(&*window);


The browsing context already exists, but another window is currently active.

asajeffrey · 2017-03-17T15:19:55Z

Rebased.

nox · 2017-03-17T15:51:54Z

Squash it and r=me.


        Implement dissimilar-origin window.parent and window.top.

asajeffrey · 2017-03-17T15:54:04Z

Squashed. @bors-servo r=nox

bors-servo · 2017-03-17T15:54:05Z

📌 Commit 6bcb5f6 has been approved by nox

bors-servo · 2017-03-17T16:18:40Z

⌛ Testing commit 6bcb5f6 with merge 0a3b373...


        Auto merge of #15799 - asajeffrey:script-window-xorigin-parent, r=nox

Implement dissimilar-origin window.parent and window.top  This PR implements `window.parent` and `window.top` for dissimilar-origin windows. This PR builds on #15536, only the last commit is part of this PR. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14996 and fix #11660. - [X] These changes do not require tests because there's already a parentage test in `mozilla/cross-origin-objects/cross-origin-objects.html`.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15799)

bors-servo · 2017-03-17T16:59:02Z

☀️ 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: nox
Pushing 0a3b373 to master...

highfive assigned KiChjang Mar 2, 2017

highfive added the S-awaiting-review label Mar 2, 2017

asajeffrey added A-content/script S-blocked-on-external and removed S-awaiting-review labels Mar 2, 2017

highfive added the S-awaiting-review label Mar 2, 2017

asajeffrey added the S-needs-squash label Mar 2, 2017

highfive assigned Ms2ger and unassigned KiChjang Mar 2, 2017

asajeffrey mentioned this pull request Mar 2, 2017

Get window.frameElement to return null #11660

Closed

Ms2ger assigned nox and unassigned Ms2ger Mar 14, 2017

highfive added the S-needs-rebase label Mar 15, 2017

asajeffrey force-pushed the asajeffrey:script-window-xorigin-parent branch from 60de134 to 166eeea Mar 15, 2017

highfive added the S-needs-rebase label Mar 15, 2017

asajeffrey force-pushed the asajeffrey:script-window-xorigin-parent branch from 166eeea to b2531fc Mar 15, 2017

asajeffrey removed the S-blocked-on-external label Mar 15, 2017

highfive added the S-awaiting-review label Mar 16, 2017

asajeffrey force-pushed the asajeffrey:script-window-xorigin-parent branch from 8e2c901 to 2563a31 Mar 16, 2017

highfive added the S-needs-rebase label Mar 16, 2017

nox reviewed Mar 17, 2017

View changes

nox added S-awaiting-answer and removed S-awaiting-review labels Mar 17, 2017

asajeffrey reviewed Mar 17, 2017

View changes

highfive added the S-awaiting-review label Mar 17, 2017

asajeffrey force-pushed the asajeffrey:script-window-xorigin-parent branch from 97bbf4f to 2155054 Mar 17, 2017

nox added S-needs-squash and removed S-awaiting-answer S-awaiting-review S-fails-tidy S-needs-rebase labels Mar 17, 2017

Implement dissimilar-origin window.parent and window.top.

Loading status checks…

6bcb5f6

asajeffrey force-pushed the asajeffrey:script-window-xorigin-parent branch from 2155054 to 6bcb5f6 Mar 17, 2017

highfive added the S-awaiting-review label Mar 17, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Mar 17, 2017

bors-servo merged commit 6bcb5f6 into servo:master Mar 17, 2017
3 checks passed

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

highfive removed the S-awaiting-merge label Mar 17, 2017

servo / servo

Join GitHub today

Implement dissimilar-origin window.parent and window.top #15799

Implement dissimilar-origin window.parent and window.top #15799

Conversation

asajeffrey commented Mar 2, 2017 • edited by Ms2ger

highfive commented Mar 2, 2017

highfive commented Mar 2, 2017

asajeffrey commented Mar 2, 2017

KiChjang commented Mar 2, 2017

asajeffrey commented Mar 2, 2017

Ms2ger commented Mar 3, 2017

asajeffrey commented Mar 3, 2017

nox commented Mar 3, 2017 • edited

asajeffrey commented Mar 3, 2017

bors-servo commented Mar 15, 2017

asajeffrey commented Mar 15, 2017

bors-servo commented Mar 15, 2017

asajeffrey commented Mar 15, 2017

bors-servo commented Mar 16, 2017

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Mar 17, 2017

nox commented Mar 17, 2017

asajeffrey commented Mar 17, 2017

bors-servo commented Mar 17, 2017

bors-servo commented Mar 17, 2017

bors-servo commented Mar 17, 2017

asajeffrey commented Mar 2, 2017 •

edited by Ms2ger

nox commented Mar 3, 2017 •

edited