[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 #3 from Kate Cheney <katherine_cheney 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.

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

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

> Source/WTF/wtf/URL.cpp:1180
> +    for (String keyValuePair : queryPart.splitAllowingEmptyEntries('&')) {

You can change String to be auto, it is shorter.

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

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

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

> Source/WTF/wtf/URL.cpp:1211
> +    if (!thisQueryParameters.value().size() && !matchingQueryParameters.value().size())

Vector has an isEmpty() function that you can use here.

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

Ditto, using isEmpty() here would be easier to read.

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

Ditto.

> Source/WTF/wtf/URL.cpp:1221
> +    for (QueryParameter keyValue : thisQueryParameters.value()) {

auto& instead of QueryParameter. Also, the name "keyValue" gets confusing when you later are trying to set values in the map. I would stick to key or queryParameter instead.

> Source/WTF/wtf/URL.cpp:1222
> +        if (!frequencyOfParameters.contains(keyValue))

I *think* you can replace this if-else statement with the HashMap's ensure() function if you want, which creates a new entry if it doesn't exist, something like:

    auto& frequency = frequencyOfParameters.ensure(keyValue, []() {
        return 0;
    }).iterator->value;

    ++frequency;

> Source/WTF/wtf/URL.cpp:1228
> +    for (QueryParameter keyValue : matchingQueryParameters.value()) {

Ditto about auto& and the keyValue naming.

> Source/WTF/wtf/URL.cpp:1239
> +    for (QueryParameter keyvalue : frequencyOfParameters.keys()) {

auto&, and keyValue should have a name change or at least be camel case.

> Source/WTF/wtf/URL.cpp:1240
> +        if (frequencyOfParameters.get(keyvalue) > 0) {

No need for > 0, you can just check if (frequencyOfParameters.get(keyvalue)

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

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

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

You can use isEmpty() here.

> Source/WTF/wtf/URL.cpp:1265
> +        return std::nullopt;

I don't think this should return an optional. I think if either or both URLs are invalid return false.

-- 
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/ef79e1a8/attachment.htm>