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.
(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.
It seems like it's not, there's something broken with mirroring.
It's updated now.
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?
Created attachment 299064 [details] Patch
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.
Comment on attachment 299064 [details] Patch Can this use ConstructorMayThrowException?
We probably want to change our record implementation to be a Vector of pairs to preserve order.
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.
Created attachment 299206 [details] Patch
(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 :/
I filed and implemented the change to record in https://bugs.webkit.org/show_bug.cgi?id=167189.
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?
Created attachment 299254 [details] Patch
(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.
(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 :).
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.
http://trac.webkit.org/r210946
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"A_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?
(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"A_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.
(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"A_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.
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.
(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
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.