Update MediaKeySession events to match latest EME spec

Update MediaKeySession events to match latest EME spec

The current prefixed event names used (e.g. webkitkeymessage, etc.) don't match
the event names in the latest EME spec, so this change updates them.  Also add
new event 'close', which wasn't specified in earlier versions of the spec.

BUG=224786
TEST=content_shell tests updated with new event names (and pass)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165023

[jrummell, 2014-01-03 23:27:41]

PTAL.

[xhwang, 2014-01-03 23:38:20]

Thanks!

acolwell: Please see my question about the "webkit" prefix.

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.idl (left):

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.idl:41:
[RuntimeEnabled=EncryptedMedia] attribute EventHandler onwebkitkeyadded;
Shall we keep the "webkit" prefix? I thought we have that prefix because EME is
not a standard yet, but I could be wrong.

+acolwell: In which case should we use "webkit" prefix, and at what point we can
drop it?

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
File Source/platform/drm/ContentDecryptionModuleSession.cpp (right):

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.cpp:85: m_client->keyReady();
Regarding my previous comment,

m_client->onReady() seems funny, how about m_client->onSessionReady()?

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
File Source/platform/drm/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:55: virtual void
keyError(MediaKeyErrorCode, unsigned long systemCode) = 0;
Here and everywhere, we should drop the "Key" part. OnError() or
OnSessionError()?

[acolwell GONE FROM CHROMIUM, 2014-01-06 20:34:10]

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.idl (left):

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.idl:41:
[RuntimeEnabled=EncryptedMedia] attribute EventHandler onwebkitkeyadded;
On 2014/01/03 23:38:20, xhwang wrote:
> Shall we keep the "webkit" prefix? I thought we have that prefix because EME
is
> not a standard yet, but I could be wrong.
> 
> +acolwell: In which case should we use "webkit" prefix, and at what point we
can
> drop it?

We should not be using a webkit prefix on new APIs. The idea is that things
should stay behind a runtime flag until they are stable enough for release.

[ddorwin, 2014-01-07 23:44:41]

https://codereview.chromium.org/124253003/diff/1/Source/core/events/EventType...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/124253003/diff/1/Source/core/events/EventType...
Source/core/events/EventTypeNames.in:48: closed
Or maybe not... This change inspired
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24227. :)

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:73:
DEFINE_ATTRIBUTE_EVENT_LISTENER(error);
on* attributes do not exist in the WD, except for needkey, where it is necessary
to handle declarative cases. (Just FYI, using addEventListener() is preferred to
using such attributes.)

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:87: virtual void
keyError(MediaKeyErrorCode, unsigned long systemCode) OVERRIDE;
I think we should drop "key" here.

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.idl (right):

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.idl:40: // EventHandlers
Delete all.

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
File Source/platform/drm/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:55: virtual void
keyError(MediaKeyErrorCode, unsigned long systemCode) = 0;
On 2014/01/03 23:38:20, xhwang wrote:
> Here and everywhere, we should drop the "Key" part.
+1
> OnError() or OnSessionError()?
"Session" is redundant. Is "On" common in Blink?
Looking at WebMediaPlayerClient.h, it seems not, but I don't know if that's a
good example.

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:57: virtual void keyReady()
= 0;
These had been in alphabetical order. The Chromium code now uses "lifecycle"
order. The EME spec had been alphabetical before "closed" was added, but I think
I'll reorder it to "lifecycle", which MSE appears to use.

In summary, I think we should use "lifecycle" order. WDYT? This probably applies
to other files in this CL too.

[jrummell, 2014-01-08 18:05:51]

Updated. PTAL.

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:73:
DEFINE_ATTRIBUTE_EVENT_LISTENER(error);
On 2014/01/07 23:44:41, ddorwin wrote:
> on* attributes do not exist in the WD, except for needkey, where it is
necessary
> to handle declarative cases. (Just FYI, using addEventListener() is preferred
to
> using such attributes.)

Removed.

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:87: virtual void
keyError(MediaKeyErrorCode, unsigned long systemCode) OVERRIDE;
On 2014/01/07 23:44:41, ddorwin wrote:
> I think we should drop "key" here.

Done.

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeySession.idl (right):

https://codereview.chromium.org/124253003/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.idl:40: // EventHandlers
On 2014/01/07 23:44:41, ddorwin wrote:
> Delete all.

Done.

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
File Source/platform/drm/ContentDecryptionModuleSession.cpp (right):

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.cpp:85: m_client->keyReady();
On 2014/01/03 23:38:20, xhwang wrote:
> Regarding my previous comment,
> 
> m_client->onReady() seems funny, how about m_client->onSessionReady()?

Removed 'key' from name.

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
File Source/platform/drm/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:55: virtual void
keyError(MediaKeyErrorCode, unsigned long systemCode) = 0;
On 2014/01/03 23:38:20, xhwang wrote:
> Here and everywhere, we should drop the "Key" part. OnError() or
> OnSessionError()?

Done. I couldn't find any examples of Onxxx anywhere in Blink.

https://codereview.chromium.org/124253003/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:57: virtual void keyReady()
= 0;
On 2014/01/07 23:44:41, ddorwin wrote:
> These had been in alphabetical order. The Chromium code now uses "lifecycle"
> order. The EME spec had been alphabetical before "closed" was added, but I
think
> I'll reorder it to "lifecycle", which MSE appears to use.
> 
> In summary, I think we should use "lifecycle" order. WDYT? This probably
applies
> to other files in this CL too.

Prefer lifecycle order, so moved Error() to end of list.

[ddorwin, 2014-01-08 18:12:16]

lgtm

[xhwang, 2014-01-09 01:16:20]

lgtm % one comment

ddorwin@: See the comment.

