Make URLSearchParams spec-compliant
Created attachment 289296 [details] Patch
Comment on attachment 289296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289296&action=review > Source/WebCore/html/DOMURL.cpp:94 > void DOMURL::setQuery(const String& query) Who is calling this method? I quickly grep'd and did not find any obvious call site. > Source/WebCore/html/DOMURL.cpp:98 > + m_searchParams->setContents(URLSearchParams::create(query).get()); Shouldn't this get use search() so that we get the *parsed* query from m_url? > Source/WebCore/html/DOMURL.cpp:129 > + m_searchParams = URLSearchParams::create("?" + m_url.query(), this); Can we just use search() instead of "?" + m_url.query(). Seems simpler. > Source/WebCore/html/URLSearchParams.h:47 > + void setContents(const Vector<std::pair<String, String>>& pairs) { m_pairs = pairs; } Instead of doing can we have an updateFromAssocatedURL() method that does the right thing? I don't like that we have to construct a URLSearchParam every time the URL gets updated.
Comment on attachment 289296 [details] Patch Attachment 289296 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2109124 New failing tests: imported/w3c/web-platform-tests/url/urlsearchparams-constructor.html
Created attachment 289299 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 289296 [details] Patch Attachment 289296 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2109105 New failing tests: imported/w3c/web-platform-tests/url/urlsearchparams-constructor.html
Created attachment 289300 [details] Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 289296 [details] Patch Attachment 289296 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2109268 New failing tests: imported/w3c/web-platform-tests/url/urlsearchparams-constructor.html
Created attachment 289302 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #2) > Comment on attachment 289296 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289296&action=review > > > Source/WebCore/html/DOMURL.cpp:94 > > void DOMURL::setQuery(const String& query) > > Who is calling this method? I quickly grep'd and did not find any obvious > call site. URLSearchParams::updateURL
Comment on attachment 289296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289296&action=review >>> Source/WebCore/html/DOMURL.cpp:94 >>> void DOMURL::setQuery(const String& query) >> >> Who is calling this method? I quickly grep'd and did not find any obvious call site. > > URLSearchParams::updateURL Well then isn't it super weird for URLSearchParams to call DOMURL::setQuery() and for DOMURL to update its URLSearchParams object back again? :)
And ideally without crashing...
Created attachment 289309 [details] Patch
Comment on attachment 289309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289309&action=review > Source/WebCore/html/DOMURL.cpp:97 > + if (m_searchParams) Why is this still updating search params if it is only called by search params > Source/WebCore/html/URLSearchParams.cpp:120 > +void URLSearchParams::updateFromAssociatedURL(const DOMURL& url) I don't think this should take a parameter?
(In reply to comment #13) > > Source/WebCore/html/URLSearchParams.cpp:120 > > +void URLSearchParams::updateFromAssociatedURL(const DOMURL& url) > > I don't think this should take a parameter? It's nice to not have a null check and to have an assertion.
(In reply to comment #14) > (In reply to comment #13) > > > Source/WebCore/html/URLSearchParams.cpp:120 > > > +void URLSearchParams::updateFromAssociatedURL(const DOMURL& url) > > > > I don't think this should take a parameter? > It's nice to not have a null check and to have an assertion. You do not need a null check, just an assertion.
Created attachment 289316 [details] Patch
Comment on attachment 289316 [details] Patch R=me
Comment on attachment 289316 [details] Patch Attachment 289316 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2110154 New failing tests: imported/w3c/web-platform-tests/url/urlsearchparams-constructor.html
Created attachment 289325 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 289326 [details] Patch
Comment on attachment 289326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289326&action=review > Source/WebCore/html/DOMURL.h:61 > + ~DOMURL(); We usually put the destructor up toward the top, below the creates(). > Source/WebCore/html/URLSearchParams.h:38 > + void URLDestroyed() { m_associatedURL = nullptr; } initial capital letter makes me sad. associatedURLDestroyed()? WeakPtr<>?
http://trac.webkit.org/changeset/206168
http://trac.webkit.org/changeset/206179