Bug 154820 – [Fetch API] Support Request and Response blob() when body data is a blob

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Body can store data as a blob, and blob() should return the corresponding data.

[youenn fablet]

Created attachment 272492 [details]
Patch

[youenn fablet]

Created attachment 272499 [details]
Patch

[Darin Adler]

Comment on attachment 272499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272499&action=review

It’s annoying to have both Vector<char> and Vector<unsigned char>. We’ll have code bloat.

> Source/WebCore/Modules/fetch/FetchBody.cpp:139
> +        RefPtr<Blob> blob = Blob::create(extractFromText(), contentType);
> +        promise.resolve(WTFMove(blob));

Would read better without the local variable:

    promise.resolve(Blob::create(extractFromText(), contentType));

But also, noticing that the type here is a RefPtr, not a Ref, can Blob::create return null? Do we need to do something different in that case?

> Source/WebCore/Modules/fetch/FetchBody.cpp:183
> +    CString data = m_text.utf8();
> +    Vector<char> value(data.length());
> +    memcpy(value.data(), data.data(), data.length());
> +    return value;

This allocates memory twice, once for the CString and the second time for the Vector. Slower than just allocating once. We should look over the String::utf8 function and figure out how to make one that goes right into a Vector<char> (or maybe into a Vector<unsigned char>). It also zeroes out the entire vector before doing the memcpy. Slightly wasteful.

> Source/WebCore/bindings/js/JSDOMBinding.h:458
> +    RefPtr<ArrayBuffer> buffer = ArrayBuffer::create(vector.data(), vector.size());
> +    return toJS(state, globalObject, buffer.get());

Can ArrayBuffer::create return null? If so, does this handle it correctly?

> Source/WebCore/bindings/js/JSDOMBinding.h:464
> +    RefPtr<ArrayBuffer> buffer = ArrayBuffer::create(vector.data(), vector.size());
> +    return toJS(state, globalObject, buffer.get());

Ditto.

[youenn fablet]

Created attachment 272555 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #3)
> Comment on attachment 272499 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272499&action=review
> 
> It’s annoying to have both Vector<char> and Vector<unsigned char>. We’ll
> have code bloat.

Right.
I updated the code to directly pass an ArrayBuffer to the promise.
A follow-up patch could remove DeferredWrapper::resolve<Vector<unsigned char>>.

> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:139
> > +        RefPtr<Blob> blob = Blob::create(extractFromText(), contentType);
> > +        promise.resolve(WTFMove(blob));
> 
> Would read better without the local variable:

OK

>     promise.resolve(Blob::create(extractFromText(), contentType));
> 
> But also, noticing that the type here is a RefPtr, not a Ref, can
> Blob::create return null? Do we need to do something different in that case?

Blob::create returns a Ref actually.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:183
> > +    CString data = m_text.utf8();
> > +    Vector<char> value(data.length());
> > +    memcpy(value.data(), data.data(), data.length());
> > +    return value;
> 
> This allocates memory twice, once for the CString and the second time for
> the Vector. Slower than just allocating once. We should look over the
> String::utf8 function and figure out how to make one that goes right into a
> Vector<char> (or maybe into a Vector<unsigned char>). It also zeroes out the
> entire vector before doing the memcpy. Slightly wasteful.

Yes, I added a more specific FIXME about it.

> > Source/WebCore/bindings/js/JSDOMBinding.h:458
> > +    RefPtr<ArrayBuffer> buffer = ArrayBuffer::create(vector.data(), vector.size());
> > +    return toJS(state, globalObject, buffer.get());
> 
> Can ArrayBuffer::create return null? If so, does this handle it correctly?

Yes it can and it will translate to a null JS value.

[WebKit Commit Bot]

Comment on attachment 272555 [details]
Patch for landing

Clearing flags on attachment: 272555

Committed r197396: <http://trac.webkit.org/changeset/197396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.