Summary: | [Fetch API] Support Request and Response blob() when body data is a blob | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 151937 | ||||||||||
Attachments: |
|
Description
youenn fablet
2016-02-29 09:16:21 PST
Created attachment 272492 [details]
Patch
Created attachment 272499 [details]
Patch
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. Created attachment 272555 [details]
Patch for landing
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. Comment on attachment 272555 [details] Patch for landing Clearing flags on attachment: 272555 Committed r197396: <http://trac.webkit.org/changeset/197396> All reviewed patches have been landed. Closing bug. |