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.

Sign up for GitHub

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

Jump to bottom

Xhr data #9972

Merged
merged 7 commits into from
Apr 3, 2016
Merged

Xhr data #9972

merged 7 commits into from
Apr 3, 2016

Conversation

dagnir
Copy link
Contributor

@dagnir dagnir commented Mar 11, 2016

Builds on existing work by @emosenkis. Fixes #8015.

This change is Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 11, 2016
@KiChjang
Copy link
Contributor

Why is tests/wpt/web-platform-tests/XMLHttpRequest/data-uri.htm modified?

@KiChjang KiChjang added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 12, 2016
@dagnir
Copy link
Contributor Author

dagnir commented Mar 12, 2016

Based on the discussion on #8807 and looking at the spec, when the response is an error the status message is empty and not 'OK' like the test expects.
https://fetch.spec.whatwg.org/#concept-network-error

As for the comparison for 'Content-Type', servo seems to return the string 'text/html; charset=utf-8', with the space, which doesn't quite match the original expected value.

KiChjang
KiChjang reviewed Mar 12, 2016
View reviewed changes
tests/wpt/web-platform-tests/XMLHttpRequest/data-uri.htm
@@ -23,14 +23,14 @@
if (method.toUpperCase() !== 'GET') {
assert_equals(client.status, 0);
assert_equals(client.responseText, '');
assert_equals(client.statusText, 'OK');
assert_equals(client.statusText, '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, why is this modified from OK to an empty string?

@KiChjang
Copy link
Contributor

My concern here is whether Firefox and other browsers pass this test as-is without your changes. We shouldn't modify it unless the test itself is wrong.

In which case, a PR to the upstream repository at https://github.com/w3c/web-platform-tests should be made.

@dagnir
Copy link
Contributor Author

dagnir commented Mar 12, 2016

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

tests/wpt/web-platform-tests/XMLHttpRequest/data-uri.htm, line 26 [r2] (raw file):
Spec says status text is empty when status == 0

tests/wpt/web-platform-tests/XMLHttpRequest/data-uri.htm, line 46 [r2] (raw file):
I was looking at this https://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7, but I seem to have missed the part about parameter values being case sensitive. Do you think it matters for this particular case (utf-8 vs UTF-8)?

Comments from the review on Reviewable.io

@dagnir
Copy link
Contributor Author

dagnir commented Mar 12, 2016

Regarding opening a PR with https://github.com/w3c/web-platform-tests, should I revert the changes here (to the test) and wait for that PR to land before proceeding with this one?

@KiChjang
Copy link
Contributor

Yes, if the tests are wrong, open a PR at w3c/web-platform-tests and wait until that PR is merged.

@KiChjang KiChjang added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Mar 12, 2016
@dagnir
Copy link
Contributor Author

dagnir commented Mar 12, 2016

Thanks, the PR is here: web-platform-tests/wpt#2667. I will update once it's merged and the servo copy is synced.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 14, 2016
@KiChjang KiChjang removed the S-awaiting-review There is new code that needs to be reviewed. label Mar 18, 2016
@jdm
Copy link
Member

jdm commented Mar 30, 2016

Still waiting on an update of our local WPT clone at this point.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #10315) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 1, 2016
@jdm
Copy link
Member

jdm commented Apr 1, 2016

It merged!

@jdm jdm removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Apr 1, 2016
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 2, 2016
@KiChjang KiChjang removed the S-needs-rebase There are merge conflict errors. label Apr 3, 2016
@KiChjang
Copy link
Contributor

KiChjang commented Apr 3, 2016

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 6f2bce7 has been approved by KiChjang

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 3, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 6f2bce7 with merge d35ae3b...

bors-servo pushed a commit that referenced this pull request Apr 3, 2016
Auto merge of #9972 - dagnir:xhr-data, r=KiChjang
d35ae3b 
Xhr data

Builds on existing work by @emosenkis.  Fixes #8015.

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

☀️ Test successful - android, arm32, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

@bors-servo bors-servo merged commit 6f2bce7 into servo:master Apr 3, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@KiChjang KiChjang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dagnir @KiChjang @jdm @bors-servo @highfive @emosenkis