[Webkit-unassigned] [Bug 228122] Expand URL class query parameter functions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=228122

Alex Christensen <achristensen at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #434531|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210729/77966371/attachment.htm>