https://codereview.chromium.org/124253003/diff/210001/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/124253003/diff/210001/Source/core/events/Even...
Source/core/events/EventTypeNames.in:48: closed
Probably we'll rename the event to "close" very soon. Shall we just do it in
this CL?

+ddorwin

[ddorwin, 2014-01-09 01:37:10]

https://codereview.chromium.org/124253003/diff/210001/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/124253003/diff/210001/Source/core/events/Even...
Source/core/events/EventTypeNames.in:48: closed
On 2014/01/09 01:16:21, xhwang wrote:
> Probably we'll rename the event to "close" very soon. Shall we just do it in
> this CL?
> 
> +ddorwin

Probably. Better to add later than add and remove?

[jrummell, 2014-01-09 18:23:10]

Renamed 'closed' event to 'close'.

[xhwang, 2014-01-10 19:30:57]

https://codereview.chromium.org/124253003/diff/340001/Source/platform/drm/Con...
File Source/platform/drm/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/124253003/diff/340001/Source/platform/drm/Con...
Source/platform/drm/ContentDecryptionModuleSession.h:57: virtual void closed() =
0;
After https://codereview.chromium.org/111043004/ is landed, you can rename this
to "close" as well.

[acolwell GONE FROM CHROMIUM, 2014-01-10 20:43:10]

lgtm

https://codereview.chromium.org/124253003/diff/340001/Source/platform/drm/Con...
File Source/platform/drm/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/124253003/diff/340001/Source/platform/drm/Con...
Source/platform/drm/ContentDecryptionModuleSession.h:57: virtual void closed() =
0;
On 2014/01/10 19:30:57, xhwang wrote:
> After https://codereview.chromium.org/111043004/ is landed, you can rename
this
> to "close" as well.
It looks like this patch has landed. Does this mean you can do the rename now?

[jrummell, 2014-01-10 22:18:21]

Renamed closed() to close(). xhwang@ please do a final review before I add blink
owners.

https://codereview.chromium.org/124253003/diff/340001/Source/platform/drm/Con...
File Source/platform/drm/ContentDecryptionModuleSession.h (right):

https://codereview.chromium.org/124253003/diff/340001/Source/platform/drm/Con...
Source/platform/drm/ContentDecryptionModuleSession.h:57: virtual void closed() =
0;
On 2014/01/10 20:43:11, acolwell wrote:
> On 2014/01/10 19:30:57, xhwang wrote:
> > After https://codereview.chromium.org/111043004/ is landed, you can rename
> this
> > to "close" as well.
> It looks like this patch has landed. Does this mean you can do the rename now?

Done.

[xhwang, 2014-01-10 22:25:18]

lgtm % nit on the test

https://codereview.chromium.org/124253003/diff/530001/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-events.html (right):

https://codereview.chromium.org/124253003/diff/530001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:31: function
keyMessage(event)
Shall we also update this (and keyError)? Basically we do not use "Key" now.

https://codereview.chromium.org/124253003/diff/530001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:33:
run('mediaKeySession.update(invalidKey)');
OOC (not required in this CL, +ddorwin): why we use incalidkey upon session
message but use validkey upon session error?

[jrummell, 2014-01-10 22:52:57]

+abarth for OWNERS review of source/platform and public/platform

https://codereview.chromium.org/124253003/diff/530001/LayoutTests/media/encry...
File LayoutTests/media/encrypted-media/encrypted-media-v2-events.html (right):

https://codereview.chromium.org/124253003/diff/530001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:31: function
keyMessage(event)
On 2014/01/10 22:25:18, xhwang wrote:
> Shall we also update this (and keyError)? Basically we do not use "Key" now.

Done.

https://codereview.chromium.org/124253003/diff/530001/LayoutTests/media/encry...
LayoutTests/media/encrypted-media/encrypted-media-v2-events.html:33:
run('mediaKeySession.update(invalidKey)');
On 2014/01/10 22:25:18, xhwang wrote:
> OOC (not required in this CL, +ddorwin): why we use incalidkey upon session
> message but use validkey upon session error?

It appears this is just a way to get an error generated, in order to test all 3
events. Can't add 'close' event yet as it's not connected fully.

[abarth-chromium, 2014-01-13 23:07:08]

lgtm

https://codereview.chromium.org/124253003/diff/430002/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/124253003/diff/430002/Source/core/events/Even...
Source/core/events/EventTypeNames.in:209: webkitkeymessage
Can we remove some of these prefixed names?

[jrummell, 2014-01-13 23:09:09]

Thanks for the reviews.

https://codereview.chromium.org/124253003/diff/430002/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/124253003/diff/430002/Source/core/events/Even...
Source/core/events/EventTypeNames.in:209: webkitkeymessage
On 2014/01/13 23:07:08, abarth wrote:
> Can we remove some of these prefixed names?

These will be removed when we remove EME v0.1b support.

[commit-bot: I haz the power, 2014-01-13 23:10:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/124253003/430002

[commit-bot: I haz the power, 2014-01-14 04:27:17]

Change committed as 165023

[ddorwin, 2014-01-23 18:55:03]

https://codereview.chromium.org/124253003/diff/210001/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/124253003/diff/210001/Source/core/events/Even...
Source/core/events/EventTypeNames.in:48: closed
On 2014/01/09 01:37:10, ddorwin wrote:
> On 2014/01/09 01:16:21, xhwang wrote:
> > Probably we'll rename the event to "close" very soon. Shall we just do it in
> > this CL?
> > 
> > +ddorwin
> 
> Probably. Better to add later than add and remove?

FTR, the spec has been updated to "close":
https://dvcs.w3.org/hg/html-media/rev/413d03f8655c
No changes necessary since we used "close".