Construct URLSearchParams from array or object #22555

CYBAI · 2018-12-25T13:58:31Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #22554, fix #22556 and also fix #22557
There are tests in url/urlsearchparams-constructor.any.js for these changes

This change is

highfive · 2018-12-25T13:58:35Z

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

CYBAI · 2018-12-25T14:38:50Z

tests/wpt/metadata/url/urlsearchparams-constructor.any.js.ini

-  [Construct with object with two keys]
-    expected: FAIL
-
-  [Construct with object with NULL, non-ASCII, and surrogate keys]


I've observed the Construct with object with two keys case and Construct with object with NULL, non-ASCII, and surrogate keys case will be PASSed intermittently in my local.

I belive the root cause would be related to the MozMap type is HashMap which is an unordered collection so we might get result in different order when using map 🤔

Per the spec for WebIDL record, it should be ordered maps. Maybe we need to make the map inside MozMap to be BTreeMap instead?

That sounds like a really good idea!

Yeah, let me file an issue for it! :)

Just found #15012. Maybe record type in WebIDL is kind of incorrect now?

@jdm would you think it's fine to file another issue to indicate we'd like to use BTreeMap for record? Or, it's better to leave comment in #15012 ?

I thought no it would be valid to change to BTreeMap now, since full record support is much more complex.

Filed at #22557

CYBAI · 2018-12-25T14:43:23Z

@bors-servo try=wpt

bors-servo · 2018-12-25T14:43:28Z

⌛ Trying commit 5879dd2 with merge eb09196...


        Auto merge of #22555 - CYBAI:new-urlsearchparams, r=<try>

Construct URLSearchParams from array or object --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22554 - [x] There are tests in `url/urlsearchparams-constructor.any.js` for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22555)

bors-servo · 2018-12-25T15:08:59Z

💔 Test failed - linux-rel-wpt

CYBAI · 2018-12-25T15:37:39Z

@bors-servo try=wpt

bors-servo · 2018-12-25T15:37:44Z

⌛ Trying commit 4b4428a with merge ac42b63...


        Auto merge of #22555 - CYBAI:new-urlsearchparams, r=<try>

Construct URLSearchParams from array or object --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22554 - [x] There are tests in `url/urlsearchparams-constructor.any.js` for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22555)

bors-servo · 2018-12-25T15:57:10Z

💔 Test failed - linux-rel-css

jdm · 2018-12-25T16:03:54Z

That failure is #13480.

components/script/dom/urlsearchparams.rs

jdm · 2018-12-25T20:24:37Z

@bors-servo r+

bors-servo · 2018-12-25T20:24:39Z

📌 Commit 6dd3efd has been approved by jdm

bors-servo · 2018-12-26T03:41:33Z

💔 Test failed - mac-rel-css1

jdm · 2018-12-26T04:32:09Z

It's a bit sad to mark the test as intermittent when this PR is fixing a bunch of its tests. Why not change the MozMap implementation in this PR instead?

CYBAI · 2018-12-26T04:39:05Z

@jdm I wondered if it's not good to change MozMap in this PR because I thought it's not related to URLSearchParams itself. But, I also start to think that it should be fine to fix it in this PR and maybe I can separate them into different commits to say why we need to change the MozMap. So, I'm working on fixing the MozMap! Thanks for telling me that you think it's fine to fix MozMap in this PR :)

CYBAI · 2018-12-26T06:12:17Z

@jdm With testing to use BTreeMap locally, I found we can pass Construct with object with NULL, non-ASCII, and surrogate keys consistently and fail Construct with object with two keys consistently.

The reason why we'll fail Construct with object with two keys is, while iterating BTreeMap, the map will be ordered by key. For example, if keys are inserted with c and then a, it will be iterated with a and then c.

After doing more investigation, maybe what we want for the orderer map for WebIDL record is something like indexmap ?

Would you think it's fine to install that crate?

CYBAI · 2018-12-26T09:05:37Z

@bors-servo try=wpt

Just tried to use IndexMap and the tests can PASS consistently in my local :)

It's a bit sad to mark the test as intermittent when this PR is fixing a bunch of its tests. Why not change the MozMap implementation in this PR instead?

Sorry again and yeah, I should tryt to avoid introducing a known intermittent in the future, thanks!

bors-servo · 2018-12-26T09:05:42Z

⌛ Trying commit 0d9b21c with merge 312f000...


        Auto merge of #22555 - CYBAI:new-urlsearchparams, r=<try>

Construct URLSearchParams from array or object --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22554, fix #22556 and also fix #22557 - [x] There are tests in `url/urlsearchparams-constructor.any.js` for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22555)

bors-servo · 2018-12-26T09:32:33Z

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

CYBAI · 2018-12-26T09:38:53Z

@bors-servo try=wpt

Let's try againg to see if we'll get that intermittents 👀

bors-servo · 2018-12-26T09:38:58Z

⌛ Trying commit 5f6b61b with merge afcd442...


        Auto merge of #22555 - CYBAI:new-urlsearchparams, r=<try>

