Update MediaKeySession method names to latest EME spec.

Update MediaKeySession method names to latest EME spec.

Renaming and comment cleanup only. The real spec compiant changes will be made in another CL.

The chromium side change is at: https://codereview.chromium.org/101693009/

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

[xhwang, 2014-01-04 01:03:46]

PTAL

[xhwang, 2014-01-04 01:07:53]

This CL is based contains changes in https://codereview.chromium.org/117323005/.
I'll rebase this CL after https://codereview.chromium.org/117323005/ is landed.

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.h:102: Deque<PendingKeyRequest>
m_pendingKeyRequests;
Dropping this because we could have only one pending initialize.

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:119: session->initialize(type,
initData);
The name "initialize" comes from the spec: "Schedule a task to initialize the
session".

[acolwell GONE FROM CHROMIUM, 2014-01-06 20:18:37]

lgtm % nits

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:113:
m_session->initialize(m_pendingMimeType, *m_pendingInitData);
The *m_pendingInitData doesn't look safe since there is no NULL check in
initialize(). Should there be an ASSERT(m_pendingInitData) here and perhaps an
ASSERT(initData) in initialize()?

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:81: // FIXME: This reference is
outdated.
nit: any reason not to update the reference in this CL?

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:94: // If type is null or an empty
string and initData is not null or an empty string, throw an
nit: Why is this no longer a step in the spec? This behavior should be defined
there.

[ddorwin, 2014-01-07 02:41:55]

Some of our comments overlap but are in different Patch Sets even though the
contents didn't change. This view might be useful:
https://codereview.chromium.org/111043004/diff2/1:100001/Source/modules/encry...

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:51: , m_initializeTimer(this,
&MediaKeySession::initializeTimerFired)
abarth: Is it a requirement that we call these xxxTimerFired? executeXxxTask
would be more meaningful.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:90: // FIXME: Eliminate
timers. Asynchronicity will be handled in Chromium.
I added this comment. I wonder if it is correct (see comments in other CL).

In MediaKeys.cpp, I suggest that this method actually be called
schedule...Task... That's fine for these local classes, but it would be a weird
name for a platform interface (e.g. the call at 113). That would be an argument
in favor of doing the asynchronicity in Blink. (See also my other comments about
this at
https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...)

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:97: if (!m_session)
Is this possible?

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:103: // 1. Let request be
null.
These comments aren't very helpful here since all the work is done in Chromium.
We should probably just say that the Chromium side is responsible for executing
the [remainder of] task steps.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:105: // 3. Wait for the
MediaKeys constructor task to complete.
Should we implement steps 3 and 4 in Blink or Chromium? These seem like
something we could handle in Blink. WDYT? The real question is where we know
that the constructor task has completed and where errors are stored.

