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

Thread the status through navigation redirects #21933

Merged
merged 2 commits into from Oct 16, 2018

Conversation

Projects
None yet
7 participants
@notriddle
Contributor

notriddle commented Oct 13, 2018

This is necessary because status codes affect whether the redirect is done with the same HTTP method or a different one.

This is part of #21886, but there's also a flaw in how iframes are handled that is causing the redirect to take over the entire window, so this commit doesn't entirely fix slither.io.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes are part of #21886, but more changes are needed to actually fix it
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

Thread the status through navigation redirects
This is necessary because status codes affect whether the redirect is
done with the same HTTP method or a different one.

This is part of #21886, but there's also a flaw in how iframes are handled
that is causing the redirect to take over the entire window, so this
commit doesn't entirely fix slither.io.
@highfive

This comment has been minimized.

highfive commented Oct 13, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/network_listener.rs
  • @cbrewster: components/constellation/network_listener.rs
  • @paulrouget: components/constellation/network_listener.rs
  • @KiChjang: components/net_traits/response.rs
@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 13, 2018

I'm going to see if WPT already has a test case for this (it seems like they should).

@emilio

This comment has been minimized.

Member

emilio commented Oct 14, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 14, 2018

⌛️ Trying commit 3b1bfa3 with merge 90bdb37...

bors-servo added a commit that referenced this pull request Oct 14, 2018

Auto merge of #21933 - notriddle:navigation-redirect-status-code, r=<…
…try>

Thread the status through navigation redirects

This is necessary because status codes affect whether the redirect is done with the same HTTP method or a different one.

This is part of #21886, but there's also a flaw in how iframes are handled that is causing the redirect to take over the entire window, so this commit doesn't entirely fix slither.io.

---
<!-- 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 are part of #21886, but more changes are needed to actually fix it

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/21933)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

💥 Test timed out

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 15, 2018

Apparently, there's no test case targetting this one (it shows that test-wpt passed, in the buildbot page, even though homu specifically timed out).

@servo-wpt-sync

This comment has been minimized.

Collaborator

servo-wpt-sync commented Oct 15, 2018

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#13511.

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 15, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

⌛️ Trying commit 65016d1 with merge 158e5bf...

bors-servo added a commit that referenced this pull request Oct 15, 2018

Auto merge of #21933 - notriddle:navigation-redirect-status-code, r=<…
…try>

Thread the status through navigation redirects

This is necessary because status codes affect whether the redirect is done with the same HTTP method or a different one.

This is part of #21886, but there's also a flaw in how iframes are handled that is causing the redirect to take over the entire window, so this commit doesn't entirely fix slither.io.

---
<!-- 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 are part of #21886, but more changes are needed to actually fix it

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/21933)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

💥 Test timed out

@notriddle

This comment has been minimized.

Contributor

notriddle commented Oct 15, 2018

It looks like it passed 😇

@nox

This comment has been minimized.

Member

nox commented Oct 16, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

📌 Commit 65016d1 has been approved by nox

bors-servo added a commit that referenced this pull request Oct 16, 2018

Auto merge of #21933 - notriddle:navigation-redirect-status-code, r=nox
Thread the status through navigation redirects

This is necessary because status codes affect whether the redirect is done with the same HTTP method or a different one.

This is part of #21886, but there's also a flaw in how iframes are handled that is causing the redirect to take over the entire window, so this commit doesn't entirely fix slither.io.

---
<!-- 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 are part of #21886, but more changes are needed to actually fix it

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/21933)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

⌛️ Testing commit 65016d1 with merge aa95911...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

@bors-servo bors-servo merged commit 65016d1 into servo:master Oct 16, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Oct 16, 2018

Merged

Update hyper to 0.12 #21644

@notriddle notriddle deleted the notriddle:navigation-redirect-status-code branch Oct 16, 2018

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