[Webkit-unassigned] [Bug 228122] Add functionalities for parsing URL query string

[bugzilla-daemon at webkit.org]

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

--- Comment #26 from Risul Islam <risul_islam 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

>> Source/WTF/wtf/KeyValuePair.h:67
>> +    }
> 
> 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?

Thank you Darin for your valuable comments. All of your suggestions are great learning for me.
I have removed the hash related codes. But removing this == operator overloading causes error in our test functions, specifically in EXPECT_EQ(differingQueryParameters(url1, url2), resultVector);
Any workaround suggestion?

>>> Source/WTF/wtf/URL.cpp:1189
>>> +        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);
> 
> By the way, the capture here is wrong. This would be better:
> 
>     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;
>     };

Wow, elegant code.

>> Source/WTF/wtf/URL.cpp:1219
>> +        }
> 
> 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 {
>         ...
>     }

Great idea. Code reuse at its best.

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

Thank you and Alex for this suggestion. Canonicalization removes the requirement of this log. Discarding it.

>> Source/WTF/wtf/URL.cpp:1251
>> +}
> 
> 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);
>     }

Done.

-- 
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/20210803/a8cb1a62/attachment-0001.htm>