Implement URL.searchParams #10351

stjepang · 2016-04-01T20:36:26Z

Spec: https://url.spec.whatwg.org/#dom-url-searchparams
Closes #10335.

This change is

highfive · 2016-04-01T20:36:32Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/url.rs, components/script/dom/webidls/URL.webidl

highfive · 2016-04-01T20:36:33Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

larsbergstrom · 2016-04-01T20:42:16Z

r? @Manishearth
(since you commented in the original issue with details on the spec, etc.)

Manishearth · 2016-04-02T19:39:06Z

components/script/dom/url.rs

@@ -24,19 +26,24 @@ pub struct URL {

    // https://url.spec.whatwg.org/#concept-urlutils-get-the-base
    base: Option<Url>,
+
+    // https://url.spec.whatwg.org/#dom-url-searchparams
+    query: JS<URLSearchParams>,


Can we calculate this on-demand instead of storing it?

Technically we can't; a quick test in Firefox shows that mutating the object returned on url.searchParams will be reflected when accessed again. The spec does mention that a URL object has an associated URLSearchParams object, so this is correct I guess.

cc @Ms2ger does this particularly matter?

Chrome doesn't seem to support searchParams at all? It autocompletes in the console but is undefined.

Actually, this is a bit hairier if URLSearchParams is mutable through URL. It's also an iterator.

What happens if I mutate the original URL whilst iterating it?

Firefox iterates index-wise, if you change the query string it will continue to iterate from that index in the new query string if that index exists.

let a=new URL("http://a.b/c?a=1&b=2&c=3&d=4"); let b = a.searchParams; for (i of b) { a.search="x=1&y=2&z=3"; console.log(i); } // outputs Array [ "a", "1" ] Array [ "y", "2" ] Array [ "z", "3" ]

Manishearth · 2016-04-02T19:43:58Z

components/script/dom/url.rs

@@ -191,6 +202,11 @@ impl URLMethods for URL {
        UrlHelper::SetSearch(&mut self.url.borrow_mut(), value);


All the setters should update the stored query object if we're going to store it as a field.

Instead of calculating query pairs on-demand, I'd rather stick to the spec and store them immediately.

Regarding iteration, this is something we currently don't handle - it's commented in URLSearchParams.webidl:
// iterable<USVString, USVString>;
Please correct me if I'm wrong.

Firefox's approach looks reasonable to me. We'll just have to be careful when implementing the iterable interface.

Manishearth · 2016-04-02T22:18:52Z

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

components/script/dom/url.rs, line 31 [r1] (raw file):
Agreed. Perhaps we should store a MutNullableHeap<JS> instead and lazy-load it when content asks for it? I don't like creating extra JS objects here and there.

Also, please leave a comment about the iterable stuff somewhere. I think that bit is simply unspecced, so ultimately it might not matter much.

Comments from Reviewable

stjepang · 2016-04-03T12:19:14Z

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

components/script/dom/url.rs, line 31 [r1] (raw file):
Perhaps. :) If you insist on using MutNullableHeap, I'll do that.
In that case... URLSearchParams::Constructor requires a GlobalRef as an argument. What would be the best way to obtain a GlobalRef when constructing it?

I'll write a comment about the iterable stuff.

Comments from Reviewable

nox · 2016-04-03T12:23:56Z

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

components/script/dom/url.rs, line 31 [r1] (raw file):
It should definitely be MutNullableHeap and you can get the global object of any DOM instance with foo.global(), just use that on the URL value. Also you should probably not use Constructor anyway but new.

I just remembered I had started to implement this, if you need some inspiration.

Comments from Reviewable

stjepang · 2016-04-03T15:19:11Z

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

components/script/dom/url.rs, line 31 [r1] (raw file):
Thanks, that's very useful!

Comments from Reviewable

Manishearth · 2016-04-05T11:11:51Z

@bors-servo r+

Thanks!

Reviewed 1 of 2 files at r1, 1 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

bors-servo · 2016-04-05T11:11:53Z

📌 Commit 471a490 has been approved by Manishearth

bors-servo · 2016-04-05T11:33:23Z

⌛ Testing commit 471a490 with merge 4eb1915...


        Auto merge of #10351 - stjepang:impl-url-searchparams, r=Manishearth

Implement URL.searchParams Spec: https://url.spec.whatwg.org/#dom-url-searchparams Closes #10335.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10351)

bors-servo · 2016-04-05T12:11:10Z

💔 Test failed - mac-rel-wpt

jdm · 2016-04-05T12:32:20Z

  ▶ Unexpected subtest result in /url/interfaces.html:
  └ PASS [expected FAIL] URL interface: attribute searchParams

  ▶ Unexpected subtest result in /url/interfaces.html:
  └ PASS [expected FAIL] URL interface: new URL("http://foo") must inherit property "searchParams" with the proper type (12)

Manishearth · 2016-04-05T14:08:13Z

r=me once you squash these commits


        Implement URL.searchParams

Spec: https://url.spec.whatwg.org/#dom-url-searchparams

Manishearth · 2016-04-05T14:11:55Z

@bors-servo r+

bors-servo · 2016-04-05T14:11:55Z

📌 Commit 7b38f28 has been approved by Manishearth

bors-servo · 2016-04-05T17:39:04Z

💔 Test failed - status-appveyor

KiChjang · 2016-04-05T17:42:52Z

@bors-servo retry

infra

bors-servo · 2016-04-05T18:42:52Z

⌛ Testing commit 7b38f28 with merge a2deaa8...


        Auto merge of #10351 - stjepang:impl-url-searchparams, r=Manishearth

Implement URL.searchParams Spec: https://url.spec.whatwg.org/#dom-url-searchparams Closes #10335.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10351)

bors-servo · 2016-04-05T18:50:12Z

💔 Test failed - status-appveyor

KiChjang · 2016-04-05T18:53:41Z

@bors-servo retry

The Annoying AppVeyor

bors-servo · 2016-04-05T19:30:25Z

⌛ Testing commit 7b38f28 with merge 7d05445...


        Auto merge of #10351 - stjepang:impl-url-searchparams, r=Manishearth

Implement URL.searchParams Spec: https://url.spec.whatwg.org/#dom-url-searchparams Closes #10335.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10351)

bors-servo · 2016-04-05T19:45:22Z

💔 Test failed - status-appveyor

jdm · 2016-04-05T22:11:32Z

@bors-servo: retry

let's do this one more time

Ms2ger · 2016-04-06T07:23:00Z

@bors-servo p=1

bors-servo · 2016-04-06T08:57:31Z

⌛ Testing commit 7b38f28 with merge e36e3be...


        Auto merge of #10351 - stjepang:impl-url-searchparams, r=Manishearth

Implement URL.searchParams Spec: https://url.spec.whatwg.org/#dom-url-searchparams Closes #10335.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10351)

bors-servo · 2016-04-06T10:00:50Z

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

highfive assigned larsbergstrom Apr 1, 2016

highfive added the S-awaiting-review label Apr 1, 2016

highfive assigned Manishearth and unassigned larsbergstrom Apr 1, 2016

Manishearth reviewed Apr 2, 2016
View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 5, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Apr 5, 2016

Implement URL.searchParams

Loading status checks…

7b38f28

Spec: https://url.spec.whatwg.org/#dom-url-searchparams

stjepang force-pushed the stjepang:impl-url-searchparams branch from 7e0cd7a to 7b38f28 Apr 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 5, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 5, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 5, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 5, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 6, 2016

bors-servo merged commit 7b38f28 into servo:master Apr 6, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 6, 2016

paulrouget mentioned this pull request Apr 6, 2016

Remove the searchParams workaround browserhtml/browserhtml#983

Closed

emilio mentioned this pull request Aug 27, 2016

Make URLSearchParams iterable #13022 #13077

Closed

4 of 5 tasks complete

servo / servo

Implement URL.searchParams #10351

Implement URL.searchParams #10351

stjepang commented Apr 1, 2016

highfive commented Apr 1, 2016

highfive commented Apr 1, 2016

larsbergstrom commented Apr 1, 2016

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.

This comment has been minimized.

Manishearth commented Apr 2, 2016

stjepang commented Apr 3, 2016

nox commented Apr 3, 2016

stjepang commented Apr 3, 2016

Manishearth commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

jdm commented Apr 5, 2016

Manishearth commented Apr 5, 2016

Manishearth commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

KiChjang commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

KiChjang commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

jdm commented Apr 5, 2016

Ms2ger commented Apr 6, 2016

bors-servo commented Apr 6, 2016

bors-servo commented Apr 6, 2016

		@@ -191,6 +202,11 @@ impl URLMethods for URL {
		UrlHelper::SetSearch(&mut self.url.borrow_mut(), value);

servo / servo

Join GitHub today

Implement URL.searchParams #10351

Implement URL.searchParams #10351

Conversation

stjepang commented Apr 1, 2016

highfive commented Apr 1, 2016

highfive commented Apr 1, 2016

larsbergstrom commented Apr 1, 2016

This comment has been minimized.

Manishearth Apr 2, 2016

This comment has been minimized.

Manishearth Apr 2, 2016

This comment has been minimized.

Manishearth Apr 2, 2016

This comment has been minimized.

Manishearth Apr 2, 2016

This comment has been minimized.

Manishearth Apr 2, 2016

This comment has been minimized.

Manishearth Apr 2, 2016

This comment has been minimized.

stjepang Apr 2, 2016

Manishearth commented Apr 2, 2016

stjepang commented Apr 3, 2016

nox commented Apr 3, 2016

stjepang commented Apr 3, 2016

Manishearth commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

jdm commented Apr 5, 2016

Manishearth commented Apr 5, 2016

Manishearth commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

KiChjang commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

KiChjang commented Apr 5, 2016

bors-servo commented Apr 5, 2016

bors-servo commented Apr 5, 2016

jdm commented Apr 5, 2016

Ms2ger commented Apr 6, 2016

bors-servo commented Apr 6, 2016

bors-servo commented Apr 6, 2016