[webkit-reviews] review denied: [Bug 228122] Expand URL class query parameter functions : [Attachment 434531] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 29 10:39:44 PDT 2021
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.
More information about the webkit-reviews
mailing list