1074886 - URLSearchParams.get() should return null (not empty string) when not find pair

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

Pers URL spec for get() method:
https://url.spec.whatwg.org/#dom-urlsearchparams-get
"The get(name) method must return the value of the first name-value pair whose name is name, and null if there is no such pair."

but now Firefox return empty string. Small test:

<script>

 var USP = new URLSearchParams();

 document.write(typeof USP.get("")+ "<br>"); // string
 document.write(USP.get("") + "<br>"); // ""
 document.write(USP.get("").length + "<br>"); 0

</script>

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Fix is simple - instead of truncating the string in URLSearchParams::Get(), call SetIsVoid(true) on it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

No, call SetDOMStringToNull.  SetIsVoid() is an implementation detail.

Are there web-platform-tests for this spec?  If not, we should probably add some...

Comment 3

[Donato Sciarra]

Attachment: bug1074886.patch

are web-platform-tests...javascript API tests? Like these ones: http://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/url/a-element.html

Shall the tests go in there?

Comment 4

[Donato Sciarra]

Is there a reason why this bug is platform specific?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Because bugs pick up the platform of the user that filed them rather than "All" by default.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Donato Sciarra from comment #3)
> Created attachment 8497666 [details] [diff] [review]
> bug1074886.patch
> 
> are web-platform-tests...javascript API tests? Like these ones:
> http://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/url/
> a-element.html
> 
> Shall the tests go in there?

The web-platform-tests are maintained by the w3c at https://github.com/w3c/web-platform-tests.
URLSearchParams does not have wpt tests yet. For now we have several tests for URLSearchParams in dom/base/test and dom/workers/test. Moving those tests to wpt would be a good first step.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8497666 [details] [diff] [review]
bug1074886.patch

r=me, but please do add a test.  In our tests for now, a followup for adding to w-p-t is good.

Comment 8

[Andrea Marchesini [:baku]]

Hi Donato, I just landed a patch for bug 1074963. You have to rebase your patch on top of mine.
Tell me if you have any problem.

For the test, probably you want to change dom/base/test/test_urlSearchParams.html

Comment 9

[Donato Sciarra]

Attachment: bug1074886.patch

The new patch has been rebased on top of Andreas's changes and adds some tests.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8500154 [details] [diff] [review]
bug1074886.patch

>+    is(u.get('foo'), null, 'URLSearchParams.get(foo)');

ise(), please, just to make sure JS is not messing with your equality tests.

>+    is(u.get(''), null, "URL.searchParams.get('') should be null");

And here.

>+    is(url.searchParams.get('b'), null, "URL.searchParams.get('b') should be null");

And here.

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24a0cb7de6d8 is the try run with that patch using is().  This should be ok, since "" == null is in fact false in JS, so is() and ise() are equivalent enough here.

Comment 12

[Boris Zbarsky [:bzbarsky]]

The dom/workers/test/test_urlSearchParams.html test is failing with this patch.  Presumably it needs to be adjusted accordingly?

Comment 13

[Donato Sciarra]

Attachment: bug1074886.patch

This new patch fixes the failing test. A grep on the entire repo did not point out other modifications.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8501189 [details] [diff] [review]
bug1074886.patch

r=me

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f67c2d3a748

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f67c2d3a748