Summary: | [Fetch API] Add support for BufferSource bodies | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, sam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 151937, 161190 | ||||||||||||
Attachments: |
|
Description
youenn fablet
2016-08-23 09:53:33 PDT
Created attachment 286737 [details]
Patch
Created attachment 286749 [details]
Adding FormData upload
Created attachment 286772 [details]
Adding FormData upload
Note the small difference of the request content-type between XHR and fetch API for FormData upload: XHR has a space before boundary (legacy), not fetch API (as per fetch spec). I guess XHR should be aligned. But from what I saw, chrome and Firefox also insert a space before boundary parameter for fetch API (and XHR I guess). Comment on attachment 286772 [details] Adding FormData upload View in context: https://bugs.webkit.org/attachment.cgi?id=286772&action=review > Source/WebCore/Modules/fetch/FetchBody.cpp:58 > + m_formData = FormData::createMultiPart(static_cast<FormDataList&>(formData), formData.encoding(), &document); I’m concerned about this static_cast to FormDataList& and I don’t understand why it is needed. > Source/WebCore/Modules/fetch/FetchBody.cpp:212 > + m_text.releaseImpl(); This is not the right thing to write. Instead please do something more like this: m_text = { }; We don’t want to start using releaseImpl just to clear out a string. > Source/WebCore/Modules/fetch/FetchBody.cpp:254 > + m_text.releaseImpl(); Ditto. > Source/WebCore/Modules/fetch/FetchBody.cpp:295 > if (m_type == Type::None) Why cascading if here instead of switch? I think it should be switch since the other functions are. > Source/WebCore/Modules/fetch/FetchBody.h:111 > - // FIXME: Add support for BufferSource and URLSearchParams. > + // FIXME: Add support for URLSearchParams. > RefPtr<Blob> m_blob; > - RefPtr<DOMFormData> m_formData; > + RefPtr<FormData> m_formData; > RefPtr<ArrayBuffer> m_data; > + RefPtr<ArrayBufferView> m_dataView; > String m_text; We should change this class to use std::experimental::variant soon. > Source/WebCore/Modules/fetch/FetchRequest.cpp:260 > + ASSERT(scriptExecutionContext()); What guarantees this assertion will not fail? > Source/WebCore/Modules/fetch/FetchRequest.cpp:300 > + ASSERT(scriptExecutionContext()); What guarantees this assertion will not fail? > Source/WebCore/Modules/fetch/FetchResponse.cpp:85 > + ASSERT(scriptExecutionContext()); What guarantees this assertion will not fail? Thanks for the review. (In reply to comment #5) > Comment on attachment 286772 [details] > Adding FormData upload > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286772&action=review > > > Source/WebCore/Modules/fetch/FetchBody.cpp:58 > > + m_formData = FormData::createMultiPart(static_cast<FormDataList&>(formData), formData.encoding(), &document); > > I’m concerned about this static_cast to FormDataList& and I don’t understand > why it is needed. It is not needed, I removed it. > > > Source/WebCore/Modules/fetch/FetchBody.cpp:212 > > + m_text.releaseImpl(); > > This is not the right thing to write. Instead please do something more like > this: > > m_text = { }; > > We don’t want to start using releaseImpl just to clear out a string. OK > > Source/WebCore/Modules/fetch/FetchBody.cpp:254 > > + m_text.releaseImpl(); > > Ditto. > > > Source/WebCore/Modules/fetch/FetchBody.cpp:295 > > if (m_type == Type::None) > > Why cascading if here instead of switch? I think it should be switch since > the other functions are. Right, we support all relevant cases now. > > Source/WebCore/Modules/fetch/FetchBody.h:111 > > - // FIXME: Add support for BufferSource and URLSearchParams. > > + // FIXME: Add support for URLSearchParams. > > RefPtr<Blob> m_blob; > > - RefPtr<DOMFormData> m_formData; > > + RefPtr<FormData> m_formData; > > RefPtr<ArrayBuffer> m_data; > > + RefPtr<ArrayBufferView> m_dataView; > > String m_text; > > We should change this class to use std::experimental::variant soon. OK. > > Source/WebCore/Modules/fetch/FetchRequest.cpp:260 > > + ASSERT(scriptExecutionContext()); > > What guarantees this assertion will not fail? setBody is called when constructing FetchRequest. FetchRequest built-in constructor takes a ScriptExecutionContext. The check is done in createJSObject. We could make setBody take a ScriptExecutionContext& but that would add never-used binding generated code and an unneeded if check. > > Source/WebCore/Modules/fetch/FetchRequest.cpp:300 > > + ASSERT(scriptExecutionContext()); > > What guarantees this assertion will not fail? internalRequest() is called from FetchLoader::start which would not be called if scriptExecutionContext is null. The check is done in DOMWindowFetch/WorkerGlobalScope::fetch. Maybe it would be clearer to pass a ScriptExecutionContext& as parameter in that case. > > Source/WebCore/Modules/fetch/FetchResponse.cpp:85 > > + ASSERT(scriptExecutionContext()); > > What guarantees this assertion will not fail? This is similar to the first case, the check is done in createJSObject. Created attachment 287258 [details]
Patch for landing
Comment on attachment 287258 [details] Patch for landing Clearing flags on attachment: 287258 Committed r205115: <http://trac.webkit.org/changeset/205115> All reviewed patches have been landed. Closing bug. |