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

Implement "potential CORS request" #9516

Merged
merged 1 commit into from Feb 4, 2016
Merged

Implement "potential CORS request" #9516

merged 1 commit into from Feb 4, 2016

Conversation

@KiChjang
Copy link
Member

KiChjang commented Feb 3, 2016

Review on Reviewable
@KiChjang KiChjang force-pushed the KiChjang:potential-cors-request branch 2 times, most recently from 642752e to b9ea083 Feb 3, 2016
components/net_traits/request.rs Outdated
@@ -144,6 +153,50 @@ impl Request {
}
}

// https://html.spec.whatwg.org/multipage/infrastructure.html#create-a-potential-cors-request

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Feb 3, 2016

Member

drop infrastructure.html
@KiChjang KiChjang force-pushed the KiChjang:potential-cors-request branch 4 times, most recently from 236d2bc to 2ff0768 Feb 3, 2016
@jdm
Copy link
Member

jdm commented Feb 3, 2016

@nikkisquared Would you like to review this?
@nikkisquared
Copy link
Contributor

nikkisquared commented Feb 3, 2016

@jdm sure thing! any pointers for how I should go about it?
@jdm
Copy link
Member

jdm commented Feb 3, 2016

  1. Read the specification that is referenced in the changes.
  2. Read the code changes.
  3. Ensure that the code changes implements the specification text. Leave comments on lines that are confusing or incorrect, possibly including suggestions for correcting them.
  4. Consider if there are parts of the changes that could be written in different ways that would improve clarity. Leave comments with suggestions.
  5. Note any stylistic nits that should be addressed.
  6. Leave a summary comment to indicate that the review is complete.
@nikkisquared
Copy link
Contributor

nikkisquared commented Feb 3, 2016

Thanks @jdm!
@nikkisquared
Copy link
Contributor

nikkisquared commented Feb 3, 2016

The destination enum/field doesn't exist, so it can't be set to subresource like the potential-CORS request spec asks for. I'm guessing none of the current Fetch implementation uses it, but now might be a good time to put it in.

Other than that, I couldn't find anything substantial needing change. It looks good to me, excepting a minor nit.

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.

components/net_traits/request.rs, line 83 [r2] (raw file):
The spec says "The empty string is also a valid keyword, and maps to the Anonymous state." I'm not sure if this helpfully applies to Servo's implementation, or where, but it seems like something to keep in mind.

components/net_traits/request.rs, line 156 [r2] (raw file):
Nit: this should be formatted as a markdown link with /// instead of //.

Comments from the review on Reviewable.io
@KiChjang
Implement "potential CORS request"
Loading status checks…
66c2f6d
@KiChjang KiChjang force-pushed the KiChjang:potential-cors-request branch from 2ff0768 to 66c2f6d Feb 3, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Feb 3, 2016

@nikkisquared I was planning to create another PR just for updating all the fields for a Request, but I'm not sure if it'll conflict with your work.

@bors-servo delegate=nikkisquared
@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

✌️ @nikkisquared can now approve this pull request
@KiChjang
Copy link
Member Author

KiChjang commented Feb 3, 2016

@bors-servo r=nikkisquared (from IRC)
@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2016

📌 Commit 66c2f6d has been approved by nikkisquared
@nikkisquared
Copy link
Contributor

nikkisquared commented Feb 3, 2016

@KiChjang Another PR for more Request fields would be great, I made a rough list of inconsistencies between the spec and the implementation a while ago, I've put it on gist here: https://gist.github.com/nikkisquared/5817408dcc279500ecac. I don't think adding fields would necessarily conflict with my work, I've had it on the back of my mind and only adding fields as necessary, which is very infrequent.
@nikkisquared nikkisquared mentioned this pull request Feb 4, 2016
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Feb 4, 2016

⌛ Testing commit 66c2f6d with merge e423172...
bors-servo added a commit that referenced this pull request Feb 4, 2016
@bors-servo
Auto merge of #9516 - KiChjang:potential-cors-request, r=nikkisquared
e423172 
Implement "potential CORS request"

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9516)
<!-- Reviewable:end -->
@bors-servo bors-servo mentioned this pull request Feb 4, 2016
Merged
@bors-servo
Copy link
Contributor

bors-servo commented Feb 4, 2016

💔 Test failed - mac-rel-wpt
@KiChjang
Copy link
Member Author

KiChjang commented Feb 4, 2016

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

bors-servo commented Feb 4, 2016

⌛ Testing commit 66c2f6d with merge df454ac...
bors-servo added a commit that referenced this pull request Feb 4, 2016
@bors-servo
Auto merge of #9516 - KiChjang:potential-cors-request, r=nikkisquared
Loading status checks…
df454ac 
Implement "potential CORS request"

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

bors-servo commented Feb 4, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 66c2f6d into servo:master Feb 4, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@KiChjang KiChjang deleted the KiChjang:potential-cors-request branch Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

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