Bug 161087 - [Fetch API] Add support for BufferSource bodies
Summary: [Fetch API] Add support for BufferSource bodies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937 161190
  Show dependency treegraph
 
Reported: 2016-08-23 09:53 PDT by youenn fablet
Modified: 2016-08-29 02:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (26.68 KB, patch)
2016-08-23 11:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding FormData upload (29.51 KB, patch)
2016-08-23 11:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding FormData upload (33.00 KB, patch)
2016-08-23 14:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (33.09 KB, patch)
2016-08-29 01:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-08-23 09:53:33 PDT
We should add support for ArrayBuffer/ArrayBufferView for fetch requests and responses.
Comment 1 youenn fablet 2016-08-23 11:03:27 PDT
Created attachment 286737 [details]
Patch
Comment 2 youenn fablet 2016-08-23 11:35:59 PDT
Created attachment 286749 [details]
Adding FormData upload
Comment 3 youenn fablet 2016-08-23 14:10:43 PDT
Created attachment 286772 [details]
Adding FormData upload
Comment 4 youenn fablet 2016-08-23 14:14:39 PDT
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 5 Darin Adler 2016-08-27 17:33:19 PDT
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?
Comment 6 youenn fablet 2016-08-29 01:48:41 PDT
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.
Comment 7 youenn fablet 2016-08-29 01:49:13 PDT
Created attachment 287258 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2016-08-29 02:19:56 PDT
Comment on attachment 287258 [details]
Patch for landing

Clearing flags on attachment: 287258

Committed r205115: <http://trac.webkit.org/changeset/205115>
Comment 9 WebKit Commit Bot 2016-08-29 02:20:01 PDT
All reviewed patches have been landed.  Closing bug.