Bug 161920 - Implement URLSearchParams
Summary: Implement URLSearchParams
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
: 133276 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-09-13 11:50 PDT by Alex Christensen
Modified: 2016-10-10 11:08 PDT (History)
2 users (show)

See Also:


Attachments
Patch (39.41 KB, patch)
2016-09-13 11:54 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (60.69 KB, patch)
2016-09-13 12:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (63.47 KB, patch)
2016-09-13 14:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (62.67 KB, patch)
2016-09-13 15:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (63.43 KB, patch)
2016-09-13 16:27 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (64.43 KB, patch)
2016-09-13 16:58 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (64.46 KB, patch)
2016-09-13 17:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-09-13 11:50:42 PDT
Implement URLSearchParams
Comment 1 Alex Christensen 2016-09-13 11:54:29 PDT
Created attachment 288707 [details]
Patch
Comment 2 Chris Dumez 2016-09-13 11:59:26 PDT
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&
Comment 3 Alex Christensen 2016-09-13 12:17:37 PDT
Created attachment 288713 [details]
Patch
Comment 4 Chris Dumez 2016-09-13 12:39:05 PDT
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*.
Comment 5 Chris Dumez 2016-09-13 13:01:40 PDT
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?
Comment 6 Chris Dumez 2016-09-13 13:09:04 PDT
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.
Comment 7 Alex Christensen 2016-09-13 13:22:18 PDT
(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.
Comment 8 Chris Dumez 2016-09-13 13:48:56 PDT
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.
Comment 9 Alex Christensen 2016-09-13 14:31:24 PDT
Created attachment 288730 [details]
Patch
Comment 10 Alex Christensen 2016-09-13 14:32:38 PDT
(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.
Comment 11 Alex Christensen 2016-09-13 15:21:36 PDT
Created attachment 288736 [details]
Patch
Comment 12 Chris Dumez 2016-09-13 15:46:07 PDT
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.
Comment 13 Chris Dumez 2016-09-13 15:46:55 PDT
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.
Comment 14 Chris Dumez 2016-09-13 15:51:47 PDT
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.
Comment 15 Chris Dumez 2016-09-13 15:54:11 PDT
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()
Comment 16 Chris Dumez 2016-09-13 15:57:14 PDT
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));
Comment 17 Alex Christensen 2016-09-13 15:58:23 PDT
(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.
Comment 18 Alex Christensen 2016-09-13 16:27:07 PDT
Created attachment 288743 [details]
Patch
Comment 19 Chris Dumez 2016-09-13 16:48:06 PDT
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.
Comment 20 Alex Christensen 2016-09-13 16:58:28 PDT
Created attachment 288748 [details]
Patch
Comment 21 Alex Christensen 2016-09-13 17:23:44 PDT
Created attachment 288753 [details]
Patch
Comment 22 Alex Christensen 2016-09-13 18:37:03 PDT
http://trac.webkit.org/changeset/205893
Comment 23 Alex Christensen 2016-09-13 18:38:07 PDT
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.
Comment 24 Chris Rebert 2016-09-30 10:16:58 PDT
*** Bug 133276 has been marked as a duplicate of this bug. ***
Comment 25 Darin Adler 2016-10-07 10:35:53 PDT
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.
Comment 26 Alex Christensen 2016-10-10 11:08:54 PDT
https://bugs.webkit.org/show_bug.cgi?id=163225