Summary: | [Fetch API] Add support for URLSearchParams body | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, esprehn+autocc, gyuyoung.kim | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
youenn fablet
2016-09-28 02:21:14 PDT
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. |