[webkit-reviews] review granted: [Bug 46610] Implement getParameter from the URL API : [Attachment 69017] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 46610: Implement getParameter from the URL API
https://bugs.webkit.org/show_bug.cgi?id=46610

Attachment 69017: Patch
https://bugs.webkit.org/attachment.cgi?id=69017&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.