[webkit-reviews] review denied: [Bug 228122] Expand URL class query parameter functions : [Attachment 434531] 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 434531: Patch

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




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

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

> Source/WTF/wtf/URL.cpp:1177
> +    if (queryPart.isEmpty())
> +	   return { };

Is this needed?

> Source/WTF/wtf/URL.cpp:1231
> +bool URL::isEqualIgnoringQuery(URL otherURL) const

Do we want to compare the fragments after the query?

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

WebKit code style needs a space before and after the minus

> Source/WTF/wtf/URL.cpp:1239
> +    return m_string.substring(0, thisPathLength) ==
otherURL.m_string.substring(0, otherPathLength);

Use StringViews.  There's no need to allocate and copy just to compare.

> Source/WTF/wtf/URL.cpp:1242
> +bool URL::isQueryEqual(URL otherURL) const

This should probably be a const URL& to avoid an unnecessary copy every time
this is called.
Also, this should probably be renamed to containsSameQueryParameters or
something like that to reflect the fact that query parameter order doesn't
matter here.  You should also add a test showing that
http://example.com/?a=b&c=d and http://example.com/?c=d&a=b are equal here.

> Source/WTF/wtf/URL.cpp:1245
> +	   return { };

This should probably be true or false instead of { }, which is less clear for
booleans.

> Source/WTF/wtf/URL.cpp:1255
> +void URL::removeQueryParameters(HashSet<String> keysToRemove)

ditto.	const reference.

> Source/WTF/wtf/URL.cpp:1267
> +	       queryWithoutRemovalKey = queryWithoutRemovalKey +
singleQueryParameter.key + "=" + singleQueryParameter.value + "&";

This should use makeString instead of operator+ to avoid all the
intermediaries.

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

You need to check if queryWithoutRemovalKey.length() is zero.