Bug 162667 – [Fetch API] Add support for URLSearchParams body

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

We should support URLSearchParams bodies as per https://fetch.spec.whatwg.org/#body-mixin

[youenn fablet]

Created attachment 290073 [details]
Patch

[Alex Christensen]

Comment on attachment 290073 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:53
> +    CString data = text.utf8();

Is this correct? I think there should be a test with an emoji in it.

> Source/WebCore/Modules/fetch/FetchBody.cpp:95
> +    , m_contentType(ASCIILiteral("application/x-www-form-urlencoded;charset=UTF-8"))

This string exists in many places in WebCore, all of them so far without the charset.  There should be a common place for this const char[] definition somewhere.

[youenn fablet]

Comment on attachment 290073 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchBody.cpp:53
>> +    CString data = text.utf8();
> 
> Is this correct? I think there should be a test with an emoji in it.

Good point, there is no test for that.
I plan to fix the double allocation by moving this function in WTFString.
Can it be tackled at that time?

>> Source/WebCore/Modules/fetch/FetchBody.cpp:95
>> +    , m_contentType(ASCIILiteral("application/x-www-form-urlencoded;charset=UTF-8"))
> 
> This string exists in many places in WebCore, all of them so far without the charset.  There should be a common place for this const char[] definition somewhere.

Sounds good, how about adding in a future patch common HTTP header values in Source/WebCore/platform/, like in a new file like network/HTTPHeaderValues.*.

[Alex Christensen]

(In reply to comment #3)
> Comment on attachment 290073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290073&action=review
> 
> >> Source/WebCore/Modules/fetch/FetchBody.cpp:53
> >> +    CString data = text.utf8();
> > 
> > Is this correct? I think there should be a test with an emoji in it.
> 
> Good point, there is no test for that.
> I plan to fix the double allocation by moving this function in WTFString.
> Can it be tackled at that time?
I think this should be done in the same patch so we don't have incorrect behavior in trunk. 
> 
> >> Source/WebCore/Modules/fetch/FetchBody.cpp:95
> >> +    , m_contentType(ASCIILiteral("application/x-www-form-urlencoded;charset=UTF-8"))
> > 
> > This string exists in many places in WebCore, all of them so far without the charset.  There should be a common place for this const char[] definition somewhere.
> 
> Sounds good, how about adding in a future patch common HTTP header values in
> Source/WebCore/platform/, like in a new file like network/HTTPHeaderValues.*.
Sure. Different patch ok.

[youenn fablet]

This behavior is already in the repository... This patch is only moving the code we are talking from method to function.

[Alex Christensen]

Comment on attachment 290073 [details]
Patch

Dang it, you're right.  This doesn't make things worse

[Alex Christensen]

We really ought to fix it soon, though.  Add a FIXME saying that it might not do the right thing with unicode characters.

[youenn fablet]

Created attachment 290308 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290308 [details]
Patch for landing

Clearing flags on attachment: 290308

Committed r206632: <http://trac.webkit.org/changeset/206632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to comment #7)
> We really ought to fix it soon, though.  Add a FIXME saying that it might
> not do the right thing with unicode characters.

I did some tests and cannot make it faulty, even with non-BMP characters.
That said, it is more consistent to use UTF8Encoding.
Also, this code needs to be changed anyway to remove the no-longer needed vector allocation.

[youenn fablet]

> > > This string exists in many places in WebCore, all of them so far without the charset.  There should be a common place for this const char[] definition somewhere.
> > 
> > Sounds good, how about adding in a future patch common HTTP header values in
> > Source/WebCore/platform/, like in a new file like network/HTTPHeaderValues.*.
> Sure. Different patch ok.

Filed bug 163002 for that.