Summary: | Make URLSearchParams spec-compliant | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, rniwa | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=133276 | ||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2016-09-19 17:49:26 PDT
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<>? |