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

Use incumbent settings object's origin in Window.postMessage #15291

Closed
wants to merge 1 commit into from
Closed

Use incumbent settings object's origin in Window.postMessage #15291

wants to merge 1 commit into from

Conversation

@KiChjang
Copy link
Member

KiChjang commented Jan 29, 2017 •
edited by larsbergstrom

Fixes #12715.

This change is Reviewable
@highfive
Copy link

highfive commented Jan 29, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/window.rs
  • @KiChjang: components/script/dom/window.rs
@highfive
Copy link

highfive commented Jan 29, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 30, 2017

Should be https://html.spec.whatwg.org/multipage/webappapis.html#concept-settings-object-origin, not the origin of get_url(), which doesn't have a well-defined meaning AFAICT.

Definitions are here:

Can we find a way to test this?
@KiChjang
Copy link
Member Author

KiChjang commented Jan 30, 2017

I expect this to pass some WPT tests; let's see if that's true.

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jan 30, 2017

⌛ Trying commit be59557 with merge 4755744...
bors-servo added a commit that referenced this pull request Jan 30, 2017
@bors-servo
Auto merge of #15291 - KiChjang:incumbent-postMessage, r=<try>
Loading status checks…
4755744 
Use incumbent settings object's origin in Window.postMessage

Fixes #12715.

<!-- 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/15291)
<!-- Reviewable:end -->
@KiChjang
Copy link
Member Author

KiChjang commented Jan 30, 2017 •
edited

Also, stepping through the code, get_url() seems to return the url that is defined by the window's document object if the global is a window, not sure for the worker's case.
@bors-servo bors-servo mentioned this pull request Jan 30, 2017
Merged
1 of 5 tasks complete
@KiChjang
Copy link
Member Author

KiChjang commented Jan 30, 2017

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 30, 2017

⌛ Trying commit be59557 with merge 762f65a...
bors-servo added a commit that referenced this pull request Jan 30, 2017
@bors-servo
Auto merge of #15291 - KiChjang:incumbent-postMessage, r=<try>
Loading status checks…
762f65a 
Use incumbent settings object's origin in Window.postMessage

Fixes #12715.

<!-- 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/15291)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 31, 2017

💥 Test timed out
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 31, 2017

Note that a document's origin is not the origin of it's url, necessarily.
@KiChjang
Copy link
Member Author

KiChjang commented Jan 31, 2017

@bors-servo try
@KiChjang
Use incumbent settings object's origin in Window.postMessage
Loading status checks…
c1a2c21
@KiChjang KiChjang force-pushed the KiChjang:incumbent-postMessage branch from be59557 to c1a2c21 Jan 31, 2017
@KiChjang
Copy link
Member Author

KiChjang commented Jan 31, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jan 31, 2017

⌛ Trying commit c1a2c21 with merge 3071947...
bors-servo added a commit that referenced this pull request Jan 31, 2017
@bors-servo
Auto merge of #15291 - KiChjang:incumbent-postMessage, r=<try>
Loading status checks…
3071947 
Use incumbent settings object's origin in Window.postMessage

Fixes #12715.

<!-- 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/15291)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 31, 2017

☀️ 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-gnu-dev, windows-msvc-dev
@nox
Copy link
Member

nox commented Feb 1, 2017

We only have cross-origin window proxies right? So this change isn't observable, is it?
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 1, 2017

Yeah, we should probably wait for @avadacatavra's XOW work.
@nox
Copy link
Member

nox commented Feb 1, 2017

@asajeffrey said on IRC he is going to work on cross-origin proxies without waiting for XOWs. I'm confused about that.
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 1, 2017

He's working on cross-reference thread coordination; similar-but-not-same-origin windows are sufficient here.
@KiChjang
Copy link
Member Author

KiChjang commented Feb 3, 2017

In which case, it sounds like this change isn't desirable at this stage.
@KiChjang KiChjang closed this Feb 3, 2017
@KiChjang KiChjang deleted the KiChjang:incumbent-postMessage branch Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@glennw glennw

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@KiChjang @highfive @Ms2ger @bors-servo @nox @glennw
You can’t perform that action at this time.