[webkit-reviews] review denied: [Bug 228122] Add functionalities for parsing URL query string : [Attachment 434736] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Risul Islam <risul_islam at apple.com>'s
request for review:
Bug 228122: Add functionalities for parsing URL query string
https://bugs.webkit.org/show_bug.cgi?id=228122

Attachment 434736: Patch

https://bugs.webkit.org/attachment.cgi?id=434736&action=review




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

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

I appreciate the work to do a sorting-based algorithm instead of a hash table.

> Source/WTF/wtf/HashFunctions.h:182
> +    template<typename T, typename U> struct KeyValuePairHash {
> +	   static unsigned hash(const KeyValuePair<T, U>& p)
> +	   {
> +	       return pairIntHash(DefaultHash<T>::hash(p.key),
DefaultHash<U>::hash(p.value));
> +	   }
> +	   static bool equal(const KeyValuePair<T, U>& a, const KeyValuePair<T,
U>& b)
> +	   {
> +	       return DefaultHash<T>::equal(a.key, b.key) &&
DefaultHash<U>::equal(a.value, b.value);
> +	   }
> +	   static constexpr bool safeToCompareToEmptyOrDeleted =
DefaultHash<T>::safeToCompareToEmptyOrDeleted &&
DefaultHash<U>::safeToCompareToEmptyOrDeleted;
> +    };

We aren’t using a KeyValuePair as a hash table key. Can we either not do this
at all at this time, or do it in a separate patch?

> Source/WTF/wtf/HashFunctions.h:282
> +    template<typename T, typename U> struct DefaultHash<KeyValuePair<T, U>>
: KeyValuePairHash<T, U> { };

We aren’t using a KeyValuePair as a hash table key. Can we either not do this
at all at this time, or do it in a separate patch?

> Source/WTF/wtf/HashTraits.h:431
> +using WTF::KeyValuePairHashTraits;

We aren’t using a KeyValuePair as a hash table key. Can we either not do this
at all at this time, or do it in a separate patch?

> 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;
> +    }

I’d like to see this be done as a free function, after the class definition:

    template<typename KeyType, typename ValueType> constexpr bool
operator=(const KeyValuePair<KeyType, ValueType>& a, const
KeyValuePair<KeyType, ValueType>& b)
    {
	return a.key == b.key && a.value == b.value;
    }

But also, this change is no longer needed in this patch. We aren’t using a
KeyValuePair as a hash table key. Can we either not do this at all at this
time, or do it in a separate patch?

> Source/WTF/wtf/URL.cpp:1189
> +	   StringBuilder firstKeyValuePairAppended;
> +	   StringBuilder secondKeyValuePairAppended;
> +	   firstKeyValuePairAppended.append(firstKeyValuePair.key,
firstKeyValuePair.value);
> +	   secondKeyValuePairAppended.append(secondKeyValuePair.key,
secondKeyValuePair.value);
> +	   return firstKeyValuePairAppended.toString().utf8() <
secondKeyValuePairAppended.toString().utf8();

1) There is no reason to repeat this lambda twice! We can put it in a local
variable and use it twice. Also, we repeat the comparison below, and should
reuse for that too.

2) Concatenating strings using StringBuilder, then converting to UTF-8 is a
super-expensive way to compare and makes this unnecessarily inefficient. Seems
unlikely the sorting order matters at all:

Code should be more like this:

    auto compare = [&] (const KeyValuePair<String, String>& a, const
KeyValuePair<String, String>& b) {
	if (int result = codePointCompare(a.key, b.key))
	    return result;
	return codePointCompare(a.value, b.value);
    };
    auto comparesLessThan = [&] (const KeyValuePair<String, String>& a, const
KeyValuePair<String, String>& b) {
	return compare(a, b) < 0;
    };

    std::sort(firstQueryParameters.begin(), firstQueryParameters.end(),
comparesLessThan);
    std::sort(secondQueryParameters.begin(), secondQueryParameters.end(),
comparesLessThan);

> Source/WTF/wtf/URL.cpp:1219
> +	   if (tempParameterFromFirst.toString().utf8() <
tempParameterFromSecond.toString().utf8()) {
> +	      
differingQueryParameters.append(firstQueryParameters[indexInFirstQueryParameter
]);
> +	       indexInFirstQueryParameter++;
> +	   } else if (tempParameterFromSecond.toString().utf8() <
tempParameterFromFirst.toString().utf8()) {
> +	      
differingQueryParameters.append(secondQueryParameters[indexInSecondQueryParamet
er]);
> +	       indexInSecondQueryParameter++;
> +	   } else {
> +	       indexInFirstQueryParameter++;
> +	       indexInSecondQueryParameter++;
> +	   }

Same issue here, should not be building temporary strings and converting to
UTF-8 just compare. Also, since it’s important to use the same comparison
function so we do it consistently, this makes the refactoring above more
valuable.

    int comparison = compare(firstQueryParameters[indexInFirstQueryParameter],
secondQueryParameters[indexInSecondQueryParameter]);
    if (comparison < 0) {
	...
    } else if (comparison > 0) {
	...
    } else {
	...
    }

> Source/WTF/wtf/URL.cpp:1240
> +    return url.string()[url.pathEnd() - 1] == '/' ? url.pathEnd() - 1 :
url.pathEnd();

I don’t understand why this "/" removal is needed. Which test case falls if we
remove this logic?

> Source/WTF/wtf/URL.cpp:1251
> +bool isEqualIgnoringQueryAndFragments(const URL& firstURL, const URL&
secondURL)
> +{
> +    if (!firstURL.isValid() || !secondURL.isValid())
> +	   return false;
> +    
> +    auto lengthOfFirstURLIgnoringQueryAndFragments =
lengthOfURLIgnoringQueryAndFragments(firstURL);
> +    auto secondLengthOfSecondURLIgnoringQueryAndFragments =
lengthOfURLIgnoringQueryAndFragments(secondURL);
> +    return StringView(firstURL.string()).substring(0,
lengthOfFirstURLIgnoringQueryAndFragments) ==
StringView(secondURL.string()).substring(0,
secondLengthOfSecondURLIgnoringQueryAndFragments);
> +}

Seems a little peculiar that two invalid URLs always compare as unequal even if
they are identical. Is that really the behavior we want?

>From a factoring point of view, one more function and shorter argument names
can make this code much clearer and easier to read:

    static StringView substringIgnoringQueryAndFragments(const URL& url)
    {
	return StringView { url.string()
}.left(lengthOfURLIgnoringQueryAndFragments(url));
    }

    bool isEqualIgnoringQueryAndFragments(const URL& a, const URL& b)
    {
	return a.isValid() && b.isValid() &&
substringIgnoringQueryAndFragments(a) == substringIgnoringQueryAndFragments(b);
    }