[webkit-reviews] review granted: [Bug 228122] Add functionalities for parsing URL query string : [Attachment 434848] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 3 13:08:29 PDT 2021


Darin Adler <darin at apple.com> has granted Risul Islam <risul_islam at apple.com>'s
request for review:
Bug 228122: Add functionalities for parsing URL query string
https://bugs.webkit.org/show_bug.cgi?id=228122

Attachment 434848: Patch

https://bugs.webkit.org/attachment.cgi?id=434848&action=review




--- Comment #30 from Darin Adler <darin at apple.com> ---
Comment on attachment 434848
  --> https://bugs.webkit.org/attachment.cgi?id=434848
Patch

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

This seems good, but there is still room for improvement.

> Source/WTF/ChangeLog:3
> +	   Add functionalities for parsing URL query string

I think we can call these "functions"; don’t need to say "functionalities".

> Source/WTF/wtf/URL.cpp:1173
> +    if (!firstURL.isValid() || !secondURL.isValid())
> +	   return { };

Not 100% sure this is the best behavior if only one of the URLs is invalid.
Depends on how we intend to use these functions.

Tests should cover this behavior.

> Source/WTF/wtf/URL.cpp:1190
> +    auto comparesLessThan = [&compare] (const KeyValuePair<String, String>&
a, const KeyValuePair<String, String>& b) {

I suspect just [compare] would be fine, don’t necessarily need [&compare].

> Source/WTF/wtf/URL.cpp:1236
> +static StringView substringIgnoringQueryAndFragments(const URL& url)
> +{
> +    return StringView(url.string()).left(url.pathEnd());
> +}
> +
> +bool isEqualIgnoringQueryAndFragments(const URL& a, const URL& b)
> +{
> +    return a.isValid() && b.isValid() &&
substringIgnoringQueryAndFragments(a) == substringIgnoringQueryAndFragments(b);
> +}

This comment of mine may have been lost in the shuffle, I did not see a
response:

Why do we want to always return false if one of the two URLs is invalid?

That means that an invalid URL would compare as not equal to itself. Is that
the best behavior for this function? Maybe if one of the URLs is invalid we
should simply compare the two strings, not ignoring anything? If so, we could
implement that behavior by altering substringIgnoringQueryAndFragments to
return the entire string when the URL is invalid.

It’s hard to judge something like this when we are just adding a function
without adding any uses of it.

And I don’t see test cases that cover this behavior; we need to test such edge
cases.

> Source/WTF/wtf/URL.cpp:1243
> +    auto queryParametersList = URLParser::parseURLEncodedForm(url.query());

This local variable is not needed. Could put this expression inside the for
loop.

    for (auto& parameter : URLParser::parseURLEncodedForm(url.query())) {
	...
    }

> Source/WTF/wtf/URL.cpp:1245
> +    for (auto& singleQueryParameter : queryParametersList) {

Given the context, I think this could be called parameter instead of having a
3-word name.

> Source/WTF/wtf/URL.cpp:1250
> +	       if (!queryWithoutRemovalKeys.isEmpty())
> +		   queryWithoutRemovalKeys.append('&',
singleQueryParameter.key, '=', singleQueryParameter.value);
> +	       else
> +		   queryWithoutRemovalKeys.append(singleQueryParameter.key,
'=', singleQueryParameter.value);

Another way to write this:

    queryWithoutRemovalKeys.append(queryWithoutRemovalKeys.isEmpty() ? "" :
"&", singleQueryParameter.key, '=', singleQueryParameter.value);

Or:

    auto separator = queryWithoutRemovalKeys.isEmpty() ? "" : "&";
    queryWithoutRemovalKeys.append(separator singleQueryParameter.key, '=',
singleQueryParameter.value);

Not sure that either of my ways are better, but they are less repetitive.

> Source/WTF/wtf/URL.cpp:1254
> +    url.setQuery(queryWithoutRemovalKeys);

I am surprised this compiles. I would have thought we’d have to call one of the
toString functions on the StringBuilder.

> Tools/TestWebKitAPI/Tests/WTF/URL.cpp:362
> +TEST_F(WTF_URL, URLDifferingQueryParameters)

I don’t think we have enough test cases. For example, all the keys are sorted
in our tests; we aren’t testing the cases where the order isn’t right. So if
our sort algorithm was broken we’d never know.


More information about the webkit-reviews mailing list