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

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com

--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 434669
  --> https://bugs.webkit.org/attachment.cgi?id=434669
Patch

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

> Source/WTF/ChangeLog:8
> +        Added more functionalities for parsing URL query parameters.

I don’t fully understand why adding more functions that are not yet used is important for WebKIt, but I trust there is some reason.

> Source/WTF/wtf/KeyValuePair.h:67
> +    template <typename K, typename V>
> +    bool operator==(const KeyValuePair<K, V>& other) const
> +    {
> +        return key == other.key && value == other.value;
> +    }

This doesn’t need to be a member. Making it a free function that takes two arguments is a little better in terms of supporting conversions on the left side of the "==" so I think we might prefer that.

> Source/WTF/wtf/URL.cpp:1176
> +    auto thisQueryParameters = URLParser::parseURLEncodedForm(query());
> +    auto otherQueryParameters = URLParser::parseURLEncodedForm(otherURL.query());

It seems to me would could implement this operation significantly more efficiently by sorting both vectors and walking through them, rather than using a hash table.

> Source/WTF/wtf/URL.cpp:1185
> +    HashMap<KeyValuePair<String, String>, unsigned> frequencyOfDistinctParameters;

We have a data structure for this called HashCountedSet. Using that would simplify the code below.

> Source/WTF/wtf/URL.cpp:1205
> +        auto frequencyOfsingleQueryParameter = frequencyOfDistinctParameters.take(singleQueryParameter);

Lowercase "s" here.

> Source/WTF/wtf/URL.cpp:1222
> +    auto thisLengthOfURLIgnoringQueryAndFragments = m_string[m_pathEnd - 1] == '/' ? m_pathEnd - 1 : m_pathEnd;
> +    auto otherLengthOfURLIgnoringQueryAndFragments = otherURL.m_string[otherURL.m_pathEnd - 1] == '/' ?  otherURL.m_pathEnd - 1 : otherURL.m_pathEnd;
> +    return StringView(m_string).substring(0, thisLengthOfURLIgnoringQueryAndFragments) == StringView(otherURL.m_string).substring(0, otherLengthOfURLIgnoringQueryAndFragments);

Seems like we should use a helper function rather than repeating this logic twice. Could be private or local to this file.

> Source/WTF/wtf/URL.cpp:1231
> +    auto queryDifference = differingQueryParameters(otherURL);
> +    return queryDifference.isEmpty();

No need for a local variable here.

This seems like an inefficient algorithm; if we were using this somewhere performance critical we’d want to do this in a way that doesn’t allocate so much memory.

> Source/WTF/wtf/URL.cpp:1242
> +        if (keysToRemove.find(singleQueryParameter.key) == keysToRemove.end()) {

The best idiom here is to call contains, rather than find == end.

> Source/WTF/wtf/URL.cpp:1244
> +                queryWithoutRemovalKeys = makeString(queryWithoutRemovalKeys, '&',  singleQueryParameter.key, '=', singleQueryParameter.value);

This is not an efficient way to build up a string. Concatenating each time means lots of memory allocation. We have a class named StringBuilder that implements this kind of idiom efficiently.

> Source/WTF/wtf/URL.cpp:1250
> +    setQuery(StringView(queryWithoutRemovalKeys));

There should not be a need to call StringView() here explicitly.

> Source/WTF/wtf/URL.h:207
> +    WTF_EXPORT_PRIVATE Vector<KeyValuePair<String, String>> differingQueryParameters(const URL&) const;
> +    WTF_EXPORT_PRIVATE bool isEqualIgnoringQueryAndFragments(const URL&) const;
> +    WTF_EXPORT_PRIVATE bool containsSameQueryParameters(const URL&) const;
> +    WTF_EXPORT_PRIVATE void removeQueryParameters(const HashSet<String>&);

How did you decide that these should be members of the URL class rather than functions that take a URL? In particular, the ones that take two URLs seem like they should be functions that take two URLs.

-- 
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/77f2f670/attachment-0001.htm>