Bug 161087

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.