Implement MediaKeySession.generateRequest()

Implement MediaKeySession.generateRequest()

The latest EME spec has MediaKeys.createSession() return the session
object, and adds MediaKeySession.generateRequest() to actually call
the CDM with the necessary |initData| for the session.

This change also updates the layout tests to match the new way of
creating sessions.

BUG=358271
TEST=updated layout tests pass

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

[jrummell, 2014-09-05 19:02:52]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, xhwang@chromium.org

[jrummell, 2014-09-05 19:02:54]

PTAL.

[ddorwin, 2014-09-09 00:44:37]

Reviewed MKS code.

https://codereview.chromium.org/543173002/diff/1/LayoutTests/media/encrypted-...
File
LayoutTests/media/encrypted-media/encrypted-media-async-creation-with-gc.html
(right):

https://codereview.chromium.org/543173002/diff/1/LayoutTests/media/encrypted-...
LayoutTests/media/encrypted-media/encrypted-media-async-creation-with-gc.html:22:
}).then(function(result) {
There is no result value.

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

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:70: String codecs =
type.parameter("codecs");
nit: Do we need the local variable?

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:148: // completeWithSession()
will resolve the promise with undefined, while
"undefined" is not specified by the spec. Please ask the promise experts if we
need a V8VoidType() or a resolve() that implies void/no value.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:160: ~NewSessionResult()
If any superclass's destructor is virtual, make this too.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:180:
m_resolver->resolve(V8UndefinedType());
See the comment on 148.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:206:
RefPtr<ScriptPromiseResolver> m_resolver;
OOC, why is this still a RefPtr?

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:217:
MediaKeySession::MediaKeySession(ScriptState* scriptState, MediaKeys* keys,
const String& sessionType)
nit: media_keys. keys seems a bit ambiguous.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:219: ,
m_keySystem(keys->keySystem())
We should not be saving this - it's no longer a member of the JS object.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:222: , m_expiration(nan(""))
Why does this take a string?
FYI, I also see NaN() in double.h.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:235:
ASSERT(sessionId().isEmpty());
nit: If we are going to keep these as members, we should test the members
directly.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:291: // From
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...:
nit: Why the URL here? I don't think we have these URLs for other algorithms.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:299: scriptState,
DOMException::create(InvalidStateError, "The session already exists."));
s/exists/initialized/
OR s/already exists/has already been used/

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:320: //    (Done in
constructor.)
nit: This isn't actually about saving it as a member - it's creating a temporary
variable that is used in later steps.
You might say. (Use m_mediaKey, which was set in the constructor.)

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:326: if
(!isKeySystemSupportedWithInitDataType(m_keySystem, initDataType)) {
Per earlier comments and the algorithm, this should be m_keys->keySystem()

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:344: if
(!m_actionTimer.isActive())
I think this is an ASSERT - nothing else should have been able to start a timer.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:463: m_session =
adoptPtr(cdm->createSession());
We should do this during creation.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:499: m_sessionId =
m_session->sessionId();
ASSERT(!empty())
Also, I'm not sure we should save a copy. WDYT?

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:507: // FIXME: Is this
required?
Probably not: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26758

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:511: //      (Done by the
CDM).
Hmm. We need to somehow guarantee that this message does not get fired until
this code runs.

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

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:73: double expiration() const {
return m_expiration; }
Ideally, expiration should have been added in a separate CL. At least mention it
in the description.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:119: String m_keySystem;
This should be removed in a separate CL. It's no longer exposed by the IDL.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:126: // initialization).
Afterwards, a weak reference is all that is needed.
We discussed a different solution that didn't require this odd transition. The
solution was to instantiate the WebCdmSessionImpl when the MKS is created.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:128: WeakMember<MediaKeys>
m_keys;
Eventually, this should be m_mediaKeys - I think that naming is clearer.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:131: String m_sessionId;
Why is this being tracked here now?

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:134: bool m_uninitialized;
m_isUn...
ditto below.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:136: 
nit: Keep the states together, separate from the properties. You can move the
comments to the right of the variable.

