Bug 161087 – [Fetch API] Add support for BufferSource bodies

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

We should add support for ArrayBuffer/ArrayBufferView for fetch requests and responses.

[youenn fablet]

Created attachment 286737 [details]
Patch

[youenn fablet]

Created attachment 286749 [details]
Adding FormData upload

[youenn fablet]

Created attachment 286772 [details]
Adding FormData upload

[youenn fablet]

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).

[Darin Adler]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 287258 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 287258 [details]
Patch for landing

Clearing flags on attachment: 287258

Committed r205115: <http://trac.webkit.org/changeset/205115>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.