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

Update fetch attributes to match the new spec #9675

Merged
merged 1 commit into from Feb 27, 2016
Merged

Update fetch attributes to match the new spec #9675

merged 1 commit into from Feb 27, 2016

Conversation

@KiChjang
Copy link
Member

KiChjang commented Feb 17, 2016

This should make fetch match the spec more closely.

Review on Reviewable
@highfive
Copy link

highfive commented Feb 17, 2016

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch 2 times, most recently from 6efcf0a to efcde05 Feb 17, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Feb 17, 2016

@nikkisquared Travis CI is complaining about the unit tests, specifically the line where it asserts the ResponseType to be Basic.

How is it possible? According to my fetch spec reading, during main fetch, the ResponseTainting should be set to Opaque because request.mode is NoCORS, so the resulting ResponseType should also be Opaque.
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch from efcde05 to c073cec Feb 17, 2016
@jdm
Copy link
Member

jdm commented Feb 17, 2016

A more descriptive commit message might be nice here.
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch 2 times, most recently from 1ab6a49 to 29ba385 Feb 17, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2016

☔ The latest upstream changes (presumably #9608) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch from 29ba385 to 8a2bf04 Feb 18, 2016
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch from 8a2bf04 to 9bb447f Feb 18, 2016
@KiChjang KiChjang changed the title Refactor fetch request's attributes Update fetch attributes to match the new spec Feb 18, 2016
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch 2 times, most recently from 8209e2e to ef99a1c Feb 18, 2016
@jdm jdm self-assigned this Feb 26, 2016
@jdm
Copy link
Member

jdm commented Feb 26, 2016

Nice improvements! There's one big issue around origins that I want to make sure we get right before merging, however.
-S-awaiting-review +S-needs-code-changes

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

components/net/fetch/methods.rs, line 56 [r2] (raw file):
This makes me uncomfortable, since it's unsafe by default. I'd prefer to either leave the origin alone or add an unimplemented!() to make it clear what's going on.

components/net_traits/request.rs, line 124 [r2] (raw file):
Due to my comment about not modifying the origin in the Client case, I think it's important to reinstate this argument to force callers to consider the origin in use. We could accept an Option value and use the Client origin if an explicit URL origin isn't present.

Comments from the review on Reviewable.io
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch from ef99a1c to 3cf70cd Feb 27, 2016
@KiChjang
Update fetch attributes to match the new spec
Loading status checks…
9697124
@KiChjang KiChjang force-pushed the KiChjang:fetch-attributes branch from 3cf70cd to 9697124 Feb 27, 2016
@jdm
Copy link
Member

jdm commented Feb 27, 2016

@bors-servo: r+
Thanks!

Reviewed 3 of 3 files at r3.
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 Feb 27, 2016

📌 Commit 9697124 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Feb 27, 2016

⌛ Testing commit 9697124 with merge 4e244b1...
bors-servo added a commit that referenced this pull request Feb 27, 2016
@bors-servo
Auto merge of #9675 - KiChjang:fetch-attributes, r=jdm
Loading status checks…
4e244b1 
Update fetch attributes to match the new spec

This should make fetch match the spec more closely.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9675)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 27, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 9697124 into servo:master Feb 27, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Feb 27, 2016
Merged
@KiChjang KiChjang deleted the KiChjang:fetch-attributes branch May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

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

Successfully merging this pull request may close these issues.

None yet

4 participants
@KiChjang @highfive @jdm @bors-servo
You can’t perform that action at this time.