[webkit-reviews] review denied: [Bug 228122] Expand URL class query parameter functions : [Attachment 434642] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 30 11:31:48 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 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.
More information about the webkit-reviews
mailing list