Closed Bug 1057170 Opened 6 years ago Closed 6 years ago

[EME] Update MediaKey* Uint8Arrays to be (ArrayBuffer or ArrayBufferView)

Categories

(Core :: Audio/Video, defect)

29 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When I implemented the EME spec the initData and MediaKeySessioj.update(response) were Uint8Arrays, but the spec has changed and now they're (ArrayBuffer or ArrayBufferView).
Attached patch PatchSplinter Review
Update instances of Uint8Aarry in EME interfaces to (ArrayBuffer or ArrayBufferView), as that's what's in the current draft spec.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #8478056 - Flags: review?(bzbarsky)
Comment on attachment 8478056 [details] [diff] [review]
Patch

>+MediaKeySession::Update(const ArrayBufferOrArrayBufferView& aResponse, ErrorResult& aRv)
>-      !aResponse.Length()) {
>+      !CopyArrayBufferOrArrayBufferViewData(aResponse, data)) {

This loses the length != 0 check.  Why?

>+CopyIntoNewJsArrayBuffer(JSContext *aCx,

Why is this not using ArrayBuffer::Create?  It should be.

>+  Promise<void> update((ArrayBuffer or ArrayBufferView) response);

It might be nice to write this as "(ArrayBufferView or ArrayBuffer)" since we already have that union type in webcrypto so we wouldn't generate extra union code...

Same in the other spots.

r=me with the first two issues dealt with and the third at least thought about.
Attachment #8478056 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8478056 [details] [diff] [review]
> Patch
> 
> >+MediaKeySession::Update(const ArrayBufferOrArrayBufferView& aResponse, ErrorResult& aRv)
> >-      !aResponse.Length()) {
> >+      !CopyArrayBufferOrArrayBufferViewData(aResponse, data)) {
> 
> This loses the length != 0 check.  Why?

Oops! I'll make CopyIntoNewJsArrayBuffer return failure if the buffer-or-view is 0 length, as all callers require this behaviour as per the spec. Thanks!


> >+CopyIntoNewJsArrayBuffer(JSContext *aCx,
> 
> Why is this not using ArrayBuffer::Create?  It should be.

You mean ArrayBufferObject::create? I'll try to make that work.

> 
> >+  Promise<void> update((ArrayBuffer or ArrayBufferView) response);
> 
> It might be nice to write this as "(ArrayBufferView or ArrayBuffer)" since
> we already have that union type in webcrypto so we wouldn't generate extra
> union code...

Will do!
Comment on attachment 8478056 [details] [diff] [review]
Patch

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

::: content/media/eme/MediaKeys.cpp
@@ +95,2 @@
>    nsRefPtr<Promise> promise(MakePromise(aRv));
> +  if (aRv.Failed() || !CopyArrayBufferOrArrayBufferViewData(aCert, data)) {

D'oh! This should only return nullptr if we can't create the promise, and reject the promise on failure.

Ditto for CreateSession below.
> You mean ArrayBufferObject::create?

No.  I meant what I said.  mozilla::dom::ArrayBuffer::Create, from mozilla/dom/TypedArray.h
https://hg.mozilla.org/mozilla-central/rev/f505272bac50
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.