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.

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 URLSearchParams.prototype.sort() #22638

Merged
merged 2 commits into from Jan 10, 2019

Conversation

@CYBAI
Copy link
Collaborator

CYBAI commented Jan 6, 2019


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22545
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 6, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/URLSearchParams.webidl, components/script/dom/urlsearchparams.rs
  • @KiChjang: components/script/dom/webidls/URLSearchParams.webidl, components/script/dom/urlsearchparams.rs
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 7, 2019

r? @Manishearth
There's enough questions about counting and comparing unicode values here that it will be quicker for someone with more experience to review this.

@highfive highfive assigned Manishearth and unassigned jdm Jan 7, 2019
@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jan 7, 2019

Comments on the issue indicate they're thinking of the comparison algorithm in https://tc39.github.io/ecma262/#sec-abstract-relational-comparison , which is based on utf16 code units (not utf8)

This implementation can either:

  • convert the list to utf16, sort, and convert back
  • write a custom comparison function for strings that one at a time compares utf16 code units. this is tricky but doable, and is faster
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 8, 2019

I doubt the performance of URLSearchParams.sort is particularly important, so I lean towards doing the less tricky thing.

@CYBAI

This comment has been minimized.

Copy link
Collaborator Author

CYBAI commented Jan 8, 2019

@jdm @Manishearth Thanks! I'll try to do the less tricky thing with comparing with encode_utf16!

@CYBAI CYBAI force-pushed the CYBAI:urlsearchparams-sort branch from eb08240 to f43c604 Jan 8, 2019
@CYBAI

This comment has been minimized.

Copy link
Collaborator Author

CYBAI commented Jan 8, 2019

@bors-servo try=wpt

  • Updated! This is ready for review!
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 8, 2019

⌛️ Trying commit f43c604 with merge dc39da9...

bors-servo added a commit that referenced this pull request Jan 8, 2019
Implement URLSearchParams.prototype.sort()

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22545
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 8, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@nox

This comment has been minimized.

Copy link
Member

nox commented Jan 8, 2019

I doubt the performance of URLSearchParams.sort is particularly important, so I lean towards doing the less tricky thing.

There is no need to be tricky, though. Just use the standard library.

a.chars().flat_map(char::decode_utf16).cmp(b.chars().flat_map(char::decode_utf16))

Edit: this should use flat_map not map.

@nox nox assigned nox and unassigned Manishearth Jan 8, 2019
@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Jan 8, 2019

That won't work, decode_utf16 doesn't return an iterator, and doesn't live long enough to be converted into a flat-mappable iterator

@CYBAI

This comment has been minimized.

Copy link
Collaborator Author

CYBAI commented Jan 8, 2019

Ah, maybe I should share the discussion with @nox on IRC earlier.
@nox just said he wrote the comment in a wrong way and he also asked me to disregard his comment.

So I think this PR is ready for review ?

Ref: https://mozilla.logbot.info/servo/20190108#c15803031

cc @Manishearth

@nox

This comment has been minimized.

Copy link
Member

nox commented Jan 9, 2019

Yes it is, and the code is correct.

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

📌 Commit f43c604 has been approved by nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Testing commit f43c604 with merge 2cd2bd8...

bors-servo added a commit that referenced this pull request Jan 9, 2019
Implement URLSearchParams.prototype.sort()

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22545
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

💔 Test failed - mac-rel-css1

@CYBAI

This comment has been minimized.

Copy link
Collaborator Author

CYBAI commented Jan 9, 2019

@bors-servo retry

{
    "status": "TIMEOUT", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/_mozilla/mozilla/referrer-policy/no-referrer/meta-csp/cross-origin/http-http/link-tag/generic.keep-origin-redirect.http.html", 
    "line": 59369, 
    "action": "test_result", 
    "expected": "OK"
}
bors-servo added a commit that referenced this pull request Jan 9, 2019
Implement URLSearchParams.prototype.sort()

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22545
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Testing commit f43c604 with merge fda1f9e...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

💔 Test failed - mac-rel-css1

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 9, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Testing commit f43c604 with merge f6610ee...

bors-servo added a commit that referenced this pull request Jan 9, 2019
Implement URLSearchParams.prototype.sort()

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22545
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

💔 Test failed - mac-rel-css1

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 9, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

💔 Test failed - mac-rel-css1

@CYBAI

This comment has been minimized.

Copy link
Collaborator Author

CYBAI commented Jan 10, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 10, 2019

⌛️ Testing commit f43c604 with merge 2e15cf0...

bors-servo added a commit that referenced this pull request Jan 10, 2019
Implement URLSearchParams.prototype.sort()

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22545
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 10, 2019

@bors-servo bors-servo merged commit f43c604 into servo:master Jan 10, 2019
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@CYBAI CYBAI deleted the CYBAI:urlsearchparams-sort branch Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.