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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-needed})

Trunk
mozilla15
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Per latest File API spec.
(Assignee)

Comment 1

5 years ago
Do we need to disable Blob constructor on aurora and beta? This is an incompatible change.
Blocks: 721569
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
(Assignee)

Updated

5 years ago
Blocks: 744907
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 621672 [details] [diff] [review]
patch

This patch doesn't remove ArrayBuffer support.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #621672 - Flags: review?(jonas)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Probably none. Withdrawn tracking request.
tracking-firefox13: ? → ---
tracking-firefox14: ? → ---
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?
(Assignee)

Comment 7

5 years ago
Comment on attachment 621672 [details] [diff] [review]
patch

Fair enough. I'll update the patch later.
Attachment #621672 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 8

5 years ago
Created attachment 623505 [details] [diff] [review]
Support ArrayBufferView as a member of blobParts parameter

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+
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 10

5 years ago
Created attachment 624857 [details] [diff] [review]
Support ArrayBufferView as a member of blobParts parameter

patch for checkin
Attachment #624857 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 13

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.