[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 #2 from John Wilander <wilander 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

Good work. I'll start with comments on the meat of your patch.

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

> Source/WTF/wtf/URL.cpp:1172
> +    Vector<QueryParameter> queryParametersKeyValues;

This can be declared after the early return.

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

> Source/WTF/wtf/URL.cpp:1185
> +                tempKeyValues.setValue("");

This may able too use emptyString.

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

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

> Source/WTF/wtf/URL.cpp:1212
> +        return diffResult;

You can probably return { } here and push the declaration of diffResult further down.

> Source/WTF/wtf/URL.cpp:1215
> +        return matchingQueryParameters;

Ditto.

> Source/WTF/wtf/URL.cpp:1218
> +        return thisQueryParameters;

Ditto.

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

> Source/WTF/wtf/URL.cpp:1227
> +    

Here's where you should declare diffResult.

> Source/WTF/wtf/URL.cpp:1249
> +std::optional<bool> URL::isEqualQuery(URL matchingURL) const

I'd go with isQueryEqual().

> Source/WTF/wtf/URL.cpp:1254
> +    std::optional<Vector<QueryParameter>> difference = differenceInQueryParameters(matchingURL);

Use auto.

> Source/WTF/wtf/URL.cpp:1255
> +    if (!difference.value().size())

I think this may fail if you get a nullopt. Regardless, you can just do:
return !difference || !difference->size();

> Source/WTF/wtf/URL.cpp:1267
> +    unsigned thisPathLength = m_string[m_pathEnd-1] == '/' ? m_pathEnd-1 : m_pathEnd;

Use auto.

> Source/WTF/wtf/URL.cpp:1268
> +    unsigned matchingPathLength = matchingURL.m_string[matchingURL.m_pathEnd-1] == '/' ?  matchingURL.m_pathEnd-1 : matchingURL.m_pathEnd;

Ditto.

> Source/WTF/wtf/URL.cpp:1269
> +    String thisURLWithoutQuery = m_string.substring(0, thisPathLength);

Ditto.

> Source/WTF/wtf/URL.cpp:1270
> +    String matchingURLWithoutQuery = matchingURL.m_string.substring(0, matchingPathLength);

Ditto.

> Source/WTF/wtf/URL.cpp:1272
> +    return thisURLWithoutQuery == matchingURLWithoutQuery;

Can we do this without creating two new String objects?

-- 
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/20210726/437c2cde/attachment.htm>