Closed Bug 1074886 Opened 5 years ago Closed 5 years ago

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

Categories

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

34 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: crimsteam, Assigned: sciarp, Mentored)

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 file, 2 obsolete files)

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>
Summary: URLSearchParams.get() should return null (not empty string) → URLSearchParams.get() should return null (not empty string) when not find pair
Fix is simple - instead of truncating the string in URLSearchParams::Get(), call SetIsVoid(true) on it.
Mentor: nsm.nikhil
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=C++]
No, call SetDOMStringToNull.  SetIsVoid() is an implementation detail.

Are there web-platform-tests for this spec?  If not, we should probably add some...
Attached patch bug1074886.patch (obsolete) — Splinter Review
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?
Attachment #8497666 - Flags: review?(bzbarsky)
Is there a reason why this bug is platform specific?
Because bugs pick up the platform of the user that filed them rather than "All" by default.
OS: Windows 7 → All
Hardware: x86_64 → All
(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 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.
Attachment #8497666 - Flags: review?(bzbarsky) → review+
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
Attached patch bug1074886.patch (obsolete) — Splinter Review
The new patch has been rebased on top of Andreas's changes and adds some tests.
Attachment #8497666 - Attachment is obsolete: true
Attachment #8500154 - Flags: review?(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
Attachment #8500154 - Flags: review?(bzbarsky) → review+
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.
The dom/workers/test/test_urlSearchParams.html test is failing with this patch.  Presumably it needs to be adjusted accordingly?
Flags: needinfo?(sciarp)
Attached patch bug1074886.patchSplinter Review
This new patch fixes the failing test. A grep on the entire repo did not point out other modifications.
Attachment #8500154 - Attachment is obsolete: true
Attachment #8501189 - Flags: review?(bzbarsky)
Flags: needinfo?(sciarp)
https://hg.mozilla.org/mozilla-central/rev/8f67c2d3a748
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.