Bug 166973 – Construct URLSearchParams from array or object

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anne van Kesteren]

Test: http://w3c-test.org/url/urlsearchparams-constructor.html, landed at https://github.com/w3c/web-platform-tests/pull/4523.

Standard: https://github.com/whatwg/url/issues/27, landed at https://github.com/whatwg/url/pull/175.

[Chris Dumez]

(In reply to comment #0)
> Test: http://w3c-test.org/url/urlsearchparams-constructor.html, landed at
> https://github.com/w3c/web-platform-tests/pull/4523.
> 
> Standard: https://github.com/whatwg/url/issues/27, landed at
> https://github.com/whatwg/url/pull/175.

Is http://w3c-test.org/url/urlsearchparams-constructor.html not up-to-date? It seems WebKit is passing the whole test already.

[Anne van Kesteren]

It seems like it's not, there's something broken with mirroring.

[Anne van Kesteren]

It's updated now.

[Alex Christensen]

I have four issues with this so far:
1) The spec doesn't specify the order we iterate the record in.  Our implementation is a HashMap<String, String>.  Is this wrong?
2) There is no test with a sequence of one USVString.  A *very* poor implementation might go out of bounds here.  I think it's worth testing.
3) There probably isn't a test for setting or appending a '+' in an existing URLSearchParams.  Lots of places we replace a '+' with a ' ', but my implementation doesn't in set or append, but there seems to be such a test with the record constructor.  urlsearchparams-stringifier.html has a contradictory test verifying that '+' is stringified to '+'.
4) The HashMap<String, String> I get from the values you see in the patch I'm about to upload.  Is that a problem in our record implementation?

[Alex Christensen]

Created attachment 299064 [details]
Patch

[Anne van Kesteren]

1) URLSearchParams is basically an ordered multimap. The specification uses a list. So yes, if you have an unordered map that should fail many tests.
2) https://github.com/w3c/web-platform-tests/pull/4545 covers that I believe. Feel free to add more tests though.
3) You'll have to elaborate on the concern here. The parser converts "+" to " " and the serializer converts " " to "+". If you operate directly on the URLSearchParams object no conversion takes place so then you might end up with a "+" in the data that would be emitted as " ".
4) A record<> should translate into an ordered map. The iteration order of the JavaScript object is significant.

Hope that helps.

[Sam Weinig]

Comment on attachment 299064 [details]
Patch

Can this use ConstructorMayThrowException?

[Sam Weinig]

We probably want to change our record implementation to be a Vector of pairs to preserve order.

[Sam Weinig]

Comment on attachment 299064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299064&action=review

> Source/WebCore/html/URLSearchParams.cpp:46
> +RefPtr<URLSearchParams> URLSearchParams::create(JSC::ExecState& state, const Variant<Vector<Vector<String>>, HashMap<String, String>, String>& variant)

This should return an ExceptionOr<Ref<URLSearchParams>> and the IDL should be annotated with ConstructorMayThrowException.

[Alex Christensen]

Created attachment 299206 [details]
Patch

[Chris Dumez]

