Fix http://w3c-test.org/url/url-setters.html failures |
||||||
Issue descriptionSee issue 651770 for the context
,
Feb 6 2017
url = new URL('https://example.com/');
url.search = '??hoge'
We expect url.search returns '??hoge' but it actually returns '?hoge'. The reason is that both DOMURLUtils::setSearchInternal and KURL::setQuery called in it trim off the leading question character.
The former was added in Sep 2014 by sof[1]. The latter was added before that. IIUC the former is not needed at all.
sof@, can you tell me if my understanding is correct?
1: https://chromium.googlesource.com/chromium/src/+/4db1e7203ea51d375f11835c2ad56de335684d6a
,
Feb 6 2017
Not sure how it ended up like this in setSearchInternal(), https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUtils.cpp#newcode126 suggests some more invasive fixup of the input was needed at the time. But if it aligns with the spec to drop the special case, then we should definitely try.
,
Feb 8 2017
Thank you. I uploaded https://codereview.chromium.org/2686653003/.
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39 commit 29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39 Author: yhirano <yhirano@chromium.org> Date: Wed Feb 08 10:03:30 2017 URL.search should return the set value via setSearch DOMURLUtils and KURL trim off the leading question mark independently, which leads to trimming off two leading questions in addition. DOMURLUtilsReadOnly::search recovers one question mark and we lose one question mark in total. The special handling in DOMURLUtils is needed because KURL::setQuery handles an empty string and a null string differently. KURL::setQuery("?") is equivalent to KURL::setQuery("") but what we need is KURL::setQuery(String()). This CL fixes the problem by calling KURL::setQuery(String()) if the given value is "?", and calling KURL::setQuery with the given string if it starts with a question mark and consists of two or more characters. BUG= 682150 Review-Url: https://codereview.chromium.org/2686653003 Cr-Commit-Position: refs/heads/master@{#448940} [modify] https://crrev.com/29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39/third_party/WebKit/LayoutTests/fast/domurl/url-search.html [modify] https://crrev.com/29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39/third_party/WebKit/Source/core/dom/DOMURLUtils.cpp
,
Feb 8 2017
,
Apr 4 2017
We now fail when setting .search = '?' Fail URL: Setting <https://example.net?lang=en-US#nav>.search = '?' assert_equals: expected "https://example.net/?#nav" but got "https://example.net/#nav" Fail <a>: Setting <https://example.net?lang=en-US#nav>.search = '?' assert_equals: expected "https://example.net/?#nav" but got "https://example.net/#nav" Fail <area>: Setting <https://example.net?lang=en-US#nav>.search = '?' assert_equals: expected "https://example.net/?#nav" but got "https://example.net/#nav" Is that a regression from the above change, or was it like that before? I get 299 failures on http://web-platform.test:8000/url/url-setters.html using Google Chrome 59.0.3062.0 (Official Build) canary (64-bit) Revision fb5c907d801f5fb0d6542c14f29c5749ae78abc7-refs/heads/master@{#461587} (I'm happy to report a new issue if you like, let me know.)
,
Jul 6 2017
There are still 299 failures for this file on Linux and 326 on Windows. URL on Windows attempts to be smart about Windows drive letters which leads to more failures. Should I reopen this issue or open a new issue for the remaining failures?
,
Jul 6 2017
> #7 It's not a regression. Even if I reverted the change I see the failure. The test succeeds with Firefox and Edge, so I think we should reopen the issue.
,
Jul 6 2017
Correction: The first one fails on Edge. The second and third ones pass on Edge and Firefox.
,
Jul 6
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 9
Let's close this bug as Fixed though there are still assertion failures on url-setters.html. This was not for all the assertions in the test. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by yhirano@chromium.org
, Feb 6 2017