[webkit-reviews] review denied: [Bug 88294] FileAPI: Blob should support ArrayBufferView instead of ArrayBuffer for Constructor Parameters : [Attachment 146275] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Li Yin <li.yin at intel.com>'s request
for review:
Bug 88294: FileAPI: Blob should support ArrayBufferView instead of ArrayBuffer
for Constructor Parameters
https://bugs.webkit.org/show_bug.cgi?id=88294

Attachment 146275: Patch
https://bugs.webkit.org/attachment.cgi?id=146275&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
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.