Closed Bug 752402 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: emk, Assigned: emk)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

Per latest File API spec.
Do we need to disable Blob constructor on aurora and beta? This is an incompatible change.
Blocks: 721569
Blocks: 744907
Summary: Blob constructor should take ArrayBufferView as a BlobParts member instead of ArrayBuffer → Blob constructor should take ArrayBufferView as a member of blobParts parameter instead of ArrayBuffer
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.
Attached patch patch (obsolete) — Splinter Review
This patch doesn't remove ArrayBuffer support.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #621672 - Flags: review?(jonas)
(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.
Probably none. Withdrawn tracking request.
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 on attachment 621672 [details] [diff] [review]
patch

Fair enough. I'll update the patch later.
Attachment #621672 - Flags: review?(jonas)
Summary: Blob constructor should take ArrayBufferView as a member of blobParts parameter instead of ArrayBuffer → Blob constructor should take ArrayBufferView as a member of blobParts parameter in addition to ArrayBuffer
Use of ArrayBuffer is no longer deprecated.
Attachment #621672 - Attachment is obsolete: true
Attachment #623505 - Flags: review?(jonas)
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.
Attachment #623505 - Flags: review?(jonas) → review+
Keywords: dev-doc-needed
Keywords: checkin-needed
Attachment #623505 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d51b043d45
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/79d51b043d45
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.