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

RESOLVED FIXED in mozilla15

Status

()

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-needed})

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

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

7 years ago
Per latest File API spec.
Assignee

Comment 1

7 years ago
Do we need to disable Blob constructor on aurora and beta? This is an incompatible change.
Blocks: 721569
Assignee

Updated

7 years ago
Blocks: 744907
Assignee

Updated

7 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

7 years ago
Posted patch patch (obsolete) — Splinter Review
This patch doesn't remove ArrayBuffer support.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Assignee

Updated

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

Comment 5

7 years ago
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?
Assignee

Comment 7

7 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

7 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

7 years ago
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

7 years ago
Keywords: dev-doc-needed
Assignee

Updated

7 years ago
Keywords: checkin-needed
Assignee

Updated

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

Comment 13

7 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.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.