Bug 82974 – Add Encrypted Media Extensions events to HTMLMediaElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[David Dorwin]

Add the new events as defined in http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media.html#events

[David Dorwin]

Created attachment 135394 [details]
Patch

[Adam Barth]

Comment on attachment 135394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135394&action=review

All changes require a ChangeLog (See http://www.webkit.org/coding/contributing.html)

> Source/WebCore/bindings/v8/Dictionary.cpp:302
> +bool Dictionary::get(const String& key, RefPtr<Uint8Array>& value) const

This function should be outside the ENABLE(ENCRYPTED_MEDIA) ifdef.

> Source/WebCore/html/HTMLMediaElement.cpp:1742
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return;

Generally WebKit leaves off the default case so that the compiler will complain if we forget an enum.

> Source/WebCore/html/HTMLMediaElement.cpp:1765
> +    initializer.message = messageRef;

You should either say "messageRef.release() or just move the call to Uint8Array::create to this line.

> Source/WebCore/html/HTMLMediaElement.cpp:1780
> +    initializer.initData = initDataRef;

ditto

> Source/WebCore/html/MediaKeyEvent.cpp:42
> +

You've got an extra blank line here.

> Source/WebKit/chromium/public/WebMediaPlayerClient.h:71
> +    virtual void keyAdded(const WebKit::WebString&, const WebKit::WebString&) = 0;
> +    virtual void keyError(const WebKit::WebString&, const WebKit::WebString&, MediaKeyErrorCode, unsigned short systemCode) = 0;
> +    virtual void keyMessage(const WebKit::WebString&, const WebKit::WebString&, const unsigned char*, unsigned) = 0;
> +    virtual void keyNeeded(const WebKit::WebString&, const WebKit::WebString&, const unsigned char* initData, unsigned initDataLength) = 0;

We're already in the WebKit namespace, so you shouldn't need the "WebKit::" in WebKit::WebString

[Adam Barth]

Comment on attachment 135394 [details]
Patch

Very minor comments above.  The main problem is just the procedural issue of needing a ChangeLog.

We should have fishd look at the WebKit API parts of this patch (and likely of the other patch as well).

[David Dorwin]

Comment on attachment 135394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135394&action=review

>> Source/WebCore/bindings/v8/Dictionary.cpp:302
>> +bool Dictionary::get(const String& key, RefPtr<Uint8Array>& value) const
> 
> This function should be outside the ENABLE(ENCRYPTED_MEDIA) ifdef.

Done

>> Source/WebCore/html/HTMLMediaElement.cpp:1742
>> +        return;
> 
> Generally WebKit leaves off the default case so that the compiler will complain if we forget an enum.

Done. Without it, we will continue through the rest of this function. That could be handled, but it adds more code.

> Source/WebCore/html/HTMLMediaElement.cpp:1749
> +    initializer.errorCode = errorCodeRef;

Changed this one too.

>> Source/WebCore/html/HTMLMediaElement.cpp:1765
>> +    initializer.message = messageRef;
> 
> You should either say "messageRef.release() or just move the call to Uint8Array::create to this line.

Done

>> Source/WebCore/html/HTMLMediaElement.cpp:1780
>> +    initializer.initData = initDataRef;
> 
> ditto

Done.

>> Source/WebCore/html/MediaKeyEvent.cpp:42
>> +
> 
> You've got an extra blank line here.

Done

>> Source/WebKit/chromium/public/WebMediaPlayerClient.h:71
>> +    virtual void keyNeeded(const WebKit::WebString&, const WebKit::WebString&, const unsigned char* initData, unsigned initDataLength) = 0;
> 
> We're already in the WebKit namespace, so you shouldn't need the "WebKit::" in WebKit::WebString

Done

[David Dorwin]

Created attachment 136344 [details]
Patch

[David Dorwin]

Created attachment 136782 [details]
Rebased after 82973 landed

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 136782 [details]
Rebased after 82973 landed

This looks good on a first pass, but I'll need to go through it in more detail to actually review it.  (Other reviewers should feel free to review it in the meantime.)

[WebKit Review Bot]

Comment on attachment 136782 [details]
Rebased after 82973 landed

Attachment 136782 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12393057

[David Dorwin]

Created attachment 136797 [details]
Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case)

[Adam Barth]

Comment on attachment 136797 [details]
Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case)

View in context: https://bugs.webkit.org/attachment.cgi?id=136797&action=review

This looks pretty straightforward.  Thanks.

> Source/WebCore/html/MediaKeyEvent.idl:37
> +        readonly attribute [InitializedByEventConstructor] DOMString defaultUrl;

Is this really supposed to be defaultUrl and not defaultURL ?  I though capitalizing URL was idiomatic in the platform.  (I could be misremembering.)

[David Dorwin]

Comment on attachment 136797 [details]
Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case)

View in context: https://bugs.webkit.org/attachment.cgi?id=136797&action=review

>> Source/WebCore/html/MediaKeyEvent.idl:37
>> +        readonly attribute [InitializedByEventConstructor] DOMString defaultUrl;
> 
> Is this really supposed to be defaultUrl and not defaultURL ?  I though capitalizing URL was idiomatic in the platform.  (I could be misremembering.)

Looking at other WebKit IDL files, there is a mixture of capitalizations but most are "URL". 
The proposed spec uses the former but not for any good reason. I'll change it here and file a bug against the proposed spec.

[David Dorwin]

Created attachment 136983 [details]
defaultUrl => defaultURL

[David Dorwin]

Created attachment 136985 [details]
Fixed ChangeLog

[Adam Barth]

Comment on attachment 136985 [details]
Fixed ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=136985&action=review

> LayoutTests/fast/js/resources/js-test-pre.js:257
> +function shouldBeZero(_a) { shouldBe(_a, "0"); }

By the way, I really appreciate you improving this core scripts rather than making one-off solutions for just your tests.

[WebKit Review Bot]

Comment on attachment 136985 [details]
Fixed ChangeLog

Clearing flags on attachment: 136985

Committed r114067: <http://trac.webkit.org/changeset/114067>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This changed the results of the fast/dom/Window/window-property-descriptors.html test, which is now broken on the bots.

[David Dorwin]

Simon, which bots? I clicked a few and didn't see that test failing. Can you provide a link? Thanks.

[Simon Fraser (smfr)]

http://build.webkit.org/results/Lion%20Release%20(Tests)/r114135%20(7573)/fast/dom/Window/window-property-descriptors-pretty-diff.html

[Adam Barth]

http://build.webkit.org/results/Lion%20Release%20(Tests)/r114135%20(7573)/fast/dom/Window/window-property-descriptors-pretty-diff.html

Looks like this just needs a new baseline.  It's picking up the new functions you added to the test harness.