[ddorwin, 2014-09-09 17:35:03]

Finished reviewing C++. The tests all appear to have the same issue with
|result|.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:207: // 2. Let session be a new
MediaKeySession object.
..., and initialize it as follows:
(Initialization is performed in the constructor.)

[jrummell, 2014-09-09 19:56:00]

Updated.

https://codereview.chromium.org/543173002/diff/1/LayoutTests/media/encrypted-...
File
LayoutTests/media/encrypted-media/encrypted-media-async-creation-with-gc.html
(right):

https://codereview.chromium.org/543173002/diff/1/LayoutTests/media/encrypted-...
LayoutTests/media/encrypted-media/encrypted-media-async-creation-with-gc.html:22:
}).then(function(result) {
On 2014/09/09 00:44:35, ddorwin wrote:
> There is no result value.

Done.

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

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:70: String codecs =
type.parameter("codecs");
On 2014/09/09 00:44:35, ddorwin wrote:
> nit: Do we need the local variable?

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:148: // completeWithSession()
will resolve the promise with undefined, while
On 2014/09/09 00:44:35, ddorwin wrote:
> "undefined" is not specified by the spec. Please ask the promise experts if we
> need a V8VoidType() or a resolve() that implies void/no value.

I asked yhirano@ a while back, and he responded "currently the V8 implementation
doesn't distinguish those (no parameter vs. one undefined parameter)."

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:160: ~NewSessionResult()
On 2014/09/09 00:44:36, ddorwin wrote:
> If any superclass's destructor is virtual, make this too.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:180:
m_resolver->resolve(V8UndefinedType());
On 2014/09/09 00:44:35, ddorwin wrote:
> See the comment on 148.

Added comment.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:206:
RefPtr<ScriptPromiseResolver> m_resolver;
On 2014/09/09 00:44:36, ddorwin wrote:
> OOC, why is this still a RefPtr?

ScriptPromiseResolver::create() returns PassRefPtr<>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:217:
MediaKeySession::MediaKeySession(ScriptState* scriptState, MediaKeys* keys,
const String& sessionType)
On 2014/09/09 00:44:36, ddorwin wrote:
> nit: media_keys. keys seems a bit ambiguous.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:219: ,
m_keySystem(keys->keySystem())
On 2014/09/09 00:44:36, ddorwin wrote:
> We should not be saving this - it's no longer a member of the JS object.

Will be done in a CL that cleans up this class.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:222: , m_expiration(nan(""))
On 2014/09/09 00:44:35, ddorwin wrote:
> Why does this take a string?
> FYI, I also see NaN() in double.h.

I just did a search for C++ nan, and it pointed to this class in <cmath>. But
this is a C++11 feature, so it should just use the other one (macro returning
float). double.h doesn't appear to be a standard C++ header (at least according
to cplusplus.com). Removed so that it can be added as a separate CL.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:235:
ASSERT(sessionId().isEmpty());
On 2014/09/09 00:44:36, ddorwin wrote:
> nit: If we are going to keep these as members, we should test the members
> directly.

Now that m_session is created above, member is gone.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:291: // From
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...:
On 2014/09/09 00:44:35, ddorwin wrote:
> nit: Why the URL here? I don't think we have these URLs for other algorithms.

Just followed the existing pattern in update() and release() below.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:299: scriptState,
DOMException::create(InvalidStateError, "The session already exists."));
On 2014/09/09 00:44:36, ddorwin wrote:
> s/exists/initialized/
> OR s/already exists/has already been used/

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:320: //    (Done in
constructor.)
On 2014/09/09 00:44:36, ddorwin wrote:
> nit: This isn't actually about saving it as a member - it's creating a
temporary
> variable that is used in later steps.
> You might say. (Use m_mediaKey, which was set in the constructor.)

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:326: if
(!isKeySystemSupportedWithInitDataType(m_keySystem, initDataType)) {
On 2014/09/09 00:44:35, ddorwin wrote:
> Per earlier comments and the algorithm, this should be m_keys->keySystem()

But then we would have the issue that it is possible that m_keys has already
been garbage collected by the time we get here.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:344: if
(!m_actionTimer.isActive())
On 2014/09/09 00:44:36, ddorwin wrote:
> I think this is an ASSERT - nothing else should have been able to start a
timer.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:463: m_session =
adoptPtr(cdm->createSession());
On 2014/09/09 00:44:36, ddorwin wrote:
> We should do this during creation.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:499: m_sessionId =
m_session->sessionId();
On 2014/09/09 00:44:35, ddorwin wrote:
> ASSERT(!empty())
> Also, I'm not sure we should save a copy. WDYT?

sessionId() now calls this function as needed.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:507: // FIXME: Is this
required?
On 2014/09/09 00:44:35, ddorwin wrote:
> Probably not: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26758

Acknowledged.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:511: //      (Done by the
CDM).
On 2014/09/09 00:44:35, ddorwin wrote:
> Hmm. We need to somehow guarantee that this message does not get fired until
> this code runs.

