752402 - Blob constructor should take ArrayBufferView as a member of blobParts parameter in addition to ArrayBuffer

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

Per latest File API spec.

Comment 1

[Masatoshi Kimura [:emk]]

Do we need to disable Blob constructor on aurora and beta? This is an incompatible change.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we should disable it no. It's unclear to me if we'll actually be able to remove ArrayBuffer as a valid type here. Based on conversations with microsoft people, apparently IE10 is going ship with the Blob constructor taking ArrayBuffers (it's too late in their cycle to change). So we might be stuck with ArrayBuffer as an overload.

We'll discuss this more on the list, but I see no reason to start disabling things in our implementation at this point.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch

This patch doesn't remove ArrayBuffer support.

Comment 4

[Alex Keybl [:akeybl]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> I don't think we should disable it no. It's unclear to me if we'll actually
> be able to remove ArrayBuffer as a valid type here. Based on conversations
> with microsoft people, apparently IE10 is going ship with the Blob
> constructor taking ArrayBuffers (it's too late in their cycle to change). So
> we might be stuck with ArrayBuffer as an overload.
> 
> We'll discuss this more on the list, but I see no reason to start disabling
> things in our implementation at this point.

If we're not considering disabling the Blob constructor, are there any remaining next actions for FF13.

Comment 5

[Masatoshi Kimura [:emk]]

Probably none. Withdrawn tracking request.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't actually think we should remove support for ArrayBuffer yet. Apparently microsoft is too late in their shipping cycle to remove support for ArrayBuffer in IE10. After which it's probably too late to remove ArrayBuffer support.

So the spec might just end up having to require support for both ArrayBuffer *and* ArrayBufferView.

I think for now we should just leave ArrayBuffer in there and see where the spec head.

Does that sound ok?

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 621672 [details] [diff] [review]
patch

Fair enough. I'll update the patch later.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Support ArrayBufferView as a member of blobParts parameter

Use of ArrayBuffer is no longer deprecated.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 623505 [details] [diff] [review]
Support ArrayBufferView as a member of blobParts parameter

Review of attachment 623505 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +263,1 @@
>        } else if (element.isString()) {

I'm not sure that this else-branch is worth it any more. JS_ValueToString should be quite fast when the passed in value is a string. I'd just remove it.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Support ArrayBufferView as a member of blobParts parameter

patch for checkin

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79d51b043d45

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/79d51b043d45

Comment 13

[Arun Ranganathan]

(In reply to Jonas Sicking (:sicking) from comment #6)
> I don't actually think we should remove support for ArrayBuffer yet.
> Apparently microsoft is too late in their shipping cycle to remove support
> for ArrayBuffer in IE10. After which it's probably too late to remove
> ArrayBuffer support.
> 
> So the spec might just end up having to require support for both ArrayBuffer
> *and* ArrayBufferView.
> 
> I think for now we should just leave ArrayBuffer in there and see where the
> spec head.
> 
> Does that sound ok?

Presently, the spec. only supports ArrayBufferView.  I'm not sure whether it should also support ArrayBuffer, but if two leading implementations support ArrayBuffer I suppose that decision is already made.