Bug 88294 - FileAPI: Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters
Summary: FileAPI: Blob should support ArrayBufferView instead of ArrayBuffer for Const...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Li Yin
URL:
Keywords:
Depends on:
Blocks: 88389
  Show dependency treegraph
 
Reported: 2012-06-04 23:26 PDT by Li Yin
Modified: 2012-06-08 00:40 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.04 KB, patch)
2012-06-05 01:23 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2012-06-05 01:56 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2012-06-05 20:04 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (16.30 KB, patch)
2012-06-07 01:10 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (16.24 KB, patch)
2012-06-07 06:31 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (16.14 KB, patch)
2012-06-07 18:11 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-06-04 23:26:01 PDT
From Spec: http://dev.w3.org/2006/webapi/FileAPI/#dfn-Blob
Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters.
Comment 1 Li Yin 2012-06-05 01:23:40 PDT
Created attachment 145721 [details]
Patch
Comment 2 Kentaro Hara 2012-06-05 01:28:24 PDT
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.
Comment 3 Li Yin 2012-06-05 01:31:37 PDT
(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.
Comment 4 Li Yin 2012-06-05 01:56:51 PDT
Created attachment 145732 [details]
Patch
Comment 5 Kinuko Yasuda 2012-06-05 05:13:25 PDT
(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?
Comment 6 Li Yin 2012-06-05 05:35:11 PDT
(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.
Comment 7 Kenneth Russell 2012-06-05 14:16:23 PDT
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.
Comment 8 Li Yin 2012-06-05 20:04:49 PDT
Created attachment 145919 [details]
Patch
Comment 9 Li Yin 2012-06-05 20:06:52 PDT
(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.
Comment 10 Li Yin 2012-06-05 20:17:21 PDT
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.
Comment 11 Li Yin 2012-06-06 17:59:35 PDT
Hi Jian,
  Could you give some comments? Thanks in advance.
Comment 12 Kinuko Yasuda 2012-06-06 20:16:45 PDT
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?
Comment 13 Li Yin 2012-06-07 01:10:19 PDT
Created attachment 146226 [details]
Patch
Comment 14 Li Yin 2012-06-07 01:12:18 PDT
(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.
Comment 15 Kinuko Yasuda 2012-06-07 01:59:18 PDT
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)?
Comment 16 Li Yin 2012-06-07 06:31:05 PDT
Created attachment 146275 [details]
Patch
Comment 17 Li Yin 2012-06-07 06:34:00 PDT
(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.
Comment 18 Jian Li 2012-06-07 11:49:32 PDT
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.
Comment 19 Li Yin 2012-06-07 17:43:29 PDT
(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.
Comment 20 Adam Barth 2012-06-07 17:48:18 PDT
> 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.
Comment 21 Li Yin 2012-06-07 17:59:32 PDT
(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.
Comment 22 Li Yin 2012-06-07 18:11:43 PDT
Created attachment 146440 [details]
Patch
Comment 23 Li Yin 2012-06-07 18:24:01 PDT
(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.
Comment 24 Jian Li 2012-06-07 19:22:12 PDT
(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.
Comment 25 Li Yin 2012-06-07 19:32:16 PDT
(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.
Comment 26 WebKit Review Bot 2012-06-07 20:15:42 PDT
Comment on attachment 146440 [details]
Patch

Clearing flags on attachment: 146440

Committed r119791: <http://trac.webkit.org/changeset/119791>
Comment 27 WebKit Review Bot 2012-06-07 20:15:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 mitz 2012-06-07 23:24:02 PDT
(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>.
Comment 29 Li Yin 2012-06-07 23:27:55 PDT
(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.
Comment 30 mitz 2012-06-08 00:27:42 PDT
(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>.
Comment 31 Li Yin 2012-06-08 00:40:04 PDT
(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.