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

[bugzilla-daemon at webkit.org]

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

--- Comment #9 from Risul Islam <risul_islam at apple.com> ---
Comment on attachment 434531
  --> https://bugs.webkit.org/attachment.cgi?id=434531
Patch

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

>> Source/WTF/wtf/URL.cpp:1177
>> +        return { };
> 
> Is this needed?

We can achieve the same functionality using query() and URLParser::parseURLEncodedForm() function. So, we can discard it.

>> Source/WTF/wtf/URL.cpp:1231
>> +bool URL::isEqualIgnoringQuery(URL otherURL) const
> 
> Do we want to compare the fragments after the query?

For now, we are only interested in anything before query. I guess we can rename the function name to 'isEqualIgnoringQueryAndFragment'?

>> Source/WTF/wtf/URL.cpp:1237
>> +    auto otherPathLength = otherURL.m_string[otherURL.m_pathEnd-1] == '/' ?  otherURL.m_pathEnd-1 : otherURL.m_pathEnd;
> 
> WebKit code style needs a space before and after the minus

On it.

>> Source/WTF/wtf/URL.cpp:1239
>> +    return m_string.substring(0, thisPathLength) == otherURL.m_string.substring(0, otherPathLength);
> 
> Use StringViews.  There's no need to allocate and copy just to compare.

On it.

>> Source/WTF/wtf/URL.cpp:1242
>> +bool URL::isQueryEqual(URL otherURL) const
> 
> This should probably be a const URL& to avoid an unnecessary copy every time this is called.
> Also, this should probably be renamed to containsSameQueryParameters or something like that to reflect the fact that query parameter order doesn't matter here.  You should also add a test showing that http://example.com/?a=b&c=d and http://example.com/?c=d&a=b are equal here.

Awesome idea, adding a test case.

>> Source/WTF/wtf/URL.cpp:1245
>> +        return { };
> 
> This should probably be true or false instead of { }, which is less clear for booleans.

Right.

>> Source/WTF/wtf/URL.cpp:1267
>> +            queryWithoutRemovalKey = queryWithoutRemovalKey + singleQueryParameter.key + "=" + singleQueryParameter.value + "&";
> 
> This should use makeString instead of operator+ to avoid all the intermediaries.

Great. That would make code clear.

>> Source/WTF/wtf/URL.cpp:1272
>> +        queryWithoutRemovalKey = queryWithoutRemovalKey.substring(0, queryWithoutRemovalKey.length() - 1);
> 
> You need to check if queryWithoutRemovalKey.length() is zero.

On it.

-- 
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/20210729/2866e467/attachment.htm>