Bug 161920 – Implement URLSearchParams

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Implement URLSearchParams

[Alex Christensen]

Created attachment 288707 [details]
Patch

[Chris Dumez]

Comment on attachment 288707 [details]
Patch

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

> Source/WebCore/html/URLSearchParams.cpp:76
> +            m_pairs.removeAllMatching([&] (const std::pair<String, String>& pair) {

This can use auto&

> Source/WebCore/html/URLSearchParams.cpp:110
> +    if (m_pairs.removeAllMatching([&] (const std::pair<String, String>& pair) { return pair.first == name; }))

This can use auto&

[Alex Christensen]

Created attachment 288713 [details]
Patch

[Chris Dumez]

Comment on attachment 288713 [details]
Patch

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

More comments to come.

> Source/WebCore/html/DOMURL.cpp:118
> +    return URLSearchParams::create(m_url.query(), this);

I think creating a new URLSearchParam every time is likely not right.

Consider the following:
u = new URL("http://www.google.com?q=lol");
u.searchParams === u.searchParams

This returns true on Firefox and Chrome but I think it will return false with your patch. (please add a test)

The spec says:
"A URL object has an associated url (a URL) and query object (a URLSearchParams object)."
-> https://url.spec.whatwg.org/#concept-url-query-object

So I think there should a single associated URLSearchParams object.

I think a simple way to fix this may be to use [CachedAttribute] on the attribute in the IDL although I have verified it works.

> Source/WebCore/html/URLSearchParams.cpp:37
> +    if (init.startsWith('?'))

This could be on one like using a ternary and in the initializer list.

> Source/WebCore/html/URLSearchParams.cpp:70
> +        if (pair.first == name) {

I would:
if (pair.first != name)
   continue;

to reduce nesting.

> Source/WebCore/html/URLSearchParams.cpp:127
> +    notImplemented();

I discourage having iterable<USVString, USVString>; in the IDL unless it fully works. We have broken sites like this in the past because they detect the type is iterable and then use the iterator if they detect we support it. Sites usually have a fallback path that works fine if the type is not iterable.

> Source/WebCore/html/URLSearchParams.h:54
> +    URLSearchParams(const URLSearchParams&);

Where is this copy constructor useful? It is kind of weird to have a RefCounted type with a copy constructor..

> Source/WebCore/html/URLSearchParams.h:55
> +    RefPtr<DOMURL> m_associatedURL;

I would add a blank line between the methods and the data members.

> Source/WebCore/html/URLSearchParams.idl:39
> +    iterable<USVString, USVString>;

I think we should omit this one unless it fully works as intended.

> Source/WebCore/platform/URLParser.cpp:1414
> +template<typename CharacterType>

Why is this templated if it only works for LChar*? We can cast char* to LChar* if necessary ( we do this in other places). We normally templatize only if we need to support LChar* and Char*.

[Chris Dumez]

Comment on attachment 288713 [details]
Patch

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

> Source/WebCore/html/URLSearchParams.cpp:44
> +    : m_associatedURL(other.m_associatedURL)

This is not as per spec, is it? The spec only says to copy the list, not the associated URL.

>> Source/WebCore/html/URLSearchParams.h:54
>> +    URLSearchParams(const URLSearchParams&);
> 
> Where is this copy constructor useful? It is kind of weird to have a RefCounted type with a copy constructor..

I see now that is is used. However, I'd prefer we use a constructor that takes a const Vector<std::pair<String, String>>& instead.

> Source/WebCore/platform/URLParser.cpp:1588
> +static Optional<String> formURLDecode(const String& input)

Looks like this should use StringView as input

> Source/WebCore/platform/URLParser.cpp:1604
> +auto URLParser::parseURLEncodedForm(const String& input) -> URLEncodedForm

Looks like this should use StringView as input

> Source/WebCore/platform/URLParser.cpp:1617
> +            auto name = formURLDecode(bytes.substring(0, valueStart));

Looks like this should use StringView

> Source/WebCore/platform/URLParser.cpp:1618
> +            auto value = formURLDecode(bytes.substring(valueStart + 1));

Looks like this should use StringView

> LayoutTests/imported/w3c/web-platform-tests/url/url-constructor-expected.txt:2
> +FAIL URL.searchParams getter assert_true: Object identity should hold. expected true got false

I think this failure is because you return a different object every time, as I mentioned earlier.

> LayoutTests/imported/w3c/web-platform-tests/url/url-constructor-expected.txt:3
> +FAIL URL.searchParams updating, clearing assert_equals: expected "" but got "a=b"

Why this failure?

> LayoutTests/imported/w3c/web-platform-tests/url/url-constructor-expected.txt:5
> +FAIL URL.searchParams and URL.search setters, update propagation assert_equals: expected "e=f&g=h" but got "a=b&c=d"

Why this failure?

[Chris Dumez]

Comment on attachment 288713 [details]
Patch

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

I haven't reviewed the parsing code in much detail yet.

> Source/WebCore/html/URLSearchParams.cpp:38
> +        m_pairs = URLParser::parseURLEncodedForm(init.substring(1));

Should use StringView.

> Source/WebCore/html/URLSearchParams.cpp:73
> +                updateURL();

Shouldn't this be done after we've removed all the other items that have the same name?

> Source/WebCore/html/URLSearchParams.h:57
> +    void updateURL();

I think this should be above the data members, separated with a blank line.

[Alex Christensen]

(In reply to comment #5)
> > LayoutTests/imported/w3c/web-platform-tests/url/url-constructor-expected.txt:2
> > +FAIL URL.searchParams getter assert_true: Object identity should hold. expected true got false
> 
> I think this failure is because you return a different object every time, as
> I mentioned earlier.
Yep, I fixed it and noticed this started passing, so I won't add another test just for this.

[Chris Dumez]

Comment on attachment 288713 [details]
Patch

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

> Source/WebCore/platform/URLParser.cpp:1415
> +static String percentDecode(const CharacterType* input, size_t length)

I think maybe this function could use a Vector internally instead of a StringBuilder and return a CString instead of a String.

> Source/WebCore/platform/URLParser.cpp:1590
> +    auto utf8 = input.utf8();

Should we use utf8(StrictConversion) here and return Nullopt if it returns a null CString? I think this would avoid the > 0xFF below I think.

> Source/WebCore/platform/URLParser.cpp:1594
> +    for (size_t i = 0; i < length; ++i) {

I think we may be able to avoid this loop if we use utf8(StrictConversion) above and have percentDecode() return a CString.

> Source/WebCore/platform/URLParser.cpp:1601
> +    return String::fromUTF8(buffer.get(), length);

if percentDecode() returned a CString, we could simply return String::fromUTF8(decoded); here

> Source/WebCore/platform/URLParser.cpp:1611
> +        bytes.replace('+', 0x20);

The spec says:
"If bytes is the empty byte sequence, run these substeps for the next byte sequence."

which I would translate as:
if (bytes.isEmpty())
    continue;

However, this seems to be missing in your implementation.

> Source/WebCore/platform/URLParser.cpp:1614
> +            if (auto name = formURLDecode(bytes))

We don't need this if() check if we do the early continue; above when bytes is empty, as per the spec.

[Alex Christensen]

Created attachment 288730 [details]
Patch

[Alex Christensen]

(In reply to comment #5)
> > LayoutTests/imported/w3c/web-platform-tests/url/url-constructor-expected.txt:3
> > +FAIL URL.searchParams updating, clearing assert_equals: expected "" but got "a=b"
> 
> Why this failure?
> 
> > LayoutTests/imported/w3c/web-platform-tests/url/url-constructor-expected.txt:5
> > +FAIL URL.searchParams and URL.search setters, update propagation assert_equals: expected "e=f&g=h" but got "a=b&c=d"
> 
> Why this failure?

I'm not sure, but our query setting is already a mess. I think this should be addressed in a followup patch.

I didn't take your latest feedback into account in the latest patch.  Give me a sec.

[Alex Christensen]

Created attachment 288736 [details]
Patch

[Chris Dumez]

Comment on attachment 288736 [details]
Patch

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

> Source/WebCore/html/DOMURL.cpp:121
> +    return m_searchParams.get();

You should try the following:
var u = new URL("http://www.google.com?q=foo");
u.searchParams.foo = 1;
gc();
console.log(u.searchParams.foo); // Should log one but I think it will log undefined because the wrapper was collected. I think the [CachedAttribute] on IDL that I suggested may fix this.

Either way, it would be good to add this as a layout test.

[Chris Dumez]

Comment on attachment 288736 [details]
Patch

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

> Source/WebCore/html/DOMURL.cpp:60
> +    , m_searchParams(URLSearchParams::create(m_url.query(), this))

I think m_searchParams should be constructed lazily.

[Chris Dumez]

Comment on attachment 288736 [details]
Patch

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

> Source/WebCore/platform/URLParser.cpp:1606
> +                    result.append(String(input.characters8() + afterLastSeparator, length));

StringView() here and 3 more times below.

> Source/WebCore/platform/URLParser.cpp:1628
> +        auto bytes = view.toString().replace('+', 0x20);

We could do the replace() on name / value, after calling formURLDecode() to avoid converting the StringView() to a String right away given than formURLDecode() takes a StringView. This is also more consistent with the spec.

[Chris Dumez]

Comment on attachment 288736 [details]
Patch

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

> Source/WebCore/html/URLSearchParams.cpp:99
> +    for (const auto& pair : m_pairs) {

value.resizeInitialCapacity(m_pairs.size());

> Source/WebCore/html/URLSearchParams.cpp:101
> +            values.append(pair.second);

uncheckedAppend()

[Chris Dumez]

Comment on attachment 288736 [details]
Patch

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

> Source/WebCore/platform/URLParser.cpp:1599
> +    Vector<StringView> result;

I think this can be written in a simpler way (similarly to WTFString's split):
    unsigned startPos = 0;
    size_t endPos;
    while ((endPos = find(separator, startPos)) != notFound) {
        if (startPos != endPos)
            result.append(substring(startPos, endPos - startPos));
        startPos = endPos + separator.length();
    }
    if (startPos != length())
        result.append(substring(startPos));

[Alex Christensen]

(In reply to comment #12)
> You should try the following:
> var u = new URL("http://www.google.com?q=foo");
> u.searchParams.foo = 1;
> gc();
> console.log(u.searchParams.foo); // Should log one but I think it will log
> undefined because the wrapper was collected. I think the [CachedAttribute]
> on IDL that I suggested may fix this.
> 
> Either way, it would be good to add this as a layout test.
Yep, that's a good test.  I'll add it.
If [CachedAttribute] is used and the URLSearchParams has a RefPtr<DOMURL>, the value is preserved across gc, otherwise it is not.

[Alex Christensen]

Created attachment 288743 [details]
Patch

[Chris Dumez]

Comment on attachment 288743 [details]
Patch

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

r=me with comments if the bots are happy.

> Source/WTF/wtf/text/StringView.h:425
> +inline Vector<StringView> StringView::split(UChar separator)

This seems like a bit function to inline. The one on WTFString is not. I suggest we un-inline it.

> Source/WebCore/html/URLSearchParams.cpp:35
> +    , m_pairs(init.startsWith('?') ? URLParser::parseURLEncodedForm(StringView(init).substring(1)) : URLParser::parseURLEncodedForm(StringView(init)))

nit: you don't need the StringView() around init for the second call.

> Source/WebCore/html/URLSearchParams.cpp:69
> +            updateURL();

Why are you still calling updateURL() here?

> Source/WebCore/html/URLSearchParams.cpp:84
> +    m_pairs.append(std::make_pair(name, value));

nit: { name, value }

> Source/WebCore/html/URLSearchParams.cpp:90
> +    m_pairs.append(std::make_pair(name, value));

nit: { name, value }

> Source/WebCore/html/URLSearchParams.h:49
> +    URLSearchParams(const Vector<std::pair<String, String>>&);

nit: explicit

> Source/WebCore/platform/URLParser.cpp:1414
> +static String percentDecode(const LChar* input, size_t length)

nit: Can you add a FIXME comment saying this should return a CString?

> Source/WebCore/platform/URLParser.cpp:1613
> +    for (auto& pair : output) {

nit: I think it would be nicer if you did this in the loop above instead of iterating a second time.

[Alex Christensen]

Created attachment 288748 [details]
Patch

[Alex Christensen]

Created attachment 288753 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/changeset/205893

[Alex Christensen]

Comment on attachment 288753 [details]
Patch

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

> Source/WebCore/html/URLSearchParams.cpp:67
> +        bool urlNeedsUpdating = false;

This was breaking correctness.  I didn't commit this optimization.

[Chris Rebert]

*** Bug 133276 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 288753 [details]
Patch

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

> Source/WTF/wtf/text/StringView.h:118
> +    WTF_EXPORT_STRING_API Vector<StringView> split(UChar);

I think it would be better to use an iterator interface rather than having to pay the cost of allocating a vector.

[Alex Christensen]

https://bugs.webkit.org/show_bug.cgi?id=163225