[webkit-reviews] review denied: [Bug 228122] Expand URL class query parameter functions : [Attachment 434642] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Risul Islam
<risul_islam at apple.com>'s request for review:
Bug 228122: Expand URL class query parameter functions
https://bugs.webkit.org/show_bug.cgi?id=228122

Attachment 434642: Patch

https://bugs.webkit.org/attachment.cgi?id=434642&action=review




--- Comment #11 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 434642
  --> https://bugs.webkit.org/attachment.cgi?id=434642
Patch

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

Needs a bit more refinement.  Also, your tests are missing queries with keys
but no values, like http://example.com/?a=b&c&d=e

> Source/WTF/wtf/KeyValuePair.h:62
> +    

We don't want to add empty whitespace here.

> Source/WTF/wtf/URL.cpp:1172
> +    

Extra space.

> Source/WTF/wtf/URL.cpp:1179
> +    if (thisQueryParameters.isEmpty() && otherQueryParameters.isEmpty())
> +	   return { };

This is completely unnecessary,  doesn't change any behavior, but does increase
complexity of the code.  Please remove.

> Source/WTF/wtf/URL.cpp:1182
> +    if (thisQueryParameters.isEmpty())
> +	   return otherQueryParameters;

Probably also true with these, although these might be slight performance
optimizations.

> Source/WTF/wtf/URL.cpp:1188
> +    HashMap<KeyValuePair<String, String>, int> frequencyOfDistinctParameter;

Does the value of this map need to be signed?  Could int be changed to unsigned
or size_t?

> Source/WTF/wtf/URL.cpp:1193
> +	   if (!frequencyOfDistinctParameter.contains(keyValue))
> +	       frequencyOfDistinctParameter.add(keyValue, 1);
> +	   else
> +	       frequencyOfDistinctParameter.set(keyValue,
frequencyOfDistinctParameter.get(keyValue) + 1);

This does up to 3 hash lookups when only one is necessary.  Use
HashMap::add(keyValue, 1) and look at the return value.  If
addResult.isNewEntry is true, then you're done.  Otherwise, use
addResult.iterator to increment the value.

> Source/WTF/wtf/URL.cpp:1203
> +		   frequencyOfDistinctParameter.set(singleQueryParameter,
frequencyOfDistinctParameter.get(singleQueryParameter) - 1);

Ditto with the duplicate hash lookups.

> Source/WTF/wtf/URL.cpp:1209
> +	       for (int i = 0; i <
frequencyOfDistinctParameter.get(singleQueryParameter); i++)

This does a get every loop iteration, when it should always return the same
value, and I'm pretty sure the compiler optimizer doesn't realize it.  Store
the value outside the for loop.

> Source/WTF/wtf/URL.cpp:1212
> +	       frequencyOfDistinctParameter.set(singleQueryParameter, 0);

This is also a duplicate hash lookup.  Use HashMap::take.

> Source/WTF/wtf/URL.cpp:1224
> +    auto thisPathLength = m_string[m_pathEnd - 1] == '/' ? m_pathEnd - 1 :
m_pathEnd;

I don't think this does what you think it does.  You have a test with
"http://www.webkit.org?" but when parsed and canonicalized it becomes
"http://www.webkit.org/?". Add a test with a zero length path, like
custom-scheme://host?a=b and http://example.com/?a=b

> Source/WTF/wtf/URL.cpp:1225
> +    auto otherPathLength = otherURL.m_string[otherURL.m_pathEnd - 1] == '/'
?  otherURL.m_pathEnd - 1 : otherURL.m_pathEnd;

Extra space.

> Source/WTF/wtf/URL.cpp:1230
> +bool URL::containsSameQueryParameters(const URL& otherURL) const

Do we want http://example.com/ and http://example.com/? to be considered to
have the same query parameters?  Add that test.

> Source/WTF/wtf/URL.cpp:1239
> +    if (queryDifference.isEmpty())
> +	   return true;
> +    
> +    return false;

return queryDifference.isEmpty();

> Source/WTF/wtf/URL.cpp:1249
> +    if (queryParametersList.isEmpty())
> +	   return;

This is almost certainly unnecessary.

> Source/WTF/wtf/URL.cpp:1254
> +	       queryWithoutRemovalKeys = makeString(queryWithoutRemovalKeys,
singleQueryParameter.key, '=', singleQueryParameter.value, '&');

This. should actually be a StringBuilder instead.  It's more efficient than
this, which is currently an O(n^2) algorithm because of all the recopying of
the current String.

> Source/WTF/wtf/URL.cpp:1255
> +	   

Extra space.

> Source/WTF/wtf/URL.cpp:1259
> +	   queryWithoutRemovalKeys = queryWithoutRemovalKeys.substring(0,
queryWithoutRemovalKeys.length() - 1);

This is also a wasteful String allocation and copy.  This could either be done
with a StringView substring, which is just an integer subtraction, or by
checking if queryWithoutRemovalKeys is empty in the loop and adding '&' before
the new values if not.