Added ASSERT in message() to verify this.

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

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:73: double expiration() const {
return m_expiration; }
On 2014/09/09 00:44:36, ddorwin wrote:
> Ideally, expiration should have been added in a separate CL. At least mention
it
> in the description.

Removed since it's not used. Adding expiration will come in a seperate CL.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:119: String m_keySystem;
On 2014/09/09 00:44:36, ddorwin wrote:
> This should be removed in a separate CL. It's no longer exposed by the IDL.

Acknowledged.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:126: // initialization).
Afterwards, a weak reference is all that is needed.
On 2014/09/09 00:44:36, ddorwin wrote:
> We discussed a different solution that didn't require this odd transition. The
> solution was to instantiate the WebCdmSessionImpl when the MKS is created.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:128: WeakMember<MediaKeys>
m_keys;
On 2014/09/09 00:44:36, ddorwin wrote:
> Eventually, this should be m_mediaKeys - I think that naming is clearer.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:131: String m_sessionId;
On 2014/09/09 00:44:36, ddorwin wrote:
> Why is this being tracked here now?

m_session wasn't being created until the generateRequest() call, so it needed
some value until then. Fixed.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:134: bool m_uninitialized;
On 2014/09/09 00:44:36, ddorwin wrote:
> m_isUn...
> ditto below.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:136: 
On 2014/09/09 00:44:36, ddorwin wrote:
> nit: Keep the states together, separate from the properties. You can move the
> comments to the right of the variable.

Done.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:207: // 2. Let session be a new
MediaKeySession object.
On 2014/09/09 17:35:03, ddorwin wrote:
> ..., and initialize it as follows:
> (Initialization is performed in the constructor.)

Done.

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:506: //      (Done by call in
sessionId()).
Oops. Forgot to add the ASSERT. Done locally, will be in next PS.

[ddorwin, 2014-09-09 21:35:25]

Reviewed everything. Some replies in the previous PS.

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

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:222: , m_expiration(nan(""))
On 2014/09/09 19:55:59, jrummell wrote:
> On 2014/09/09 00:44:35, ddorwin wrote:
> > Why does this take a string?
> > FYI, I also see NaN() in double.h.
> 
> I just did a search for C++ nan, and it pointed to this class in <cmath>. But
> this is a C++11 feature, so it should just use the other one (macro returning
> float). double.h doesn't appear to be a standard C++ header (at least
according
> to http://cplusplus.com). Removed so that it can be added as a separate CL.

