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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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).

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

Update instances of Uint8Aarry in EME interfaces to (ArrayBuffer or ArrayBufferView), as that's what's in the current draft spec.

Comment 2

[Boris Zbarsky [: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.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> You mean ArrayBufferObject::create?

No.  I meant what I said.  mozilla::dom::ArrayBuffer::Create, from mozilla/dom/TypedArray.h

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f505272bac50

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f505272bac50

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Mass update firefox-status to track EME uplift.