Add new needkey event for EME.

Add new needkey event for EME.

EME WD uses 'needkey' event (EME v0.1b was 'webkitneedkey').
This change adds support for the new event while retaining
support for the previous event.

BUG=224786
TEST=new test added

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

[jrummell, 2014-01-11 00:34:25]

PTAL.

[acolwell GONE FROM CHROMIUM, 2014-01-11 01:11:24]

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1713: bool
HTMLMediaElement::mediaPlayerKeyNeeded(const String& mimeType, const unsigned
char* initData, unsigned initDataLength)
nit: s/mimeType/contentType/

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1734: if
(hasEventListeners(EventTypeNames::needkey)) {
Do we really want to dispatch to both events? ISTM that we should dispatch
'needkey' if there are listeners for that OR 'webkitneedkey' if there are no
'needkey' listeners. That way an application that registers both, because they
want to work in prefixed and unprefixed worlds, will only get one notification.

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1744: result = true;
If you take my suggestion above then you should be able to return early here and
above.

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:363: virtual bool mediaPlayerKeyNeeded(const
String& mimeType, const unsigned char* initData, unsigned initDataLength)
OVERRIDE FINAL;
nit: s/mimeType/contentType/ to match all the other related changes.

https://codereview.chromium.org/134663005/diff/1/Source/platform/graphics/med...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/134663005/diff/1/Source/platform/graphics/med...
Source/platform/graphics/media/MediaPlayer.h:95: virtual bool
mediaPlayerKeyNeeded(const String& /* mimeType */, const unsigned char* /*
initData */, unsigned /* initDataLength */) = 0;
nit: s/mimeType/contentType/

[jrummell, 2014-01-11 01:56:56]

Updated with test. PTAL.

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1713: bool
HTMLMediaElement::mediaPlayerKeyNeeded(const String& mimeType, const unsigned
char* initData, unsigned initDataLength)
On 2014/01/11 01:11:25, acolwell wrote:
> nit: s/mimeType/contentType/

Done.

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1734: if
(hasEventListeners(EventTypeNames::needkey)) {
On 2014/01/11 01:11:25, acolwell wrote:
> Do we really want to dispatch to both events? ISTM that we should dispatch
> 'needkey' if there are listeners for that OR 'webkitneedkey' if there are no
> 'needkey' listeners. That way an application that registers both, because they
> want to work in prefixed and unprefixed worlds, will only get one
notification.

Good idea. Reordered so unprefixed event is preferred.

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:1744: result = true;
On 2014/01/11 01:11:25, acolwell wrote:
> If you take my suggestion above then you should be able to return early here
and
> above.

Done.

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/134663005/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:363: virtual bool mediaPlayerKeyNeeded(const
String& mimeType, const unsigned char* initData, unsigned initDataLength)
OVERRIDE FINAL;
On 2014/01/11 01:11:25, acolwell wrote:
> nit: s/mimeType/contentType/ to match all the other related changes.

Done.

https://codereview.chromium.org/134663005/diff/1/Source/platform/graphics/med...
File Source/platform/graphics/media/MediaPlayer.h (right):

https://codereview.chromium.org/134663005/diff/1/Source/platform/graphics/med...
Source/platform/graphics/media/MediaPlayer.h:95: virtual bool
mediaPlayerKeyNeeded(const String& /* mimeType */, const unsigned char* /*
initData */, unsigned /* initDataLength */) = 0;
On 2014/01/11 01:11:25, acolwell wrote:
> nit: s/mimeType/contentType/

Done.

[acolwell GONE FROM CHROMIUM, 2014-01-13 18:19:04]

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-needkey.html (right):

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:9: var arr=[];
nit: Add spaces around '=' here and elsewhere

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:10: for(var
i=0,j=str.length;i<j;++i)
nit: add spaces around '<'

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:26:
run('mediaKeys = new MediaKeys("org.w3.clearkey")');
nit: Why is this and the line below in a run()? Seems unnecessary.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1734: initializer.sessionId = String();
Where these always empty strings before?

https://codereview.chromium.org/134663005/diff/80001/public/platform/WebMedia...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/134663005/diff/80001/public/platform/WebMedia...
public/platform/WebMediaPlayerClient.h:70: virtual void keyNeeded(const
WebString& keySystem, const WebString& sessionId, const unsigned char* initData,
unsigned initDataLength) = 0;
I think you should put a default implementation that forwards to the method
below here instead of in WebMediaPlayerClientImpl so that you can remove this
method on the Chromium side when you land the changes that start calling the
method below instead of this one. That should save you a CL.

[xhwang, 2014-01-13 18:42:40]

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-needkey.html (right):

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:28:
video.setMediaKeys(mediaKeys);
Based on the spec, we don't need a MediaKeys object for the needkey event. Can
we remove the above three lines?

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1715: // Send event for non-prefixed EME,
if available.
ditto, replace non-prefixed/prefixed to EME version.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:150: // encrypted media extensions
(prefixed)
s/prefixed/v0.1b/ ?

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:162: // encrypted media extensions
(non-prefixed)
s/non-prefixed/WD/ ?

[jrummell, 2014-01-13 19:01:31]

Updated.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-needkey.html (right):

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:9: var arr=[];
On 2014/01/13 18:19:04, acolwell wrote:
> nit: Add spaces around '=' here and elsewhere

Done. This function (and most of the code) was copied from
encrypted-media-v2-events.html.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:10: for(var
i=0,j=str.length;i<j;++i)
On 2014/01/13 18:19:04, acolwell wrote:
> nit: add spaces around '<'

Done.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:26:
run('mediaKeys = new MediaKeys("org.w3.clearkey")');
On 2014/01/13 18:19:04, acolwell wrote:
> nit: Why is this and the line below in a run()? Seems unnecessary.

run() logs the statement and catches exceptions. It looks like most of the tests
in this directory wrap all calls using run(), so I've updated the remaining line
below that doesn't.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:28:
video.setMediaKeys(mediaKeys);
On 2014/01/13 18:42:40, xhwang wrote:
> Based on the spec, we don't need a MediaKeys object for the needkey event. Can
> we remove the above three lines?

Done.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1715: // Send event for non-prefixed EME,
if available.
On 2014/01/13 18:42:40, xhwang wrote:
> ditto, replace non-prefixed/prefixed to EME version.

Done.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1734: initializer.sessionId = String();
On 2014/01/13 18:19:04, acolwell wrote:
> Where these always empty strings before?

Yup.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
is "client_->keyNeeded(WebString(), WebString(), init_data_ptr,
init_data.size());".

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:150: // encrypted media extensions
(prefixed)
On 2014/01/13 18:42:40, xhwang wrote:
> s/prefixed/v0.1b/ ?

Done.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:162: // encrypted media extensions
(non-prefixed)
On 2014/01/13 18:42:40, xhwang wrote:
> s/non-prefixed/WD/ ?

Done.

https://codereview.chromium.org/134663005/diff/80001/public/platform/WebMedia...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/134663005/diff/80001/public/platform/WebMedia...
public/platform/WebMediaPlayerClient.h:70: virtual void keyNeeded(const
WebString& keySystem, const WebString& sessionId, const unsigned char* initData,
unsigned initDataLength) = 0;
On 2014/01/13 18:19:04, acolwell wrote:
> I think you should put a default implementation that forwards to the method
> below here instead of in WebMediaPlayerClientImpl so that you can remove this
> method on the Chromium side when you land the changes that start calling the
> method below instead of this one. That should save you a CL.

Done.

[ddorwin, 2014-01-13 19:08:37]

Publishing comments in PS2. I'll nowmove to PS3.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-needkey.html (right):

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:1: <!DOCTYPE
html>
We should start writing tests in a W3C test-compatible way. This is fine, but
let's not write more like this. We should look at how to convert this and
others.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:9: var arr=[];
add spaces to all of this

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:17: var initData
= stringToUint8Array('mock');
Why "mock"?

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:29:
video.src="../content/test-encrypted.webm";
spaces

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:31: // Will get 2
identical events, one for audio, one for video.
Would this comment be better above line 21?

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:41: // Following
line commented out as contentType not currently set.
FIXME: Enable the following line when...

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1716: if
(hasEventListeners(EventTypeNames::needkey)) {
The check this copies was related to part of the prefixed spec that fired an
error if needkey was not present. This does not exist in the WD, so we should
always fire.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1730: // FIXME: Remove once prefixed EME
is removed.
Probably not necessary. We'll know to search for and delete this code.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1745: // Unable to send an event, so send
an error.
The error doesn't exist in the WD, and we never really used it in the prefixed
API. While we could figure out when to fire this, I'd say to just remove it
rather than adding the extra logic.

[ddorwin, 2014-01-13 19:17:49]

https://codereview.chromium.org/134663005/diff/200001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/200001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1726: return true;
We shouldn't return. We may need to fire both events. (Really, we should just
remove the if block altogether per my comments in PS2.

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:72: // FIXME: remove after callers change
to pass contentType.
Should be above 70, right?

[acolwell GONE FROM CHROMIUM, 2014-01-13 19:19:38]

lgtm

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-needkey.html (right):

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:26:
run('mediaKeys = new MediaKeys("org.w3.clearkey")');
On 2014/01/13 19:01:32, jrummell wrote:
> On 2014/01/13 18:19:04, acolwell wrote:
> > nit: Why is this and the line below in a run()? Seems unnecessary.
> 
> run() logs the statement and catches exceptions. It looks like most of the
tests
> in this directory wrap all calls using run(), so I've updated the remaining
line
> below that doesn't.

Ok. I'm not a fan of this style, but since it appears to be the local convention
in this directory this is fine.

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:72: // FIXME: remove after callers change
to pass contentType.
nit: Move FIXME to above method and s/remove/Remove this method/ so it is clear
that we want this whole method removed.

[jrummell, 2014-01-13 22:43:43]

Updated.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-needkey.html (right):

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:9: var arr=[];
On 2014/01/13 19:08:38, ddorwin wrote:
> add spaces to all of this

Done.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:17: var initData
= stringToUint8Array('mock');
On 2014/01/13 19:08:38, ddorwin wrote:
> Why "mock"?

Clone from encrypted-media-v2-events.html. Now that we're not calling
createSession(), it's not needed anymore.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:29:
video.src="../content/test-encrypted.webm";
On 2014/01/13 19:08:38, ddorwin wrote:
> spaces

Done.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:31: // Will get 2
identical events, one for audio, one for video.
On 2014/01/13 19:08:38, ddorwin wrote:
> Would this comment be better above line 21?

Sure.

https://codereview.chromium.org/134663005/diff/80001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-needkey.html:41: // Following
line commented out as contentType not currently set.
On 2014/01/13 19:08:38, ddorwin wrote:
> FIXME: Enable the following line when...

Done.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1716: if
(hasEventListeners(EventTypeNames::needkey)) {
On 2014/01/13 19:08:38, ddorwin wrote:
> The check this copies was related to part of the prefixed spec that fired an
> error if needkey was not present. This does not exist in the WD, so we should
> always fire.

Done.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1730: // FIXME: Remove once prefixed EME
is removed.
On 2014/01/13 19:08:38, ddorwin wrote:
> Probably not necessary. We'll know to search for and delete this code.

Done.

https://codereview.chromium.org/134663005/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1745: // Unable to send an event, so send
an error.
On 2014/01/13 19:08:38, ddorwin wrote:
> The error doesn't exist in the WD, and we never really used it in the prefixed
> API. While we could figure out when to fire this, I'd say to just remove it
> rather than adding the extra logic.

Done.

https://codereview.chromium.org/134663005/diff/200001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/134663005/diff/200001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:1726: return true;
On 2014/01/13 19:17:49, ddorwin wrote:
> We shouldn't return. We may need to fire both events. (Really, we should just
> remove the if block altogether per my comments in PS2.

Done.

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
File public/platform/WebMediaPlayerClient.h (right):

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:72: // FIXME: remove after callers change
to pass contentType.
On 2014/01/13 19:17:49, ddorwin wrote:
> Should be above 70, right?

Done.

https://codereview.chromium.org/134663005/diff/200001/public/platform/WebMedi...
public/platform/WebMediaPlayerClient.h:72: // FIXME: remove after callers change
to pass contentType.
On 2014/01/13 19:19:39, acolwell wrote:
> nit: Move FIXME to above method and s/remove/Remove this method/ so it is
clear
> that we want this whole method removed.

Done.

[abarth-chromium, 2014-01-14 04:03:32]

public/ LGTM

[xhwang, 2014-01-14 04:06:12]

lgtm

[commit-bot: I haz the power, 2014-01-14 17:38:28]

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

[commit-bot: I haz the power, 2014-01-14 19:04:50]

Change committed as 165087