Bug 46610 – Implement getParameter from the URL API

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Implement getParameter from the URL API

[Adam Barth]

Created attachment 68884 [details]
Patch

[WebKit Review Bot]

Attachment 68884 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4005163

[Adam Barth]

Hum...  Looks like I need a GURL implementation of the KURL part of this change.

[Adam Barth]

Created attachment 69017 [details]
Patch

[Adam Barth]

Duplicated code for GURL => sadness.

[Daniel Bates]

(In reply to comment #5)
> Duplicated code for GURL => sadness.

It seems the two implementations of copyParsedQueryTo() simply differ in their initialization of pos and end.

Could we use a similar approach to ScriptControllerBase for KURL? That is, add a header and implementation file called KURLBase.h and KURLBase.cpp and extract the common code from the KURL and KURLGoogle files into a private function, say copyParsedQueryTo(ParsedURLParameters&, const UChar* startPosOfQueryString, const UChar* endPosOfQueryString). Then have KURL.h include KURLBase.h and have each implementation of the one-argument public variant of copyParsedQueryTo call the common private routine.

[Daniel Bates]

Comment on attachment 69017 [details]
Patch

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

> WebCore/ChangeLog:29
> +        * html/HTMLAnchorElement.cpp:
> +        (WebCore::HTMLAnchorElement::getParameter):
> +        * html/HTMLAnchorElement.h:
> +        * html/HTMLAnchorElement.idl:
> +        * page/Location.cpp:
> +        (WebCore::Location::getParameter):
> +        * page/Location.h:
> +        * page/Location.idl:
> +        * platform/KURL.cpp:
> +        (WebCore::KURL::copyParsedQueryTo):
> +        * platform/KURLGoogle.cpp:
> +        (WebCore::KURL::copyParsedQueryTo):
> +        * platform/KURL.h:

For your consideration, I would suggest using the space to the right of the ':'  in each entry above to remark on which methods were added by this patch.

> LayoutTests/fast/dom/anchor-getParameter.html:31
> +for (var i = 0; i < cases.length; ++i)
> +    document.write('<a href="' + cases[i][0] + '">Link</a><br>');

Could we hide the output of this so that the text results don't show a long list of the word "Link"? For instance, we could enclose the output within a <div id="test-container"></div>, which we remove from the DOM before the of the <script>. If you see value in keeping such a list of hyperlinks, say for inspecting when running this test by hand, then could we just hide this output when the test is run with DRT?

> LayoutTests/fast/dom/anchor-getParameter.html:38
> +</html>

Is there a reason why you chose not to use fast/js/resources/js-test-pre.js, inline the expected results, and make this a PASS/FAIL test? I see the benefit of the verbose output (i.e. A => B) and we could make the PASS/FAIL message include similar verbose output so as to add context to the PASS/FAIL text results should a human read them.

[Darin Adler]

Comment on attachment 69017 [details]
Patch

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

Thanks, looks good. I didn’t set commit-queue+ -- I’ll let you make the call on that.

> WebCore/platform/KURL.cpp:624
> +        const UChar* parameterBegin = pos;

I’d prefer the noun “start” to the verb “begin” for the name here.

> WebCore/platform/KURL.cpp:634
> +        const UChar* nameBegin = parameterBegin;

And here.

> WebCore/platform/KURL.cpp:640
> +        String name(nameBegin, equalSign - nameBegin);
> +        if (name.isEmpty())
> +            continue;

Might be slightly cleaner to do the empty check by checking if equalSign == nameBegin rather than constructing the string first.

> WebCore/platform/KURL.h:63
> +typedef WTF::HashMap<String, String> ParsedURLParameters;

No need for the WTF:: prefix here.

> WebCore/platform/KURL.h:139
> +    void copyParsedQueryTo(ParsedURLParameters& parameters) const;

No need for the argument name “parameters” here.

> WebCore/platform/KURLGoogle.cpp:556
> +    String query = m_url.componentString(m_url.m_parsed.query);
> +    const UChar* pos = query.characters();
> +    const UChar* end = query.characters() + query.length();

Too bad we have two copies of this given that almost all the code is identical.

>> LayoutTests/fast/dom/anchor-getParameter.html:38
>> +</html>
> 
> Is there a reason why you chose not to use fast/js/resources/js-test-pre.js, inline the expected results, and make this a PASS/FAIL test? I see the benefit of the verbose output (i.e. A => B) and we could make the PASS/FAIL message include similar verbose output so as to add context to the PASS/FAIL text results should a human read them.

I agree with Daniel. I’d go further and say that a script-tests style test would be even better than the test here.

[Adam Barth]

Thanks for the reviews.  I've applied all of your suggestions except converting the test to a script-test.  I'm not a big fan of the script-test framework, but I'll use it for these patches in the future.

[Adam Barth]

Created attachment 70666 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 70666 [details]
Patch for landing

Rejecting patch 70666 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
.
Files=14, Tests=304,  3 wallclock secs ( 0.10 usr  0.07 sys +  0.65 cusr  0.20 csys =  1.02 CPU)
Result: PASS
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 21249 test cases.
fast/dom/Window/window-appendages-cleared.html -> failed

Exiting early after 1 failures. 6976 tests run.
210.34s total testing time

6975 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://queues.webkit.org/results/4455017

[Adam Barth]

Committed r69749: <http://trac.webkit.org/changeset/69749>

[WebKit Review Bot]

http://trac.webkit.org/changeset/69749 might have broken Qt Linux Release
The following tests are not passing:
fast/dom/Window/window-properties.html

[Csaba Osztrogonác]

(In reply to comment #13)
> http://trac.webkit.org/changeset/69749 might have broken Qt Linux Release
> The following tests are not passing:
> fast/dom/Window/window-properties.html

Qt specific expected file updated: http://trac.webkit.org/changeset/69754

[Adam Barth]

> Qt specific expected file updated: http://trac.webkit.org/changeset/69754

Thanks!