Construct URLSearchParams from array or object --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22554, fix #22556 and also fix #22557 - [x] There are tests in `url/urlsearchparams-constructor.any.js` for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22555)

components/script/dom/urlsearchparams.rs

bors-servo · 2018-12-26T10:05:26Z

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


        Construct URLSearchParams from array or object


        Use `IndexMap` for WebIDL Record

jdm · 2018-12-26T13:35:57Z

@bors-servo r+
Thanks for fixing that!

bors-servo · 2018-12-26T13:35:57Z

📌 Commit 115b73f has been approved by jdm

bors-servo · 2018-12-26T13:36:02Z

⌛ Testing commit 115b73f with merge 7bc6c8d...


        Auto merge of #22555 - CYBAI:new-urlsearchparams, r=jdm

Construct URLSearchParams from array or object --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #22554, fix #22556 and also fix #22557 - [x] There are tests in `url/urlsearchparams-constructor.any.js` for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22555)

bors-servo · 2018-12-26T14:56:23Z

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm
Pushing 7bc6c8d to master...

highfive assigned avadacatavra Dec 25, 2018

highfive added the S-awaiting-review label Dec 25, 2018

CYBAI reviewed Dec 25, 2018

View changes

highfive added the S-tests-failed label Dec 25, 2018

CYBAI force-pushed the CYBAI:new-urlsearchparams branch from 5879dd2 to 4b4428a Dec 25, 2018

highfive removed the S-tests-failed label Dec 25, 2018

highfive added the S-tests-failed label Dec 25, 2018

jdm requested changes Dec 25, 2018

View changes

components/script/dom/urlsearchparams.rs Outdated Show resolved Hide resolved

jdm added S-needs-code-changes and removed S-awaiting-review S-tests-failed labels Dec 25, 2018

jdm assigned jdm and unassigned avadacatavra Dec 25, 2018

CYBAI force-pushed the CYBAI:new-urlsearchparams branch from 4b4428a to 6dd3efd Dec 25, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 25, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 25, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Dec 26, 2018

CYBAI force-pushed the CYBAI:new-urlsearchparams branch from 0d9b21c to 5f6b61b Dec 26, 2018

emilio reviewed Dec 26, 2018

View changes

components/script/dom/urlsearchparams.rs Outdated Show resolved Hide resolved

CYBAI added 2 commits Dec 25, 2018

Construct URLSearchParams from array or object

4ba0ab7

Use `IndexMap` for WebIDL Record

Loading status checks…

115b73f

CYBAI force-pushed the CYBAI:new-urlsearchparams branch from 5f6b61b to 115b73f Dec 26, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 26, 2018

bors-servo merged commit 115b73f into servo:master Dec 26, 2018
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 26, 2018

CYBAI deleted the CYBAI:new-urlsearchparams branch Dec 26, 2018

CYBAI mentioned this pull request Jan 7, 2019

Move from OrderedMap to IndexMap for custom properties #22643

Closed

servo / servo

Construct URLSearchParams from array or object #22555

Construct URLSearchParams from array or object #22555

CYBAI commented Dec 25, 2018 •

edited

highfive commented Dec 25, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

CYBAI commented Dec 25, 2018

bors-servo commented Dec 25, 2018

bors-servo commented Dec 25, 2018

CYBAI commented Dec 25, 2018

bors-servo commented Dec 25, 2018

bors-servo commented Dec 25, 2018

jdm commented Dec 25, 2018

jdm commented Dec 25, 2018

bors-servo commented Dec 25, 2018

bors-servo commented Dec 26, 2018

jdm commented Dec 26, 2018

CYBAI commented Dec 26, 2018

CYBAI commented Dec 26, 2018

CYBAI commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

CYBAI commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

jdm commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

servo / servo

Join GitHub today

Construct URLSearchParams from array or object #22555

Construct URLSearchParams from array or object #22555

Conversation

CYBAI commented Dec 25, 2018 • edited

highfive commented Dec 25, 2018

This comment has been minimized.

CYBAI Dec 25, 2018 • edited

This comment has been minimized.

jdm Dec 25, 2018

This comment has been minimized.

CYBAI Dec 25, 2018

This comment has been minimized.

CYBAI Dec 25, 2018

This comment has been minimized.

jdm Dec 25, 2018

This comment has been minimized.

CYBAI Dec 26, 2018

CYBAI commented Dec 25, 2018

bors-servo commented Dec 25, 2018

bors-servo commented Dec 25, 2018

CYBAI commented Dec 25, 2018

bors-servo commented Dec 25, 2018

bors-servo commented Dec 25, 2018

jdm commented Dec 25, 2018

jdm commented Dec 25, 2018

bors-servo commented Dec 25, 2018

bors-servo commented Dec 26, 2018

jdm commented Dec 26, 2018

CYBAI commented Dec 26, 2018

CYBAI commented Dec 26, 2018

CYBAI commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

CYBAI commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

jdm commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

bors-servo commented Dec 26, 2018

CYBAI commented Dec 25, 2018 •

edited

CYBAI Dec 25, 2018 •

edited