double.h is in Blink's WTF IIRC.

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:326: if
(!isKeySystemSupportedWithInitDataType(m_keySystem, initDataType)) {
On 2014/09/09 19:55:59, jrummell wrote:
> On 2014/09/09 00:44:35, ddorwin wrote:
> > Per earlier comments and the algorithm, this should be m_keys->keySystem()
> 
> But then we would have the issue that it is possible that m_keys has already
> been garbage collected by the time we get here.

Good point! I probably need to update the spec too:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24771#c5

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html
(right):

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html:51:
return mediaKeySession;
Does this implicitly return a resolved promise with the session?

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:128: //
shortened name, the error message for the last couple of
Is this still true? These tests aren't using mk#.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:131: // Too few
parameters.
These should be copied to generateRequests tests.
Then this comment should be updated to "Invalid parameter."

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:158: func:
function(mk) { var initData = stringToUint8Array('init data'); return
mk.createSession(initData); }
Is this just trying to test an invalid type? Would return mk.createSession(new
Uint8Array(16)) be sufficient? Of course, we don't have such a test for
generateRequest() below. It's probably not necessary since I don't think an
empty array would be a type error if a non-empty one was not. Maybe we should
just make the empty array tests a non-empty array.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:169: // FIXME:
Remove "video/" from the calls to isTypeSupported() once it is updated.
I don't think this comment applies to this case.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:312: // Added
index to each variable name as otherwise the first 60
Still true and necessary?

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:326: func:
function(mk3) { return mk3.createSession(undefined); },
Hmm. It seems this should fail. Maybe our IDL infrastructure is not as strict as
it should be. Dan and I were discussing this type of thing yesterday.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:352: if
(mediaKeySession) {
If not supported, it will throw! That means this must be a try/catch instead.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:361:
assert_equals(typeof mediaKeySession.onopen, 'undefined');
As part of the cleanup CL, the event names should be updated.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:390: if
(mediaKeySession) {
ditto

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:181: // this has the correct
effect.
We should probably have a Blink bug to support Promise<void>. It could then be
referenced here.

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:239:
m_session->setClientInterface(this);
cleanup: Can we now pass the client to createSession() rather than having a
separate setter?

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:529: ASSERT(m_isCallable);
It may not be obvious why this is necessary. We should add:
<< "'message' fired before session initialization completed.";
Does this macro accept a stream like DCHECK? If not, comment this.

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

Updated.

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

https://codereview.chromium.org/543173002/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:222: , m_expiration(nan(""))
On 2014/09/09 21:35:24, ddorwin wrote:
> On 2014/09/09 19:55:59, jrummell wrote:
> > On 2014/09/09 00:44:35, ddorwin wrote:
> > > Why does this take a string?
> > > FYI, I also see NaN() in double.h.
> > 
> > I just did a search for C++ nan, and it pointed to this class in <cmath>.
But
> > this is a C++11 feature, so it should just use the other one (macro
returning
> > float). double.h doesn't appear to be a standard C++ header (at least
> according
> > to http://cplusplus.com). Removed so that it can be added as a separate CL.
> 
> double.h is in Blink's WTF IIRC.

But that is for "class Double", which is different than the compiler "double".
You can convert back and forth, but that seems more complicated than it needs to
be.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
File
LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html
(right):

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-lifetime-multiple-mediakeys.html:51:
return mediaKeySession;
On 2014/09/09 21:35:24, ddorwin wrote:
> Does this implicitly return a resolved promise with the session?

Correct. Otherwise I'd need to find a way to pass back the newly created
session.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:128: //
shortened name, the error message for the last couple of
On 2014/09/09 21:35:24, ddorwin wrote:
> Is this still true? These tests aren't using mk#.

The last sentence is false (for this group), so I removed it. Maybe I should
remove this comment completely, since it appears that every test case uses short
names (rather than mediaKeys, mediaKeySession, etc.). WDYT?

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:131: // Too few
parameters.
On 2014/09/09 21:35:24, ddorwin wrote:
> These should be copied to generateRequests tests.
> Then this comment should be updated to "Invalid parameter."

Done.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:158: func:
function(mk) { var initData = stringToUint8Array('init data'); return
mk.createSession(initData); }
On 2014/09/09 21:35:24, ddorwin wrote:
> Is this just trying to test an invalid type? Would return mk.createSession(new
> Uint8Array(16)) be sufficient? Of course, we don't have such a test for
> generateRequest() below. It's probably not necessary since I don't think an
> empty array would be a type error if a non-empty one was not. Maybe we should
> just make the empty array tests a non-empty array.

Removed, since it is a TypeError and the previous case covers Uint8Array.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:169: // FIXME:
Remove "video/" from the calls to isTypeSupported() once it is updated.
On 2014/09/09 21:35:24, ddorwin wrote:
> I don't think this comment applies to this case.

Done.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:312: // Added
index to each variable name as otherwise the first 60
On 2014/09/09 21:35:24, ddorwin wrote:
> Still true and necessary?

Nope. Removed.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:326: func:
function(mk3) { return mk3.createSession(undefined); },
On 2014/09/09 21:35:24, ddorwin wrote:
> Hmm. It seems this should fail. Maybe our IDL infrastructure is not as strict
as
> it should be. Dan and I were discussing this type of thing yesterday.

In the previous discussion resolve(void) and resolve(undefined) are expected to
be equivalent. So I suspect that this become createSession(), which works with
the default value.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:352: if
(mediaKeySession) {
On 2014/09/09 21:35:24, ddorwin wrote:
> If not supported, it will throw! That means this must be a try/catch instead.

Done.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:361:
assert_equals(typeof mediaKeySession.onopen, 'undefined');
On 2014/09/09 21:35:24, ddorwin wrote:
> As part of the cleanup CL, the event names should be updated.

Acknowledged.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:390: if
(mediaKeySession) {
On 2014/09/09 21:35:24, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:181: // this has the correct
effect.
On 2014/09/09 21:35:25, ddorwin wrote:
> We should probably have a Blink bug to support Promise<void>. It could then be
> referenced here.

There is resolve(), so switched to that and removed comment.

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:239:
m_session->setClientInterface(this);
On 2014/09/09 21:35:25, ddorwin wrote:
> cleanup: Can we now pass the client to createSession() rather than having a
> separate setter?

The code is still there on the Chromium side, but not in the blink API. Will do
in a separate CL.

https://codereview.chromium.org/543173002/diff/20001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:529: ASSERT(m_isCallable);
On 2014/09/09 21:35:24, ddorwin wrote:
> It may not be obvious why this is necessary. We should add:
> << "'message' fired before session initialization completed.";
> Does this macro accept a stream like DCHECK? If not, comment this.

ASSERT is a macro, but added comment.

[ddorwin, 2014-09-10 02:30:52]

LGTM % comments.

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/543173002/diff/20001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:128: //
shortened name, the error message for the last couple of
On 2014/09/10 01:18:25, jrummell wrote:
> On 2014/09/09 21:35:24, ddorwin wrote:
> > Is this still true? These tests aren't using mk#.
> 
> The last sentence is false (for this group), so I removed it. Maybe I should
> remove this comment completely, since it appears that every test case uses
short
> names (rather than mediaKeys, mediaKeySession, etc.). WDYT?

If they aren't cut off, then remove the false statement.

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:129: // Too few
parameters.
Invalid parameter.

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:214: func:
function(mk8) { return mk8.createSession().generateRequest('TEMPORARY'); }
This one is probably unnecessary - we already test a string.

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:416: return
mediaKeySession.generateRequest(type, initData);
nit: It would be nice if the tests were consistent. IOW, move this below the
catch.

https://codereview.chromium.org/543173002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/543173002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:502: //      (Done by call in
sessionId()).
What does this mean? Did you use the wrong method name?

[jrummell, 2014-09-10 18:11:35]

Thanks for the reviews.

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
File LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html (right):

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:129: // Too few
parameters.
On 2014/09/10 02:30:51, ddorwin wrote:
> Invalid parameter.

Done.

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:214: func:
function(mk8) { return mk8.createSession().generateRequest('TEMPORARY'); }
On 2014/09/10 02:30:51, ddorwin wrote:
> This one is probably unnecessary - we already test a string.

Done.

https://codereview.chromium.org/543173002/diff/40001/LayoutTests/media/encryp...
LayoutTests/media/encrypted-media/encrypted-media-v2-syntax.html:416: return
mediaKeySession.generateRequest(type, initData);
On 2014/09/10 02:30:51, ddorwin wrote:
> nit: It would be nice if the tests were consistent. IOW, move this below the
> catch.

Done.

https://codereview.chromium.org/543173002/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/543173002/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:502: //      (Done by call in
sessionId()).
On 2014/09/10 02:30:51, ddorwin wrote:
> What does this mean? Did you use the wrong method name?

Removed comment.

[jrummell, 2014-09-10 18:11:39]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-09-10 18:12:07]

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

[commit-bot: I haz the power, 2014-09-10 18:12:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 18:12:21]

Failed to apply patch for Source/modules/encryptedmedia/MediaKeySession.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/encryptedmedia/MediaKeySession.cpp
  Hunk #3 FAILED at 124.
  Hunk #4 succeeded at 381 (offset -1 lines).
  Hunk #5 succeeded at 550 (offset -1 lines).
  Hunk #6 succeeded at 579 (offset -1 lines).
  Hunk #7 succeeded at 591 (offset -1 lines).
  Hunk #8 succeeded at 634 (offset -1 lines).
  Hunk #9 succeeded at 724 (offset -1 lines).
  Hunk #10 succeeded at 749 (offset -1 lines).
  1 out of 10 hunks FAILED -- saving rejects to file
Source/modules/encryptedmedia/MediaKeySession.cpp.rej

Patch:       Source/modules/encryptedmedia/MediaKeySession.cpp
Index: Source/modules/encryptedmedia/MediaKeySession.cpp
diff --git a/Source/modules/encryptedmedia/MediaKeySession.cpp
b/Source/modules/encryptedmedia/MediaKeySession.cpp
index
7c2f1ae23f82e74fa9a98c62ef4ac74724f23920..ec0459f20edc26ca005ad7f5a06c35005515581e
100644
--- a/Source/modules/encryptedmedia/MediaKeySession.cpp
+++ b/Source/modules/encryptedmedia/MediaKeySession.cpp
@@ -38,7 +38,9 @@
 #include "modules/encryptedmedia/MediaKeys.h"
 #include "modules/encryptedmedia/SimpleContentDecryptionModuleResult.h"
 #include "platform/ContentDecryptionModuleResult.h"
+#include "platform/ContentType.h"
 #include "platform/Logging.h"
+#include "platform/MIMETypeRegistry.h"
 #include "platform/Timer.h"
 #include "public/platform/WebContentDecryptionModule.h"
 #include "public/platform/WebContentDecryptionModuleException.h"
@@ -50,52 +52,69 @@
 
 namespace blink {
 
+static bool isKeySystemSupportedWithInitDataType(const String& keySystem, const
String& initDataType)
+{
+    ASSERT(!keySystem.isEmpty());
+
+    // FIXME: initDataType != contentType. Implement this properly.
+    // http://crbug.com/385874.
+    String contentType = initDataType;
+    if (initDataType == "webm") {
+        contentType = "video/webm";
+    } else if (initDataType == "cenc") {
+        contentType = "video/mp4";
+    }
+
+    ContentType type(contentType);
+    return MIMETypeRegistry::isSupportedEncryptedMediaMIMEType(keySystem,
type.type(), type.parameter("codecs"));
+}
+
 // A class holding a pending action.
 class MediaKeySession::PendingAction : public
GarbageCollectedFinalized<MediaKeySession::PendingAction> {
 public:
     enum Type {
+        GenerateRequest,
         Update,
-        Release,
-        Message
+        Release
     };
 
     Type type() const { return m_type; }
 
     const Persistent<ContentDecryptionModuleResult> result() const
     {
-        ASSERT(m_type == Update || m_type == Release);
         return m_result;
     }
 
     const RefPtr<ArrayBuffer> data() const
     {
-        ASSERT(m_type == Update);
+        ASSERT(m_type == GenerateRequest || m_type == Update);
         return m_data;
     }
 
-    RefPtrWillBeRawPtr<Event> event()
+    const String& initDataType() const
     {
-        ASSERT(m_type == Message);
-        return m_event;
+        ASSERT(m_type == GenerateRequest);
+        return m_initDataType;
     }
 
-    static PendingAction* CreatePendingUpdate(ContentDecryptionModuleResult*
result, PassRefPtr<ArrayBuffer> data)
+    static PendingAction*
CreatePendingGenerateRequest(ContentDecryptionModuleResult* result, const
String& initDataType, PassRefPtr<ArrayBuffer> initData)
     {
         ASSERT(result);
-        ASSERT(data);
-        return new PendingAction(Update, result, data);
+        ASSERT(initData);
+        return new PendingAction(GenerateRequest, result, initDataType,
initData);
     }
 
-    static PendingAction* CreatePendingRelease(ContentDecryptionModuleResult*
result)
+    static PendingAction* CreatePendingUpdate(ContentDecryptionModuleResult*
result, PassRefPtr<ArrayBuffer> data)
     {
         ASSERT(result);
-        return new PendingAction(Release, result, PassRefPtr<ArrayBuffer>());
+        ASSERT(data);
+        return new PendingAction(Update, result, String(), data);
     }
 
-    static PendingAction* CreatePendingMessage(PassRefPtrWillBeRawPtr<Event>
event)
+    static PendingAction* CreatePendingRelease(ContentDecryptionModuleResult*
result)
     {
-        ASSERT(event);
-        return new PendingAction(Message, event);
+        ASSERT(result);
+        return new PendingAction(Release, result, String(),
PassRefPtr<ArrayBuffer>());
     }
 
     ~PendingAction()
@@ -105,235 +124,134 @@ public:
     void trace(Visitor* visitor)
     {
         visitor->trace(m_result);
-        visitor->trace(m_event);
     }
 
 private:
-    PendingAction(Type type, ContentDecryptionModuleResult* result,
PassRefPtr<ArrayBuffer> data)
+    PendingAction(Type type, ContentDecryptionModuleResult* result, const
String& initDataType, PassRefPtr<ArrayBuffer> data)
         : m_type(type)
         , m_result(result)
+        , m_initDataType(initDataType)
         , m_data(data)
     {
     }
 
-    PendingAction(Type type, PassRefPtrWillBeRawPtr<Event> event)
-        : m_type(type)
-        , m_event(event)
-    {
-    }
-
     const Type m_type;
     const Member<ContentDecryptionModuleResult> m_result;
+    const String m_initDataType;
     const RefPtr<ArrayBuffer> m_data;
-    const RefPtrWillBeMember<Event> m_event;
 };
 
-// This class allows a MediaKeySession object to be created asynchronously.
-class MediaKeySessionInitializer : public ScriptPromiseResolver {
-    WTF_MAKE_NONCOPYABLE(MediaKeySessionInitializer);
-
+// This class wraps the promise resolver used when initializing a new session
+// and is passed to Chromium to fullfill the promise. This implementation of
+// completeWithSession() will resolve the promise with void, while
+// completeWithError() will reject the promise with an exception. complete()
+// is not expected to be called, and will reject the promise.
+class NewSessionResult : public ContentDecryptionModuleResult {
 public:
-    static ScriptPromise create(ScriptState*, MediaKeys*, const String&
initDataType, PassRefPtr<ArrayBuffer> initData, const String& sessionType);
-    virtual ~MediaKeySessionInitializer();
-
-    void completeWithSession(WebContentDecryptionModuleResult::SessionStatus);
-    void completeWithDOMException(ExceptionCode, const String& errorMessage);
-
-private:
-    MediaKeySessionInitializer(ScriptState*, MediaKeys*, const String&
initDataType, PassRefPtr<ArrayBuffer> initData, const String& sessionType);
-    void timerFired(Timer<MediaKeySessionInitializer>*);
-
-    Persistent<MediaKeys> m_mediaKeys;
-    OwnPtr<WebContentDecryptionModuleSession> m_cdmSession;
-
-    // The next 3 values are simply the initialization data saved so that the
-    // asynchronous creation has the data needed.
-    String m_initDataType;
-    RefPtr<ArrayBuffer> m_initData;
-    String m_sessionType;
-
-    Timer<MediaKeySessionInitializer> m_timer;
-};
+    NewSessionResult(ScriptState* scriptState, MediaKeySession* session)
+        : m_resolver(ScriptPromiseResolver::create(scriptState))
+        , m_session(session)
+    {
+        WTF_LOG(Media, "NewSessionResult(%p)", this);
+    }
 
-// Represents the result used when a new WebContentDecryptionModuleSession
-// object has been created. Needed as MediaKeySessionInitializer can't be both
-// a ScriptPromiseResolver and ContentDecryptionModuleResult at the same time.
-class NewMediaKeySessionResult FINAL : public ContentDecryptionModuleResult {
-public:
-    NewMediaKeySessionResult(MediaKeySessionInitializer* initializer)
-        : m_initializer(initializer)
+    virtual ~NewSessionResult()
     {
+        WTF_LOG(Media, "~NewSessionResult(%p)", this);
     }
 
     // ContentDecryptionModuleResult implementation.
     virtual void complete() OVERRIDE
     {
         ASSERT_NOT_REACHED();
-        m_initializer->completeWithDOMException(InvalidStateError, "Unexpected
completion.");
+        completeWithDOMException(InvalidStateError, "Unexpected completion.");
     }
 
     virtual void
completeWithSession(WebContentDecryptionModuleResult::SessionStatus status)
OVERRIDE
     {
-        m_initializer->completeWithSession(status);
+        if (status != WebContentDecryptionModuleResult::NewSession) {
+            ASSERT_NOT_REACHED();
+            completeWithDOMException(InvalidStateError, "Unexpected
completion.");
+        }
+
+        m_session->finishGenerateRequest();
+        m_resolver->resolve();
+        m_resolver.clear();
     }
 
-    virtual void completeWithError(WebContentDecryptionModuleException code,
unsigned long systemCode, const WebString& message) OVERRIDE
+    virtual void completeWithError(WebContentDecryptionModuleException
exceptionCode, unsigned long systemCode, const WebString& errorMessage) OVERRIDE
     {
-       
m_initializer->completeWithDOMException(WebCdmExceptionToExceptionCode(code),
message);
+        completeWithDOMException(WebCdmExceptionToExceptionCode(exceptionCode),
errorMessage);
     }
 
-private:
-    MediaKeySessionInitializer* m_initializer;
-};
-
-ScriptPromise MediaKeySessionInitializer::create(ScriptState* scriptState,
MediaKeys* mediaKeys, const String& initDataType, PassRefPtr<ArrayBuffer>
initData, const String& sessionType)
-{
-    RefPtr<MediaKeySessionInitializer> initializer = adoptRef(new
MediaKeySessionInitializer(scriptState, mediaKeys, initDataType, initData,
sessionType));
-    initializer->suspendIfNeeded();
-    initializer->keepAliveWhilePending();
-    return initializer->promise();
-}
-
-MediaKeySessionInitializer::MediaKeySessionInitializer(ScriptState*
scriptState, MediaKeys* mediaKeys, const String& initDataType,
PassRefPtr<ArrayBuffer> initData, const String& sessionType)
-    : ScriptPromiseResolver(scriptState)
-    , m_mediaKeys(mediaKeys)
-    , m_initDataType(initDataType)
-    , m_initData(initData)
-    , m_sessionType(sessionType)
-    , m_timer(this, &MediaKeySessionInitializer::timerFired)
-{
-    WTF_LOG(Media, "MediaKeySessionInitializer::MediaKeySessionIniti…
(message too large)

[jrummell, 2014-09-10 19:15:14]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-09-10 19:16:17]

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

[commit-bot: I haz the power, 2014-09-10 20:21:20]

Committed patchset #5 (id:80001) as 181765