Skip to content
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

Disable cross origin check for mozbrowser-enabled top level pipelines #10068

Merged
merged 1 commit into from Mar 28, 2016

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Mar 18, 2016

Closes #9582


This change is Reviewable

@jdm jdm assigned jdm and unassigned glennw Mar 18, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 18, 2016

Remove components/script/parser.out.

@paulrouget paulrouget force-pushed the paulrouget:disableXOriginCheck branch from 41aa98e to fd6878c Mar 18, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 21, 2016

Remove components/script/parser.out.

Done.

@paulrouget paulrouget mentioned this pull request Mar 21, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 23, 2016

Anything in particular that might prevent this to land?

@jdm
Copy link
Member

jdm commented Mar 24, 2016

-S-awaiting-review +S-needs-code-changes


Reviewed 3 of 4 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


components/script/dom/xmlhttprequest.rs, line 878 [r2] (raw file):
This breaks XHR in workers, since this method panics for non-windows.


tests/wpt/mozilla/tests/mozilla/mozbrowser/crossorigin_xhr.html, line 14 [r2] (raw file):
this.unreached_func("Cross origin xhr() should not have failed");


tests/wpt/mozilla/tests/mozilla/mozbrowser/crossorigin_xhr.html, line 19 [r2] (raw file):
this.step_func_done and remove the t.done().


Comments from the review on Reviewable.io

@paulrouget paulrouget force-pushed the paulrouget:disableXOriginCheck branch from fd6878c to 0b65411 Mar 25, 2016
@paulrouget
Copy link
Contributor Author

paulrouget commented Mar 25, 2016

Addressed comments. For workers, I don't think there's an easy way to know if it's part of the top level window. We don't need it now though.

@jdm
Copy link
Member

jdm commented Mar 28, 2016

-S-awaiting-review +S-needs-code-changes


Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/script/dom/xmlhttprequest.rs, line 878 [r1] (raw file):

if let GlobalRoot::Window(win) = self.global() {
    let is_root_pipeline = win.parent_info.is_none();
    ...
}

Comments from the review on Reviewable.io

@paulrouget paulrouget force-pushed the paulrouget:disableXOriginCheck branch from 0b65411 to dd08e90 Mar 28, 2016
@jdm
Copy link
Member

jdm commented Mar 28, 2016

@bors-servo: r+


Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2016

📌 Commit dd08e90 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2016

Testing commit dd08e90 with merge 37799a4...

bors-servo added a commit that referenced this pull request Mar 28, 2016
Disable cross origin check for mozbrowser-enabled top level pipelines

Closes #9582

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10068)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 28, 2016

@bors-servo bors-servo merged commit dd08e90 into servo:master Mar 28, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.