(Regardless, I think I'm going to swap 1-2 and 3-4 in the spec.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:81: // FIXME: This reference is
outdated.
Yeah, we should not call release() here. WebCDM should be responsible for any
deinitialization.
There are also lifetime issues we'll need to make sure we get right.

For now, I think we should just remove this code. Maybe add a FIXME to make sure
things are torn down correctly.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:94: // If type is null or an empty
string and initData is not null or an empty string, throw an
Can type be null?
The parameters are no longer optional in the spec IDL. Please update the IDL and
test it. Then you can probably remove this step.

That leaves the empty string, which is not explicitly enforced by the spec. We
should probably just let supportsMIMEType() handle that (but make sure we have
tests!).

initData is not optional either (can't be NULL I hope), but it could
theoretically be empty. (It was never a "string". :) )

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:118: // 4. Schedule a task to
initialize the session, providing type, initData, and the new object.
Hmm, so I even used "initialize" in the spec... (I was struggling with wording,
and that was before loadSession() became a possibility.) At least in the spec,
this is used within the createSession() algorithm, so there is little ambiguity.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:119: session->initialize(type,
initData);
If we are going to keep the asynchronicity in Blink, shouldn't the timer be
here? (Though that's not very appealing since we want to keep the deferred
parameters in the session class.)
OR, the MKS method could actually be called scheduleTask... or schedule...Task
and actually handle (or delegate) the scheduling as it currently does.

Basically, I feel like the comment and the method name don't match at all.

(Another argument for scheduling the task here is that the task is supposed to
be provided the new object. That's a minor issue, though.)

[xhwang, 2014-01-09 01:04:41]

I added FIXME for some non-renaming related comments, which will be addressed in
a later CL. Now this CL mostly only contains renaming and comments cleanup
changes.

PTAL again.

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:113:
m_session->initialize(m_pendingMimeType, *m_pendingInitData);
On 2014/01/06 20:18:37, acolwell wrote:
> The *m_pendingInitData doesn't look safe since there is no NULL check in
> initialize(). Should there be an ASSERT(m_pendingInitData) here and perhaps an
> ASSERT(initData) in initialize()?

Added a FIXME about checking NULL initData in MediaKeys::createSession.

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:81: // FIXME: This reference is
outdated.
On 2014/01/06 20:18:37, acolwell wrote:
> nit: any reason not to update the reference in this CL?

This is removed now.

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:94: // If type is null or an empty
string and initData is not null or an empty string, throw an
On 2014/01/06 20:18:37, acolwell wrote:
> nit: Why is this no longer a step in the spec? This behavior should be defined
> there.

Added FIXME.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:90: // FIXME: Eliminate
timers. Asynchronicity will be handled in Chromium.
On 2014/01/07 02:41:55, ddorwin wrote:
> I added this comment. I wonder if it is correct (see comments in other CL).
> 
> In MediaKeys.cpp, I suggest that this method actually be called
> schedule...Task... That's fine for these local classes, but it would be a
weird
> name for a platform interface (e.g. the call at 113). That would be an
argument
> in favor of doing the asynchronicity in Blink. (See also my other comments
about
> this at
>
https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...)

Based on abarth@'s response, we should do this acync'ly in blink.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:97: if (!m_session)
On 2014/01/07 02:41:55, ddorwin wrote:
> Is this possible?

This is possible when release() is called.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:103: // 1. Let request be
null.
On 2014/01/07 02:41:55, ddorwin wrote:
> These comments aren't very helpful here since all the work is done in
Chromium.
> We should probably just say that the Chromium side is responsible for
executing
> the [remainder of] task steps.

Added FIXME

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:105: // 3. Wait for the
MediaKeys constructor task to complete.
On 2014/01/07 02:41:55, ddorwin wrote:
> Should we implement steps 3 and 4 in Blink or Chromium? These seem like
> something we could handle in Blink. WDYT? The real question is where we know
> that the constructor task has completed and where errors are stored.
> 
> (Regardless, I think I'm going to swap 1-2 and 3-4 in the spec.

Added a FIXME.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:81: // FIXME: This reference is
outdated.
On 2014/01/07 02:41:55, ddorwin wrote:
> Yeah, we should not call release() here. WebCDM should be responsible for any
> deinitialization.
> There are also lifetime issues we'll need to make sure we get right.
> 
> For now, I think we should just remove this code. Maybe add a FIXME to make
sure
> things are torn down correctly.

Done.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:94: // If type is null or an empty
string and initData is not null or an empty string, throw an
On 2014/01/07 02:41:55, ddorwin wrote:
> Can type be null?
> The parameters are no longer optional in the spec IDL. Please update the IDL
and
> test it. Then you can probably remove this step.
> 
> That leaves the empty string, which is not explicitly enforced by the spec. We
> should probably just let supportsMIMEType() handle that (but make sure we have
> tests!).
> 
> initData is not optional either (can't be NULL I hope), but it could
> theoretically be empty. (It was never a "string". :) )

This change will be non-tribial. Added FIXME.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:118: // 4. Schedule a task to
initialize the session, providing type, initData, and the new object.
On 2014/01/07 02:41:55, ddorwin wrote:
> Hmm, so I even used "initialize" in the spec... (I was struggling with
wording,
> and that was before loadSession() became a possibility.) At least in the spec,
> this is used within the createSession() algorithm, so there is little
ambiguity.

Yeah, this is how I come up with this word in the first place :) The new name is
initializeNewSession.

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:119: session->initialize(type,
initData);
On 2014/01/07 02:41:55, ddorwin wrote:
> If we are going to keep the asynchronicity in Blink, shouldn't the timer be
> here? (Though that's not very appealing since we want to keep the deferred
> parameters in the session class.)
> OR, the MKS method could actually be called scheduleTask... or schedule...Task
> and actually handle (or delegate) the scheduling as it currently does.
> 
> Basically, I feel like the comment and the method name don't match at all.
> 
> (Another argument for scheduling the task here is that the task is supposed to
> be provided the new object. That's a minor issue, though.)

Added FIXME.

[ddorwin, 2014-01-09 01:35:31]

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:59: release();
We should NOT call release(). We may, however, want to run the other code in
release(). Actually, I think that code should NOT be in release() since
release() != CLOSED.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:69: ASSERT(!m_keys ==
!m_session);
This probably doesn't make sense anymore given the other comments I made.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:73: m_session.clear();
I think this line and below should only be in the destructor.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:96: return;
It might be worth noting why this can happen (release() immediately after
createSession()) since it was not obvious (to me).

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.h:98:
InitializeNewSessionData(const String& mimeType, Uint8Array* initData) :
mimeType(mimeType), initData(initData) { }
Is '*' correct? (I don't know.)

Also, if this RefPtr'd object is shared with JS, I think it's possible that the
data could be changed, which we don't want. Maybe we need to make a copy.
(Again, I don't have the answer.)
Consider:
  createSession(type, array);
  array[0]=9;
  // Timer fires.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.h:102:
Deque<InitializeNewSessionData> m_pendingInitializeNewSessionData;
We don't actually need a queue because this can only be called once per object.
We don't need to change that in this renaming CL, though. Also, this queue code
would work if we move this to MediaKeys.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.idl (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.idl:38: void release();
[RaisesException] - It can throw in the spec.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:117: // FIXME: |initData| may be 0.
We need to add more check before we dereference it.
Empty, but not NULL. Maybe you mean the IDL needs to be updated to match the
spec? Is that covered by l90?

[xhwang, 2014-01-09 02:31:08]

comments addressed

[xhwang, 2014-01-09 02:32:17]

Comments mostly addressed and more FIXME added :) PTAL again

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:59: release();
On 2014/01/09 01:35:31, ddorwin wrote:
> We should NOT call release(). We may, however, want to run the other code in
> release(). Actually, I think that code should NOT be in release() since
> release() != CLOSED.

Done.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:69: ASSERT(!m_keys ==
!m_session);
On 2014/01/09 01:35:31, ddorwin wrote:
> This probably doesn't make sense anymore given the other comments I made.

Done.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:73: m_session.clear();
On 2014/01/09 01:35:31, ddorwin wrote:
> I think this line and below should only be in the destructor.

Done.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:96: return;
On 2014/01/09 01:35:31, ddorwin wrote:
> It might be worth noting why this can happen (release() immediately after
> createSession()) since it was not obvious (to me).

Now we remove m_session.clear() to the dtor, so we don't need this check
anymore.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.h:98:
InitializeNewSessionData(const String& mimeType, Uint8Array* initData) :
mimeType(mimeType), initData(initData) { }
On 2014/01/09 01:35:31, ddorwin wrote:
> Is '*' correct? (I don't know.)
> 
> Also, if this RefPtr'd object is shared with JS, I think it's possible that
the
> data could be changed, which we don't want. Maybe we need to make a copy.
> (Again, I don't have the answer.)
> Consider:
>   createSession(type, array);
>   array[0]=9;
>   // Timer fires.

Added FIXME.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.h:102:
Deque<InitializeNewSessionData> m_pendingInitializeNewSessionData;
On 2014/01/09 01:35:31, ddorwin wrote:
> We don't actually need a queue because this can only be called once per
object.
> We don't need to change that in this renaming CL, though. Also, this queue
code
> would work if we move this to MediaKeys.

Done.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.idl (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.idl:38: void release();
On 2014/01/09 01:35:31, ddorwin wrote:
> [RaisesException] - It can throw in the spec.

Done.

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:117: // FIXME: |initData| may be 0.
We need to add more check before we dereference it.
On 2014/01/09 01:35:31, ddorwin wrote:
> Empty, but not NULL. Maybe you mean the IDL needs to be updated to match the
> spec? Is that covered by l90?

In this function, |initData| is a pointer which can be 0. Yes, the IDL change is
covered by l.90.

[ddorwin, 2014-01-09 18:05:32]

LGTM with 2 comments (one in PS4).

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:117: // FIXME: |initData| may be 0.
We need to add more check before we dereference it.
On 2014/01/09 02:32:17, xhwang wrote:
> On 2014/01/09 01:35:31, ddorwin wrote:
> > Empty, but not NULL. Maybe you mean the IDL needs to be updated to match the
> > spec? Is that covered by l90?
> 
> In this function, |initData| is a pointer which can be 0. Yes, the IDL change
is
> covered by l.90.

Shouldn't we note that this FIXME may become obsolete with that change? We don't
want someone to come along and fix this after that change.

https://codereview.chromium.org/111043004/diff/340001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/340001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:75: ASSERT(m_session);
Probably not necessary. We don't have it elsewhere.

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

comments addressed

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

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/200001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:117: // FIXME: |initData| may be 0.
We need to add more check before we dereference it.
On 2014/01/09 18:05:33, ddorwin wrote:
> On 2014/01/09 02:32:17, xhwang wrote:
> > On 2014/01/09 01:35:31, ddorwin wrote:
> > > Empty, but not NULL. Maybe you mean the IDL needs to be updated to match
the
> > > spec? Is that covered by l90?
> > 
> > In this function, |initData| is a pointer which can be 0. Yes, the IDL
change
> is
> > covered by l.90.
> 
> Shouldn't we note that this FIXME may become obsolete with that change? We
don't
> want someone to come along and fix this after that change.

Done.

https://codereview.chromium.org/111043004/diff/340001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/340001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:75: ASSERT(m_session);
On 2014/01/09 18:05:33, ddorwin wrote:
> Probably not necessary. We don't have it elsewhere.

Done.

[xhwang, 2014-01-09 20:08:14]

rebase only

[xhwang, 2014-01-09 20:44:04]

abarth@: Please owners review this CL, thanks!

[xhwang, 2014-01-09 20:49:14]

On 2014/01/09 20:44:04, xhwang wrote:
> abarth@: Please owners review this CL, thanks!

To clarify, please OWNERS review */platform/* files, which is pretty much just
renaming.

[abarth-chromium, 2014-01-09 22:27:10]

Source/platform and public/platform LGTM

[xhwang, 2014-01-10 18:16:00]

rebase only

[commit-bot: I haz the power, 2014-01-10 18:25:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/111043004/580001

[commit-bot: I haz the power, 2014-01-10 19:52:23]

Change committed as 164896

[ddorwin, 2014-01-25 01:49:35]

I added a few updates on things discussed in this CL that have now been resolved
in the spec. Some of the FIXMEs have been resolved, but we should go through and
make sure we're following the spec and have the appropriate steps listed.

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

https://codereview.chromium.org/111043004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeys.cpp:94: // If type is null or an empty
string and initData is not null or an empty string, throw an
On 2014/01/09 01:04:41, xhwang wrote:
> On 2014/01/06 20:18:37, acolwell wrote:
> > nit: Why is this no longer a step in the spec? This behavior should be
defined
> > there.
> 
> Added FIXME.

This is now steps in the spec:
https://dvcs.w3.org/hg/html-media/rev/2e9c25eede16

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeySession.cpp (right):

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeySession.cpp:105: // 3. Wait for the
MediaKeys constructor task to complete.
On 2014/01/09 01:04:41, xhwang wrote:
> On 2014/01/07 02:41:55, ddorwin wrote:
> > Should we implement steps 3 and 4 in Blink or Chromium? These seem like
> > something we could handle in Blink. WDYT? The real question is where we know
> > that the constructor task has completed and where errors are stored.
> > 
> > (Regardless, I think I'm going to swap 1-2 and 3-4 in the spec.
> 
> Added a FIXME.

FYI, I made this change: https://dvcs.w3.org/hg/html-media/rev/b24748c3d28a

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
File Source/modules/encryptedmedia/MediaKeys.cpp (right):

https://codereview.chromium.org/111043004/diff/100001/Source/modules/encrypte...
Source/modules/encryptedmedia/MediaKeys.cpp:94: // If type is null or an empty
string and initData is not null or an empty string, throw an
On 2014/01/09 01:04:41, xhwang wrote:
> On 2014/01/07 02:41:55, ddorwin wrote:
> > Can type be null?
> > The parameters are no longer optional in the spec IDL. Please update the IDL
> and
> > test it. Then you can probably remove this step.
> > 
> > That leaves the empty string, which is not explicitly enforced by the spec.
We
> > should probably just let supportsMIMEType() handle that (but make sure we
have
> > tests!).
> > 
> > initData is not optional either (can't be NULL I hope), but it could
> > theoretically be empty. (It was never a "string". :) )
> 
> This change will be non-tribial. Added FIXME.

This is now enforced by steps in the spec:
https://dvcs.w3.org/hg/html-media/rev/2e9c25eede16