[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 #434642|review?                     |review-
              Flags|                            |

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

-- 
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/20210730/0d60b17f/attachment.htm>