Bug 82974 - Add Encrypted Media Extensions events to HTMLMediaElement
Summary: Add Encrypted Media Extensions events to HTMLMediaElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Dorwin
URL:
Keywords:
Depends on:
Blocks: 82968
  Show dependency treegraph
 
Reported: 2012-04-02 16:44 PDT by David Dorwin
Modified: 2012-04-13 10:24 PDT (History)
13 users (show)

See Also:


Attachments
Patch (83.82 KB, patch)
2012-04-03 12:33 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Patch (90.19 KB, patch)
2012-04-09 16:49 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Rebased after 82973 landed (90.12 KB, patch)
2012-04-11 16:25 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case) (90.15 KB, patch)
2012-04-11 17:35 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
defaultUrl => defaultURL (89.95 KB, patch)
2012-04-12 15:31 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Fixed ChangeLog (89.79 KB, patch)
2012-04-12 15:37 PDT, David Dorwin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Dorwin 2012-04-02 16:44:21 PDT
Add the new events as defined in http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media.html#events
Comment 1 David Dorwin 2012-04-03 12:33:31 PDT
Created attachment 135394 [details]
Patch
Comment 2 Adam Barth 2012-04-09 10:49:27 PDT
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
Comment 3 Adam Barth 2012-04-09 10:51:06 PDT
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).
Comment 4 David Dorwin 2012-04-09 16:47:08 PDT
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
Comment 5 David Dorwin 2012-04-09 16:49:28 PDT
Created attachment 136344 [details]
Patch
Comment 6 David Dorwin 2012-04-11 16:25:33 PDT
Created attachment 136782 [details]
Rebased after 82973 landed
Comment 7 WebKit Review Bot 2012-04-11 16:30:19 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 8 Adam Barth 2012-04-11 16:47:57 PDT
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.)
Comment 9 WebKit Review Bot 2012-04-11 17:24:17 PDT
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
Comment 10 David Dorwin 2012-04-11 17:35:16 PDT
Created attachment 136797 [details]
Avoid using uninitialized mediaKeyErrorCode (exposed by removing default case)
Comment 11 Adam Barth 2012-04-12 14:49:51 PDT
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.)
Comment 12 David Dorwin 2012-04-12 15:30:36 PDT
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.
Comment 13 David Dorwin 2012-04-12 15:31:55 PDT
Created attachment 136983 [details]
defaultUrl => defaultURL
Comment 14 David Dorwin 2012-04-12 15:37:57 PDT
Created attachment 136985 [details]
Fixed ChangeLog
Comment 15 Adam Barth 2012-04-12 15:53:47 PDT
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.
Comment 16 WebKit Review Bot 2012-04-12 18:32:54 PDT
Comment on attachment 136985 [details]
Fixed ChangeLog

Clearing flags on attachment: 136985

Committed r114067: <http://trac.webkit.org/changeset/114067>
Comment 17 WebKit Review Bot 2012-04-12 18:33:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Simon Fraser (smfr) 2012-04-13 08:34:07 PDT
This changed the results of the fast/dom/Window/window-property-descriptors.html test, which is now broken on the bots.
Comment 19 David Dorwin 2012-04-13 09:28:21 PDT
Simon, which bots? I clicked a few and didn't see that test failing. Can you provide a link? Thanks.
Comment 21 Adam Barth 2012-04-13 10:24:33 PDT
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.