Bug 154820

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.