MediaKeySession: update(key) -> update(response).

MediaKeySession: update(key) -> update(response).

This change matches the latest Encrypted Media Extension WD spec.

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

[xhwang, 2014-01-03 18:50:44]

Learning blink... This is one of my ramp-up CLs :) PTAL

jrummell/ddorwin: FYI

[acolwell GONE FROM CHROMIUM, 2014-01-03 19:28:48]

lgtm

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

https://codereview.chromium.org/117323005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:115: // From
<https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi...>:
nit: You don't want tip here. I'm pretty sure that refers the most recent change
to the repository and not the most recent change to the default branch (i.e.,
HEAD on master in git terms).

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

https://codereview.chromium.org/117323005/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:70: void update(const
WTF::Uint8Array& response);
nit: You shouldn't need the WTF:: here and above. Just include "wtf/Forward.h"
and you should be able to remove the 3 other includes and the forward decl.

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

comments addressed

[xhwang, 2014-01-03 20:20:45]

abarth@: Please do an OWNERS review.

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

https://codereview.chromium.org/117323005/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/MediaKeySession.cpp:115: // From
<https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi...>:
On 2014/01/03 19:28:49, acolwell wrote:
> nit: You don't want tip here. I'm pretty sure that refers the most recent
change
> to the repository and not the most recent change to the default branch (i.e.,
> HEAD on master in git terms).

Done.

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

https://codereview.chromium.org/117323005/diff/1/Source/platform/drm/ContentD...
Source/platform/drm/ContentDecryptionModuleSession.h:70: void update(const
WTF::Uint8Array& response);
On 2014/01/03 19:28:49, acolwell wrote:
> nit: You shouldn't need the WTF:: here and above. Just include "wtf/Forward.h"
> and you should be able to remove the 3 other includes and the forward decl.

Done.

[xhwang, 2014-01-06 23:38:52]

abarth@: kindly ping!

[ddorwin, 2014-01-07 01:10:43]

LGTM. I have an unrelated question for abarth@.

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

https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:128:
m_updateTimer.startOneShot(0);
abarth: Is it preferable to use Blink timers to "Schedule a task to..." or to
use Chromium's PostTask?

Doing this in Chromium would seem to give us more control over which thread and
maybe allow us to be more precise at which step we start the task. However, this
might mean more of the algorithm steps would be in Chromium. We'll need to keep
this in mind as we get further in implementation, but I'm curious about best
practices too.

https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeySession.h (right):

https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.h:106: Timer<MediaKeySession>
m_updateTimer;
"update timer" could be ambiguous. Minor issue.

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

More thoughts on where to schedule the task.

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

https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:128:
m_updateTimer.startOneShot(0);
On 2014/01/07 01:10:43, ddorwin wrote:
> abarth: Is it preferable to use Blink timers to "Schedule a task to..." or to
> use Chromium's PostTask?
> 
> Doing this in Chromium would seem to give us more control over which thread
and
> maybe allow us to be more precise at which step we start the task. However,
this
> might mean more of the algorithm steps would be in Chromium. We'll need to
keep
> this in mind as we get further in implementation, but I'm curious about best
> practices too.

Looking at the steps in the algorithm, there may not be steps that we want more
control of. We may even want to do some of the similar createSession() task in
Blink (e.g. steps 4.3 and 4.4).

The main disadvantage of posting the task in Blink might be:
 * An unnecessary post if we end up posting in the Chromium code.
 * The task still runs on the main thread.
On the other hand, posting the task here enforces a clear barrier between the
synchronous and asynchronous parts of the algorithm regardless of what happens
in the more complex Chromium code.

[xhwang, 2014-01-07 17:33:53]

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

