[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 #16 from Risul Islam <risul_islam 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.

Thank you Darin for your valuable comments and insights. We are trying to come up with a feature that restricts tracking parameters from the URL while loading or sharing by copying. Thats why we are adding the functionalities in this patch.

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

Wouldn't sorting be costly in terms of time? We preferred to sacrifice some memory. Although the length of the URL and the number of the parameter is not that huge, still we can discuss about any kind of optimization.

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

Great idea, on to it.

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

Sorry for the mistake. Fixing it.

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

We can write a static function in this file.

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

I agree. we can directly use: return differingQueryParameters(otherURL).isEmpty(); Even discard the function.

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

On to it.

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

Using StringBuilder now.

>> Source/WTF/wtf/URL.cpp:1250
>> +    setQuery(StringView(queryWithoutRemovalKeys));
> 
> There should not be a need to call StringView() here explicitly.

setQuery(StringView) takes StringView as argument. Will it type cast to StringView automatically if we provide String?

>> Source/WTF/wtf/URL.h:207
>> +    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.

I am sorry I am a little bit unclear here. It would be very helpful if I could be a bit clear.

-- 
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/20210731/06a403b6/attachment-0001.htm>