Bug 88294 – FileAPI: Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Li Yin]

From Spec: http://dev.w3.org/2006/webapi/FileAPI/#dfn-Blob
Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters.

[Li Yin]

Created attachment 145721 [details]
Patch

[Kentaro Hara]

Comment on attachment 145721 [details]
Patch

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

The implementation looks correct but I am not sure if the change is acceptable in terms of compatibility. I'll delegate the review to File API folks.

> LayoutTests/fast/files/script-tests/blob-constructor.js:70
> +var intArray = new Uint32Array(100);
> +var shortArray = new Uint16Array(100);
> +var byteArray = new Uint8Array(100);

Let's add test cases for Int8Array, Int16Array, Int32Array, Uint8ClampedArray, Float32Array and Float64Array.

[Li Yin]

(In reply to comment #2)
> (From update of attachment 145721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145721&action=review
> 
> The implementation looks correct but I am not sure if the change is acceptable in terms of compatibility. I'll delegate the review to File API folks.
> 
> > LayoutTests/fast/files/script-tests/blob-constructor.js:70
> > +var intArray = new Uint32Array(100);
> > +var shortArray = new Uint16Array(100);
> > +var byteArray = new Uint8Array(100);
> 
> Let's add test cases for Int8Array, Int16Array, Int32Array, Uint8ClampedArray, Float32Array and Float64Array.

Okay, I will add more tests. Thanks for your review.

[Li Yin]

Created attachment 145732 [details]
Patch

[Kinuko Yasuda]

(Changing issue summary as (I was told) we usually use [...] tags for ports/platforms)

The change looks good to me, but since this change drops the backward compatibility (as the spec change intended) it might be good to post a short heads-up on webkit-dev?

[Li Yin]

(In reply to comment #5)
> (Changing issue summary as (I was told) we usually use [...] tags for ports/platforms)
> 
> The change looks good to me, but since this change drops the backward compatibility (as the spec change intended) it might be good to post a short heads-up on webkit-dev?

Good idea. I have sent a mail to the webkit-dev mail list already.
Thanks.

[Kenneth Russell]

Comment on attachment 145732 [details]
Patch

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

It may be worth supporting the Blob constructor taking ArrayBuffer for a brief period of time, warning that the old constructor is deprecated. See Source/WebCore/Modules/indexeddb/IDBIndex.cpp for an example.

The patch looks OK overall; I'm r-'ing it due to the need for a test of DataView.

> LayoutTests/ChangeLog:8
> +        Change ArrayBuffer into ArrayBufferView in Blob constrcuting function.

typo: constrcuting

> LayoutTests/fast/files/script-tests/blob-constructor.js:68
> +var blobObj = new Blob([new Int32Array(100)]);

Please test DataView too. DataView is the primary view intended to be used for file and network I/O.

[Li Yin]

Created attachment 145919 [details]
Patch

[Li Yin]

(In reply to comment #7)
> (From update of attachment 145732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145732&action=review
> 
> It may be worth supporting the Blob constructor taking ArrayBuffer for a brief period of time, warning that the old constructor is deprecated. See Source/WebCore/Modules/indexeddb/IDBIndex.cpp for an example.
> 
Great suggestion. Done

> The patch looks OK overall; I'm r-'ing it due to the need for a test of DataView.
> 
Done

> > LayoutTests/ChangeLog:8
> > +        Change ArrayBuffer into ArrayBufferView in Blob constrcuting function.
> 
> typo: constrcuting
>
Done
 
> > LayoutTests/fast/files/script-tests/blob-constructor.js:68
> > +var blobObj = new Blob([new Int32Array(100)]);
> 
> Please test DataView too. DataView is the primary view intended to be used for file and network I/O.

Done

Thanks for your review.

[Li Yin]

I filed a new bug 88389, it will remove the ArrayBuffer related support in future.

I have no the privilege to edit the bug.
Who can help me add this privilege?
Thanks a million in advance.

[Li Yin]

Hi Jian,
  Could you give some comments? Thanks in advance.

[Kinuko Yasuda]

Comment on attachment 145919 [details]
Patch

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

> Source/WebCore/fileapi/WebKitBlobBuilder.cpp:94
> +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("ArrayBuffer values are deprecated in Blob Constructor. Use ArrayBufferView instead."));

DEFINE_STATIC_LOCAL is not thread-safe while I think this could be also called on workers.

> Source/WebCore/fileapi/WebKitBlobBuilder.h:65
> +    WebKitBlobBuilder(ScriptExecutionContext*);

explicit

> Source/WebCore/fileapi/WebKitBlobBuilder.idl:35
> +        CallWith=ScriptExecutionContext,

Could we make append(ArrayBuffer) get called with ScriptExecutionContext rather than making the entire BlobBuilder callwith=SEC?

[Li Yin]

Created attachment 146226 [details]
Patch

[Li Yin]

(In reply to comment #12)
> DEFINE_STATIC_LOCAL is not thread-safe while I think this could be also called on workers.

Done.

> > Source/WebCore/fileapi/WebKitBlobBuilder.idl:35
> > +        CallWith=ScriptExecutionContext,
> 
> Could we make append(ArrayBuffer) get called with ScriptExecutionContext rather than making the entire BlobBuilder callwith=SEC?

Done.

Thanks for your review.

[Kinuko Yasuda]

Comment on attachment 146226 [details]
Patch

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

The change looks good to me.

> Source/WebCore/fileapi/WebKitBlobBuilder.h:35
> +#include "ScriptExecutionContext.h"

nit: could this be moved to .cpp (as we also forward-declare)?

[Li Yin]

Created attachment 146275 [details]
Patch

[Li Yin]

(In reply to comment #15)
> (From update of attachment 146226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146226&action=review
> 
> The change looks good to me.
> 
> > Source/WebCore/fileapi/WebKitBlobBuilder.h:35
> > +#include "ScriptExecutionContext.h"
> 
> nit: could this be moved to .cpp (as we also forward-declare)?

Done. Thanks.

[Jian Li]

Comment on attachment 146275 [details]
Patch

BlobBuilder interface is deprecated, but we're adding append(ArrayBufferView) to BlobBuilder unintentionally because we need to add ArrayBufferView parameter to Blob constructor and the blob construction is routed through BlobBuilder. I am not sure how this could be addressed.


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

> Source/WebCore/ChangeLog:9
> +        Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters.

nit: No need to repeat this line.

> Source/WebCore/ChangeLog:10
> +        Currently, we just add the support of ArrayBufferView, not delete ArrayBuffer because

nit: Currently we add the support for ArrayBufferView, while still keeping ArrayBuffer for backward compatibility. We will remove it in the near future.

> Source/WebCore/ChangeLog:25
> +        * fileapi/WebKitBlobBuilder.idl: Change it to support ArrayBufferView in append method

We should not say "Change" since we have not removed the legacy one yet.

> Source/WebCore/fileapi/WebKitBlobBuilder.cpp:93
> +    String consoleMessage("ArrayBuffer values are deprecated in Blob Constructor. Use ArrayBufferView instead.");

Use DEFINE_STATIC_LOCAL.

> LayoutTests/fast/files/script-tests/blob-constructor.js:79
> +shouldBe("new Blob([blobObj, new Uint8Array(100), new Float32Array(100), new DataView(new ArrayBuffer(100))]).size", "1000");

nit: Either you can move "var blobObj = ..." to just before this line. Or you can replace blobObj in shouldBe with the expression.

[Li Yin]

(In reply to comment #12)
> (From update of attachment 145919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145919&action=review
> 
> > Source/WebCore/fileapi/WebKitBlobBuilder.cpp:94
> > +    DEFINE_STATIC_LOCAL(String, consoleMessage, ("ArrayBuffer values are deprecated in Blob Constructor. Use ArrayBufferView instead."));
> 
> DEFINE_STATIC_LOCAL is not thread-safe while I think this could be also called on workers.

All the thread just read only the static value. and it isn't destroyed until the process exits. It should not have the thread-safe issue.
In addition, DEFINE_STATIC_LOCAL is used in many modules, according to my understanding, the advantage is reducing applying for local variables.

[Adam Barth]

> All the thread just read only the static value. and it isn't destroyed until the process exits. It should not have the thread-safe issue.

We don't use DEFINE_STATIC_LOCAL in multithreaded code because there's a race condition in initialization.

> In addition, DEFINE_STATIC_LOCAL is used in many modules, according to my understanding, the advantage is reducing applying for local variables.

Most code in WebCore is single threaded, which is why you see this macro used commonly.

[Li Yin]

(In reply to comment #20)
> > All the thread just read only the static value. and it isn't destroyed until the process exits. It should not have the thread-safe issue.
> 
> We don't use DEFINE_STATIC_LOCAL in multithreaded code because there's a race condition in initialization.
> 

I ignored the initialization, thanks for your reminding.

> > In addition, DEFINE_STATIC_LOCAL is used in many modules, according to my understanding, the advantage is reducing applying for local variables.
> 
> Most code in WebCore is single threaded, which is why you see this macro used commonly.

[Li Yin]

Created attachment 146440 [details]
Patch

[Li Yin]

(In reply to comment #18)
> (From update of attachment 146275 [details])
> BlobBuilder interface is deprecated, but we're adding append(ArrayBufferView) to BlobBuilder unintentionally because we need to add ArrayBufferView parameter to Blob constructor and the blob construction is routed through BlobBuilder. I am not sure how this could be addressed.
> 
Is WebKitBlobBuilder interface deprecated? Why?
If so, how will we implement Blob Constructor? Could you share some related information? Or where can I find this discussion?

> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=146275&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters.
> 
> nit: No need to repeat this line.
>
Done.
 
> > Source/WebCore/ChangeLog:10
> > +        Currently, we just add the support of ArrayBufferView, not delete ArrayBuffer because
> 
> nit: Currently we add the support for ArrayBufferView, while still keeping ArrayBuffer for backward compatibility. We will remove it in the near future.
>
Done.
 
> > Source/WebCore/ChangeLog:25
> > +        * fileapi/WebKitBlobBuilder.idl: Change it to support ArrayBufferView in append method
> 
> We should not say "Change" since we have not removed the legacy one yet.
> 
> > Source/WebCore/fileapi/WebKitBlobBuilder.cpp:93
> > +    String consoleMessage("ArrayBuffer values are deprecated in Blob Constructor. Use ArrayBufferView instead.");
> 
> Use DEFINE_STATIC_LOCAL.

DEFINE_STATIC_LOCAL is not thread-safe. and it isn't performance sensitive, I will keep a local string to do that.
Thanks Adam Barth and Kinuko's comments.

> > LayoutTests/fast/files/script-tests/blob-constructor.js:79
> > +shouldBe("new Blob([blobObj, new Uint8Array(100), new Float32Array(100), new DataView(new ArrayBuffer(100))]).size", "1000");
> 
> nit: Either you can move "var blobObj = ..." to just before this line. Or you can replace blobObj in shouldBe with the expression.

Done.

[Jian Li]

(In reply to comment #23)
> (In reply to comment #18)
> > (From update of attachment 146275 [details] [details])
> > BlobBuilder interface is deprecated, but we're adding append(ArrayBufferView) to BlobBuilder unintentionally because we need to add ArrayBufferView parameter to Blob constructor and the blob construction is routed through BlobBuilder. I am not sure how this could be addressed.
> > 
> Is WebKitBlobBuilder interface deprecated? Why?
> If so, how will we implement Blob Constructor? Could you share some related information? Or where can I find this discussion?
> 
See http://www.w3.org/TR/file-writer-api/#idl-def-BlobBuilder for the plan to deprecate BlobBuilder.
   The BlobBuilder interface is deprecated in favor of the new constructible Blob. However, at this time implementations generally support BlobBuilder and not constructible Blob.

Probably we do not need to do anything about exposing a new way to append sth in BlobBuilder since it will get deprecated in the future. Just let you know we might have such issue.

[Li Yin]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #18)
> > > (From update of attachment 146275 [details] [details] [details])
> > > BlobBuilder interface is deprecated, but we're adding append(ArrayBufferView) to BlobBuilder unintentionally because we need to add ArrayBufferView parameter to Blob constructor and the blob construction is routed through BlobBuilder. I am not sure how this could be addressed.
> > > 
> > Is WebKitBlobBuilder interface deprecated? Why?
> > If so, how will we implement Blob Constructor? Could you share some related information? Or where can I find this discussion?
> > 
> See http://www.w3.org/TR/file-writer-api/#idl-def-BlobBuilder for the plan to deprecate BlobBuilder.
>    The BlobBuilder interface is deprecated in favor of the new constructible Blob. However, at this time implementations generally support BlobBuilder and not constructible Blob.
> 
> Probably we do not need to do anything about exposing a new way to append sth in BlobBuilder since it will get deprecated in the future. Just let you know we might have such issue.

Okay, thanks for your clarification.

[WebKit Review Bot]

Comment on attachment 146440 [details]
Patch

Clearing flags on attachment: 146440

Committed r119791: <http://trac.webkit.org/changeset/119791>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #26)
> (From update of attachment 146440 [details])
> Clearing flags on attachment: 146440
> 
> Committed r119791: <http://trac.webkit.org/changeset/119791>

This caused fast/files/read-blob-async.html to fail. See for example <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119795%20(7453)/fast/files/read-blob-async-pretty-diff.html>.

[Li Yin]

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 146440 [details] [details])
> > Clearing flags on attachment: 146440
> > 
> > Committed r119791: <http://trac.webkit.org/changeset/119791>
> 
> This caused fast/files/read-blob-async.html to fail. See for example <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119795%20(7453)/fast/files/read-blob-async-pretty-diff.html>.

Thanks for your report. I will have a look.

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 146440 [details] [details])
> > Clearing flags on attachment: 146440
> > 
> > Committed r119791: <http://trac.webkit.org/changeset/119791>
> 
> This caused fast/files/read-blob-async.html to fail. See for example <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119795%20(7453)/fast/files/read-blob-async-pretty-diff.html>.

Fixed in <http://trac.webkit.org/r119805>.

[Li Yin]

(In reply to comment #30)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (From update of attachment 146440 [details] [details] [details])
> > > Clearing flags on attachment: 146440
> > > 
> > > Committed r119791: <http://trac.webkit.org/changeset/119791>
> > 
> > This caused fast/files/read-blob-async.html to fail. See for example <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119795%20(7453)/fast/files/read-blob-async-pretty-diff.html>.
> 
> Fixed in <http://trac.webkit.org/r119805>.

Yeah, the array.buffer should change into array.
My apology for missed changing in these tests because they are skipped on chromium  and Gtk platform.
Thank you do that.