Make URLSearchParams iterable #13022 #13077

yoyo930021 · 2016-08-27T06:50:26Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13022 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is


        add test for URLSearthParams


        Make URLSearchParams iterable #13022

highfive · 2016-08-27T06:50:31Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @connorgbrewster (or someone else) soon.

highfive · 2016-08-27T06:50:32Z

Heads up! This PR modifies the following files:

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

izgzhen · 2016-08-27T07:43:15Z

Use ./mach update-manifest instead of changing it manually can solve the manifest_changed.sh problem

jdm · 2016-08-27T07:58:00Z

tests/wpt/web-platform-tests/url/urlsearchparams-foreach.html

+<script>
+test(function() {
+    var params = new URLSearchParams('a=1&b=2&c=3');
+    for(var URLSearchParam in params){


This test doesn't actually verify that the keys are present. I don't think for in works on iterators this way, either, since Firefox didn't show me any of those keys when I tried it.

Yes, I test.
for-in isn't used iterators.

emilio · 2016-08-27T08:05:33Z

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/script/dom/urlsearchparams.rs, line 172 [r2] (raw file):

    pub fn sort_list(&self) -> Vec<(String, String)> {
        let borrowed_list = self.list.borrow();
        let URLSearchParams_iter = borrowed_list.iter();

nit: Use snake_case variable names.

components/script/dom/urlsearchparams.rs, line 194 [r2] (raw file):

    fn get_value_at_index(&self, n: u32) -> USVString {
        let sorted_URLSearchParam_vec = self.sort_list();

why are we sorting this each time we're accessed? I don't think that's necessary?

Comments from Reviewable


        modify urlsearchparams tests


        Make URLSearchParams iterable #13022 to delete sort

yoyo930021 · 2016-08-27T08:44:06Z

@emilio
Yes,no need sort.
I finished.

KiChjang · 2016-08-27T08:49:00Z

components/script/dom/urlsearchparams.rs

+    type Value = USVString;
+
+    fn get_iterable_length(&self) -> u32 {
+        self.list.borrow().iter().count() as u32


Doesn't self.list.borrow().len() work?

Yes, it should, r=me with that. A test with something like what is described in #10351 would be great too :)

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" ]

@KiChjang
It work!
Thank you.

@emilio
Add test!
finish!

emilio · 2016-08-27T09:05:15Z

@bors-servo: try

bors-servo · 2016-08-27T09:05:18Z

⌛ Trying commit 8a16a43 with merge 638b553...


        Auto merge of #13077 - yoyo930021:master, r=<try>

Make URLSearchParams iterable #13022  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13022 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13077)


        Make URLSearchParams iterable #13022 to modify iterable length

nox · 2016-08-27T09:28:03Z

The test is incorrect, it could pass if the URLSearchParams is thought to be empty.


        modify tests for for-in

yoyo930021 · 2016-08-27T09:40:33Z

@nox
tests to add
if URLSearchParams is nothing?


        modify tests to add if empty status

Ms2ger · 2016-08-31T08:30:50Z

tests/wpt/web-platform-tests/url/urlsearchparams-foreach.html

+
+test(function() {
+    var params = new URLSearchParams('a=1&b=2&c=3');
+    params.forEach(function(value,index){


I think the suggestion was to make this test more like

var found = []; params.forEach(function(value, index) { found.push(value); }); assert_array_equals(['1', '2', '3']);

nit: space after comma here.

Ms2ger · 2016-09-05T14:44:08Z

@yoyo930021 hey, do you know if you'll have time to finish this PR?

yoyo930021 · 2016-09-05T14:59:04Z

I dont know!
I will try tomorrow.


        modify tests to add foreach

cbrewster · 2016-09-12T17:51:29Z

r? @emilio

emilio

Sorry for the huge delay in reviewing this! There are only formatting nits to fix.

If for some reason you're busy and have the PR marked as editable for the Servo members, I can make the fixes myself and get this landed.

Thank you so much for your contribution, and sorry for the delay again, it has totally been my fault!

emilio · 2016-09-21T23:37:56Z

tests/wpt/web-platform-tests/url/urlsearchparams-foreach.html

+    let b = a.searchParams;
+    var c = [];
+    for (i of b) {
+        a.search="x=1&y=2&z=3";


nit: Space before and after equals here.

emilio · 2016-09-21T23:37:56Z

tests/wpt/web-platform-tests/url/urlsearchparams-foreach.html

+}, "For-of Check");
+
+test(function(){
+    let a=new URL("http://a.b/c");


nit: Space before and after equals here.

emilio · 2016-09-21T23:37:56Z

tests/wpt/web-platform-tests/url/urlsearchparams-foreach.html

+    for (i of b) {
+        c.push(i);
+    }
+    assert_equals(c.length,0);


nit: Space after comma here.

bors-servo · 2016-10-07T14:10:26Z

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


        Make URLSearchParams iterable.

Fixes #13022. Fixes #13077.


        Auto merge of #13637 - servo:URLSearchParams, r=emilio

Make URLSearchParams iterable. Fixes #13022. Fixes #13077.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13637)

yoyo930021 added 2 commits Aug 27, 2016

add test for URLSearthParams

66c8496

Make URLSearchParams iterable #13022

Loading status checks…

564cedc

highfive assigned cbrewster Aug 27, 2016

highfive added the S-awaiting-review label Aug 27, 2016

shinglyu modified the milestone: Taiwan Code Sprint Aug 27, 2016

jdm reviewed Aug 27, 2016
View changes

yoyo930021 added 2 commits Aug 27, 2016

modify urlsearchparams tests

ba6e975

Make URLSearchParams iterable #13022 to delete sort

Loading status checks…

8a16a43

KiChjang reviewed Aug 27, 2016
View changes

Make URLSearchParams iterable #13022 to modify iterable length

Loading status checks…

2e1d3a5

modify tests for for-in

Loading status checks…

01d4888

modify tests to add if empty status

Loading status checks…

a9ef63f

Ms2ger reviewed Aug 31, 2016
View changes

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Sep 5, 2016

modify tests to add foreach

Loading status checks…

cdc5b91

highfive removed the S-needs-code-changes label Sep 12, 2016

highfive added the S-awaiting-review label Sep 12, 2016

highfive assigned emilio and unassigned cbrewster Sep 12, 2016

emilio reviewed Sep 21, 2016

View changes

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Sep 21, 2016

highfive added the S-needs-rebase label Oct 7, 2016

Ms2ger added a commit that referenced this pull request Oct 7, 2016

Make URLSearchParams iterable.

Loading status checks…

980c644

Fixes #13022. Fixes #13077.

Ms2ger mentioned this pull request Oct 7, 2016

Make URLSearchParams iterable. #13637

Merged

bors-servo closed this in #13637 Oct 7, 2016

servo / servo

Make URLSearchParams iterable #13022 #13077

Make URLSearchParams iterable #13022 #13077

yoyo930021 commented Aug 27, 2016 •

edited by larsbergstrom

highfive commented Aug 27, 2016

highfive commented Aug 27, 2016

izgzhen commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 27, 2016

yoyo930021 commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 27, 2016

bors-servo commented Aug 27, 2016

nox commented Aug 27, 2016

yoyo930021 commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Sep 5, 2016

yoyo930021 commented Sep 5, 2016

cbrewster commented Sep 12, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 7, 2016

servo / servo

Join GitHub today

Make URLSearchParams iterable #13022 #13077

Make URLSearchParams iterable #13022 #13077

Conversation

yoyo930021 commented Aug 27, 2016 • edited by larsbergstrom

highfive commented Aug 27, 2016

highfive commented Aug 27, 2016

izgzhen commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 27, 2016

yoyo930021 commented Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 27, 2016

bors-servo commented Aug 27, 2016

nox commented Aug 27, 2016

yoyo930021 commented Aug 27, 2016

This comment has been minimized.

Ms2ger Aug 31, 2016 • edited

This comment has been minimized.

emilio Sep 21, 2016 • edited

Ms2ger commented Sep 5, 2016

yoyo930021 commented Sep 5, 2016

cbrewster commented Sep 12, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 7, 2016

yoyo930021 commented Aug 27, 2016 •

edited by larsbergstrom

Ms2ger Aug 31, 2016 •

edited

emilio Sep 21, 2016 •

edited