https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeySession.cpp:128:
m_updateTimer.startOneShot(0);
On 2014/01/07 02:41:21, ddorwin wrote:
> On 2014/01/07 01:10:43, ddorwin wrote:
> > abarth: Is it preferable to use Blink timers to "Schedule a task to..." or
to
> > use Chromium's PostTask?
> > 
> > Doing this in Chromium would seem to give us more control over which thread
> and
> > maybe allow us to be more precise at which step we start the task. However,
> this
> > might mean more of the algorithm steps would be in Chromium. We'll need to
> keep
> > this in mind as we get further in implementation, but I'm curious about best
> > practices too.
> 
> Looking at the steps in the algorithm, there may not be steps that we want
more
> control of. We may even want to do some of the similar createSession() task in
> Blink (e.g. steps 4.3 and 4.4).
> 
> The main disadvantage of posting the task in Blink might be:
>  * An unnecessary post if we end up posting in the Chromium code.
>  * The task still runs on the main thread.
> On the other hand, posting the task here enforces a clear barrier between the
> synchronous and asynchronous parts of the algorithm regardless of what happens
> in the more complex Chromium code.

I like posting the task here so that:
* Blink impl is close to the spec
* Chromium impl doesn't need to worry about sync/async.

I agree that this may end up with an extra post if Chromium also does a post.
But I don't feel it's a big performance issue because the whole
MediaKeys/MediaKeySession operations are relatively inexpensive/infrequent
compared to the media demuxing/decoding.

[xhwang, 2014-01-08 17:54:35]

On 2014/01/07 17:33:53, xhwang wrote:
>
https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
> File Source/modules/encryptedmedia/MediaKeySession.cpp (right):
> 
>
https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
> Source/modules/encryptedmedia/MediaKeySession.cpp:128:
> m_updateTimer.startOneShot(0);
> On 2014/01/07 02:41:21, ddorwin wrote:
> > On 2014/01/07 01:10:43, ddorwin wrote:
> > > abarth: Is it preferable to use Blink timers to "Schedule a task to..." or
> to
> > > use Chromium's PostTask?
> > > 
> > > Doing this in Chromium would seem to give us more control over which
thread
> > and
> > > maybe allow us to be more precise at which step we start the task.
However,
> > this
> > > might mean more of the algorithm steps would be in Chromium. We'll need to
> > keep
> > > this in mind as we get further in implementation, but I'm curious about
best
> > > practices too.
> > 
> > Looking at the steps in the algorithm, there may not be steps that we want
> more
> > control of. We may even want to do some of the similar createSession() task
in
> > Blink (e.g. steps 4.3 and 4.4).
> > 
> > The main disadvantage of posting the task in Blink might be:
> >  * An unnecessary post if we end up posting in the Chromium code.
> >  * The task still runs on the main thread.
> > On the other hand, posting the task here enforces a clear barrier between
the
> > synchronous and asynchronous parts of the algorithm regardless of what
happens
> > in the more complex Chromium code.
> 
> I like posting the task here so that:
> * Blink impl is close to the spec
> * Chromium impl doesn't need to worry about sync/async.
> 
> I agree that this may end up with an extra post if Chromium also does a post.
> But I don't feel it's a big performance issue because the whole
> MediaKeys/MediaKeySession operations are relatively inexpensive/infrequent
> compared to the media demuxing/decoding.

abarth@! kindly ping! This CL is trivial to review (only renaming parameters),
but you
may want to look at the questions/discussions :)

[abarth-chromium, 2014-01-08 19:10:13]

On 2014/01/07 01:10:43, ddorwin wrote:
>
https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
> File Source/modules/encryptedmedia/MediaKeySession.cpp (right):
> 
>
https://codereview.chromium.org/117323005/diff/70001/Source/modules/encrypted...
> Source/modules/encryptedmedia/MediaKeySession.cpp:128:
> m_updateTimer.startOneShot(0);
> abarth: Is it preferable to use Blink timers to "Schedule a task to..." or to
> use Chromium's PostTask?

Please use Timers when scheduling work on the current thread.  They're carefully
implemented to follow the spec for scheduling work on the current thread.  We
use PostTask to cross thread boundaries.

[abarth-chromium, 2014-01-08 19:11:44]

lgtm

[commit-bot: I haz the power, 2014-01-08 19:12:03]

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

[commit-bot: I haz the power, 2014-01-08 20:26:22]

Change committed as 164707