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

Change all DOMStrings to USV strings for XHR #9294

Merged
merged 1 commit into from Jan 17, 2016
Merged

Change all DOMStrings to USV strings for XHR #9294

merged 1 commit into from Jan 17, 2016

Conversation

@KiChjang
Copy link
Member

KiChjang commented Jan 13, 2016

This is in compliance with the new spec here.

Review on Reviewable
@highfive
Copy link

highfive commented Jan 13, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 13, 2016

If this doesn't fix any tests, we should write some.
@KiChjang
Copy link
Member Author

KiChjang commented Jan 13, 2016

Only one way to know.

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

⌛ Trying commit d5b227e with merge be83701...
bors-servo added a commit that referenced this pull request Jan 13, 2016
@bors-servo
Auto merge of #9294 - KiChjang:xhr-usvstring, r=<try>
be83701 
Change all DOMStrings to USV strings for XHR

This is in compliance with the new spec [here](https://xhr.spec.whatwg.org/#xmlhttprequest).

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

bors-servo commented Jan 13, 2016

💔 Test failed - mac-dev-ref-unit
@KiChjang
Copy link
Member Author

KiChjang commented Jan 13, 2016

Wow. That was quick.

@bors-servo retry

  • Networking issue with git, perhaps?
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2016

⌛ Trying commit d5b227e with merge 483a5af...
bors-servo added a commit that referenced this pull request Jan 13, 2016
@bors-servo
Auto merge of #9294 - KiChjang:xhr-usvstring, r=<try>
483a5af 
Change all DOMStrings to USV strings for XHR

This is in compliance with the new spec [here](https://xhr.spec.whatwg.org/#xmlhttprequest).

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

bors-servo commented Jan 13, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@KiChjang
Copy link
Member Author

KiChjang commented Jan 13, 2016

@Ms2ger I'm not sure what to test for in this case. I'm assuming USVStrings still get converted to normal JS Strings?
@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 14, 2016

For things that return USVString, there's nothing you can do. You should test that unpaired surrogates in the arguments to open are replaced by U+fffd, though.
@KiChjang KiChjang force-pushed the KiChjang:xhr-usvstring branch from d5b227e to 87ed042 Jan 15, 2016
@KiChjang KiChjang removed the C-needs-test label Jan 15, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Jan 15, 2016

Added tests, one should be enough.
@nox
Copy link
Member

nox commented Jan 15, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2016

📌 Commit 87ed042 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2016

⌛ Testing commit 87ed042 with merge ef15faa...
@nox nox self-assigned this Jan 15, 2016
bors-servo added a commit that referenced this pull request Jan 15, 2016
@bors-servo
Auto merge of #9294 - KiChjang:xhr-usvstring, r=nox
ef15faa 
Change all DOMStrings to USV strings for XHR

This is in compliance with the new spec [here](https://xhr.spec.whatwg.org/#xmlhttprequest).

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

Ms2ger commented Jan 15, 2016

Why does your new test fail?
@nox
Copy link
Member

nox commented Jan 15, 2016

What. I can't read. Just read the test and didn't read the expectations… :(

@bors-servo r-
@bors-servo
Copy link
Contributor

bors-servo commented Jan 15, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@KiChjang KiChjang force-pushed the KiChjang:xhr-usvstring branch from 87ed042 to 76d01e8 Jan 15, 2016
@KiChjang KiChjang force-pushed the KiChjang:xhr-usvstring branch from 57f7822 to 7b475f7 Jan 15, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Jan 15, 2016

I've updated the test to instead use \u00DF to represent the german eszett (the ß character). Somehow, the test file stores the ß character as instead of the unicode \u00DF.

With that, the test should now pass.
@KiChjang KiChjang force-pushed the KiChjang:xhr-usvstring branch from 7b475f7 to a64f832 Jan 16, 2016
@KiChjang
Change all DOMStrings to USV strings for XHR
Loading status checks…
a64f832
@nox
Copy link
Member

nox commented Jan 16, 2016

@bors-servo r+

Reviewed 4 of 4 files at r1.
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 Jan 16, 2016

📌 Commit a64f832 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2016

⌛ Testing commit a64f832 with merge dca89e4...
bors-servo added a commit that referenced this pull request Jan 16, 2016
@bors-servo
Auto merge of #9294 - KiChjang:xhr-usvstring, r=nox
dca89e4 
Change all DOMStrings to USV strings for XHR

This is in compliance with the new spec [here](https://xhr.spec.whatwg.org/#xmlhttprequest).

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

bors-servo commented Jan 16, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member Author

KiChjang commented Jan 16, 2016

@bors-servo retry
@nox nox added S-awaiting-merge and removed S-tests-failed labels Jan 16, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2016

⌛ Testing commit a64f832 with merge cd9284e...
bors-servo added a commit that referenced this pull request Jan 17, 2016
@bors-servo
Auto merge of #9294 - KiChjang:xhr-usvstring, r=nox
cd9284e 
Change all DOMStrings to USV strings for XHR

This is in compliance with the new spec [here](https://xhr.spec.whatwg.org/#xmlhttprequest).

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

bors-servo commented Jan 17, 2016

💔 Test failed - linux-rel
@nox
Copy link
Member

nox commented Jan 17, 2016

Weird. What failed this time?
@KiChjang
Copy link
Member Author

KiChjang commented Jan 17, 2016

@bors-servo retry

  • Buildbot interruption
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2016

⌛ Testing commit a64f832 with merge 7ae16c7...
bors-servo added a commit that referenced this pull request Jan 17, 2016
@bors-servo
Auto merge of #9294 - KiChjang:xhr-usvstring, r=nox
Loading status checks…
7ae16c7 
Change all DOMStrings to USV strings for XHR

This is in compliance with the new spec [here](https://xhr.spec.whatwg.org/#xmlhttprequest).

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

bors-servo commented Jan 17, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit a64f832 into servo:master Jan 17, 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:xhr-usvstring branch Jan 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

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 @highfive @Ms2ger @bors-servo @nox @frewsxcv
You can’t perform that action at this time.