We should support URLSearchParams bodies as per https://fetch.spec.whatwg.org/#body-mixin
Created attachment 290073 [details] Patch
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.
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.*.
(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.
This behavior is already in the repository... This patch is only moving the code we are talking from method to function.
Comment on attachment 290073 [details] Patch Dang it, you're right. This doesn't make things worse
We really ought to fix it soon, though. Add a FIXME saying that it might not do the right thing with unicode characters.
Created attachment 290308 [details] Patch for landing
Comment on attachment 290308 [details] Patch for landing Clearing flags on attachment: 290308 Committed r206632: <http://trac.webkit.org/changeset/206632>
All reviewed patches have been landed. Closing bug.
(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.
> > > 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.