(In reply to comment #9)
> Comment on attachment 299064 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299064&action=review
> 
> > Source/WebCore/html/URLSearchParams.cpp:46
> > +RefPtr<URLSearchParams> URLSearchParams::create(JSC::ExecState& state, const Variant<Vector<Vector<String>>, HashMap<String, String>, String>& variant)
> 
> This should return an ExceptionOr<Ref<URLSearchParams>> and the IDL should
> be annotated with ConstructorMayThrowException.

Oops, sorry about that. I have Alex outdated information :/

[Sam Weinig]

I filed and implemented the change to record in https://bugs.webkit.org/show_bug.cgi?id=167189.

[Sam Weinig]

Comment on attachment 299206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299206&action=review

> Source/WebCore/html/DOMURL.cpp:115
> -        m_searchParams = URLSearchParams::create(search(), this);
> +        m_searchParams = URLSearchParams::create(search(), this).releaseReturnValue();

Why can this bypass the exception check? If it is guaranteed  not to show, can you add an overload of create that doesn't return an ExceptionOr (may have to have another name due to Variant conversion fun)?

> Source/WebCore/html/URLSearchParams.cpp:48
> +        ASSERT(!associatedURL);

If you are asserting !associatedURL in two cases here, it seems like you should have a create overload that just a String and the DOMURL*. That way, this one can remove the DOMURL* parameter, and the assertions.

> Source/WebCore/html/URLSearchParams.cpp:58
> +        return adoptRef(*new URLSearchParams(pairs));

Can this move as well? Why force the copy?

[Alex Christensen]

Created attachment 299254 [details]
Patch

[Alex Christensen]

(In reply to comment #13)
> > Source/WebCore/html/URLSearchParams.cpp:58
> > +        return adoptRef(*new URLSearchParams(pairs));
> 
> Can this move as well? Why force the copy?
Our WTF::Variant implementation doesn't allow you to WTFMove Variants into WTF::Visit.

[Sam Weinig]

(In reply to comment #15)
> (In reply to comment #13)
> > > Source/WebCore/html/URLSearchParams.cpp:58
> > > +        return adoptRef(*new URLSearchParams(pairs));
> > 
> > Can this move as well? Why force the copy?
> Our WTF::Variant implementation doesn't allow you to WTFMove Variants into
> WTF::Visit.

We should fix that I guess :).

[WebKit Commit Bot]

Attachment 299254 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

http://trac.webkit.org/r210946

[Alex Christensen]

Comment on attachment 299254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=299254&action=review

> LayoutTests/imported/w3c/web-platform-tests/url/urlsearchparams-constructor-expected.txt:4
> +FAIL URLSearchParams constructor, DOMException.prototype as argument assert_equals: expected "Error=" but got "INDEX_SIZE_ERR=1&DOMSTRING_SIZE_ERR=2&HIERARCHY_REQUEST_ERR=3&WRONG_DOCUMENT_ERR=4&INVALID_CHARACTER_ERR=5&NO_DATA_ALLOWED_ERR=6&NO_MODIFICATION_ALLOWED_ERR=7&NOT_FOUND_ERR=8&NOT_SUPPORTED_ERR=9&INUSE_ATTRIBUTE_ERR=10&INVALID_STATE_ERR=11&SYNTAX_ERR=12&INVALID_MODIFICATION_ERR=13&NAMESPACE_ERR=14&INVALID_ACCESS_ERR=15&VALIDATION_ERR=16&TYPE_MISMATCH_ERR=17&SECURITY_ERR=18&NETWORK_ERR=19&ABORT_ERR=20&URL_MISMATCH_ERR=21&QUOTA_EXCEEDED_ERR=22&TIMEOUT_ERR=23&INVALID_NODE_TYPE_ERR=24&DATA_CLONE_ERR=25"

Is this remaining failure a problem caused by our Record implementation?

[Sam Weinig]

(In reply to comment #19)
> Comment on attachment 299254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299254&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/url/urlsearchparams-constructor-expected.txt:4
> > +FAIL URLSearchParams constructor, DOMException.prototype as argument assert_equals: expected "Error=" but got "INDEX_SIZE_ERR=1&DOMSTRING_SIZE_ERR=2&HIERARCHY_REQUEST_ERR=3&WRONG_DOCUMENT_ERR=4&INVALID_CHARACTER_ERR=5&NO_DATA_ALLOWED_ERR=6&NO_MODIFICATION_ALLOWED_ERR=7&NOT_FOUND_ERR=8&NOT_SUPPORTED_ERR=9&INUSE_ATTRIBUTE_ERR=10&INVALID_STATE_ERR=11&SYNTAX_ERR=12&INVALID_MODIFICATION_ERR=13&NAMESPACE_ERR=14&INVALID_ACCESS_ERR=15&VALIDATION_ERR=16&TYPE_MISMATCH_ERR=17&SECURITY_ERR=18&NETWORK_ERR=19&ABORT_ERR=20&URL_MISMATCH_ERR=21&QUOTA_EXCEEDED_ERR=22&TIMEOUT_ERR=23&INVALID_NODE_TYPE_ERR=24&DATA_CLONE_ERR=25"
> 
> Is this remaining failure a problem caused by our Record implementation?

I don't think so.  The test states that the correct result would be "Error=", which would be true if passing DOMException.prototype caused the union conversion code to choose to convert to a String. 

But, by my reading of the WebIDL union conversion code (https://heycam.github.io/webidl/#es-union), we should go down step 11. ("If V is any kind of object"), try 11.1 ("If types includes a sequence type, then"), but continue on because 'method' will undefined per 11.1.3. Then we will 11.4 ("If types includes a record type"), and return the result of that.

But, there as so many little things I may be overlooking.

[Chris Dumez]

(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 299254 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=299254&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/url/urlsearchparams-constructor-expected.txt:4
> > > +FAIL URLSearchParams constructor, DOMException.prototype as argument assert_equals: expected "Error=" but got "INDEX_SIZE_ERR=1&DOMSTRING_SIZE_ERR=2&HIERARCHY_REQUEST_ERR=3&WRONG_DOCUMENT_ERR=4&INVALID_CHARACTER_ERR=5&NO_DATA_ALLOWED_ERR=6&NO_MODIFICATION_ALLOWED_ERR=7&NOT_FOUND_ERR=8&NOT_SUPPORTED_ERR=9&INUSE_ATTRIBUTE_ERR=10&INVALID_STATE_ERR=11&SYNTAX_ERR=12&INVALID_MODIFICATION_ERR=13&NAMESPACE_ERR=14&INVALID_ACCESS_ERR=15&VALIDATION_ERR=16&TYPE_MISMATCH_ERR=17&SECURITY_ERR=18&NETWORK_ERR=19&ABORT_ERR=20&URL_MISMATCH_ERR=21&QUOTA_EXCEEDED_ERR=22&TIMEOUT_ERR=23&INVALID_NODE_TYPE_ERR=24&DATA_CLONE_ERR=25"
> > 
> > Is this remaining failure a problem caused by our Record implementation?
> 
> I don't think so.  The test states that the correct result would be
> "Error=", which would be true if passing DOMException.prototype caused the
> union conversion code to choose to convert to a String. 
> 
> But, by my reading of the WebIDL union conversion code
> (https://heycam.github.io/webidl/#es-union), we should go down step 11. ("If
> V is any kind of object"), try 11.1 ("If types includes a sequence type,
> then"), but continue on because 'method' will undefined per 11.1.3. Then we
> will 11.4 ("If types includes a record type"), and return the result of that.
> 
> But, there as so many little things I may be overlooking.

I interpret the spec the same way as Sam. I think the test is wrong.

[Anne van Kesteren]

Posted a fix for the test at https://github.com/w3c/web-platform-tests/pull/4581. It doesn't quite much your expected result as per the IDL specification there need to be less properties.

[Sam Weinig]

(In reply to comment #22)
> Posted a fix for the test at
> https://github.com/w3c/web-platform-tests/pull/4581. It doesn't quite much
> your expected result as per the IDL specification there need to be less
> properties.

Thanks Anne!

It looks like Anne also added back the legacy constants, so I think our result is correct now: https://github.com/w3c/web-platform-tests/pull/4581/commits/5add35e139eaa01966e0505e7a9fd95a99d8ef26

[Anne van Kesteren]

Issue 3 from comment 4 is now being fixed in https://github.com/w3c/web-platform-tests/pull/4647. Sorry for not noticing that issue initially.