[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 #6 from Risul Islam <risul_islam at apple.com> ---
Comment on attachment 434214
  --> https://bugs.webkit.org/attachment.cgi?id=434214
Patch

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

>> Source/WTF/wtf/URL.cpp:1169
>> +std::optional<Vector<URL::QueryParameter>> URL::queryParameters() const
> 
> I wonder if this could return an empty vector for an invalid URL instead of being optional.

That would be good. Done.

>>> Source/WTF/wtf/URL.cpp:1171
>>> +    // Assumption 1: Parameters are seperated by '&' although ';' is also possible and ';' is not handled here.
>> 
>> You don't seem to have further assumptions so the number is superfluous. I'd go with "Assumes that parameters are …" if we're keeping this comment.
>> 
>> I would prefer a reference to a spec though. https://datatracker.ietf.org/doc/html/rfc3986/#section-3.4 is a good starting point. You'd have to find some reference to the use of semicolons.
> 
> I agree with John about removing the number. I don't really think a comment is necessary, it is quite clear in the code that parameters are being split by &.

on it.

>> Source/WTF/wtf/URL.cpp:1172
>> +    Vector<QueryParameter> queryParametersKeyValues;
> 
> This can be declared after the early return.

Agreed. Done.

>> Source/WTF/wtf/URL.cpp:1176
>> +    String queryPart = m_queryEnd == m_pathEnd ? "" : m_string.substring(m_pathEnd + 1, m_queryEnd - (m_pathEnd + 1));
> 
> Can you call query() here to avoid code duplication?

yes. on it. Done.

>> Source/WTF/wtf/URL.cpp:1178
>> +        return queryParametersKeyValues;
> 
> I think this can return an { } instead and you can push the declaration of queryParametersKeyValues even further down.

yes. Done.

>> Source/WTF/wtf/URL.cpp:1180
>> +    for (String keyValuePair : queryPart.splitAllowingEmptyEntries('&')) {
> 
> You can change String to be auto, it is shorter.

Done using "auto" whenever applicable.

>> Source/WTF/wtf/URL.cpp:1185
>> +                tempKeyValues.setValue("");
> 
> This may able too use emptyString.

Used another function (URLParser::parseURLEncodedForm) suggested by Alex.

>>> Source/WTF/wtf/URL.cpp:1199
>>> +std::optional<Vector<URL::QueryParameter>> URL::differenceInQueryParameters(URL matchingURL) const
>> 
>> I would call this function differingQueryParameters() since that sounds more like the getter it is.
> 
> matchingURL sounds like the URLs are the same. I would also change it to be something else, like otherURL.

Done using other and differingQueryParameters.

>> Source/WTF/wtf/URL.cpp:1205
>> +    std::optional<Vector<QueryParameter>> matchingQueryParameters = matchingURL.queryParameters();
> 
> Ditto, I would change this to be otherQueryParameters. You can also use auto for both of these.

yes.

>> Source/WTF/wtf/URL.cpp:1207
>> +    if (!thisQueryParameters || !matchingQueryParameters)
> 
> I would write these as two early returns to not have to do all the work upfront. Like this:
> if (!thisQueryParameters = queryParameters())
>     return std::nullopt;
> if (!matchingQueryParameters = matchingURL.queryParameters())
>     return std::nullopt;

sure. btw, I really like this code style.

>> Source/WTF/wtf/URL.cpp:1210
>> +    Vector<QueryParameter> diffResult;
> 
> I would avoid abbreviations as much as possible (that is a part of our style guidelines), so I would recommend changing this vector name.

it would be good. Done.

>> Source/WTF/wtf/URL.cpp:1211
>> +    if (!thisQueryParameters.value().size() && !matchingQueryParameters.value().size())
> 
> Vector has an isEmpty() function that you can use here.

Done using isEmpty() whenever possible.

>> Source/WTF/wtf/URL.cpp:1220
>> +    HashMap<URL::QueryParameter, int> frequencyOfParameters;
> 
> The purpose of this code is unclear. I wonder if we can find a more verbose variable name?

Used frequencyOfDistinctParameter

>> Source/WTF/wtf/URL.cpp:1228
>> +    for (QueryParameter keyValue : matchingQueryParameters.value()) {
> 
> Ditto about auto& and the keyValue naming.

Used singleQueryParameter instead of keyValue.

>> Source/WTF/wtf/URL.cpp:1240
>> +        if (frequencyOfParameters.get(keyvalue) > 0) {
> 
> No need for > 0, you can just check if (frequencyOfParameters.get(keyvalue)

Here, frequencyOfParameters.get(keyvalue) can also be negative. Rearranging the code so that it stays non-negative.

>> Source/WTF/wtf/URL.cpp:1246
>> +    return diffResult;
> 
> This does not address query parameter ordering. should we consider ?key1=value1&key2=value2 the same as ?key2=value2&key1=value1 ?

Addressed this issue. Now ensures ordering.

>> Source/WTF/wtf/URL.cpp:1252
>> +        return std::nullopt;
> 
> I don't think this should return an optional. I think if either or both URLs are invalid return false.

Done.

>>> Source/WTF/wtf/URL.cpp:1269
>>> +    String thisURLWithoutQuery = m_string.substring(0, thisPathLength);
>> 
>> Ditto.
> 
> You don't need to allocate and copy the substrings just to check if they are equal.  Use StringView.

on it.

>> Source/WTF/wtf/URL.cpp:1272
>> +    return thisURLWithoutQuery == matchingURLWithoutQuery;
> 
> Can we do this without creating two new String objects?

Done.

>> Source/WTF/wtf/URL.h:229
>> +    WTF_EXPORT_PRIVATE std::optional<Vector<QueryParameter>> queryParameters() const;
> 
> This should probably just use URLParser::parseURLEncodedForm instead of adding this.  That also returns a URLEncodedForm, which basically makes your QueryParameter class unnecessary.

I am onto it. Done. Used the URLParser::parseURLEncodedForm. Works just the same and fine.

-- 
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/20210728/630401c2/attachment-0001.htm>