Allow setContentDecryptionModule() to get called when setting is done.

Allow setContentDecryptionModule() to get called when setting is done.

Decoders can register to get notified when a Decryptor is set.
setContentDecryptionModule() provides a new Decryptor to
WebMediaPlayer. Since the decoders run on the media thread, it may
take some time for the old Decryptor to get detached and a new one
connected. Adding a callback to be used so that
setContentDecryptionModule() knows when the Decoders are done with
the notification. The additional callback is optional. This will be
used to resolve the setMediaKeys() promise on the blink side.

BUG=358271
TEST=media unittests and EME layout tests pass

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289197

[jrummell, 2014-07-26 00:43:49]

PTAL. Blink changes in https://codereview.chromium.org/423633002/.

[xhwang, 2014-07-29 20:58:07]

The CL looks good. Just some questions for discussion.

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:787:
.Run(proxy_decryptor_->GetDecryptor(), base::Closure());
Here and below, use base::Bind(&base::DoNothing) instead of base::Closure() so
that we can assume |done_cb| is never null. Then we can drop the done_cb
checking in Decrypting* classes.

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:916: "The existing MediaKeys
object cannot be removed.");
We may not have a CDM set yet. So more accurately this should be something like:
"Setting a null MediaKeys object not supported."

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:928: // No player, so setting CDM
is complete.
This is more of a spec question, so +ddorwin for comments.

This comment here let me feel a bit of inconsistency. If there's no player,
setting a CDM finishes immediately. If there is a player, setting a CDM will be
delayed until the playback resumes/starts. So setMeidaKeys + load is quite
different from load + setMediaKeys.

This will be more complicated on Chrome for Android because we send an IPC to
the browser to really "set" the CDM.

How about we remove "Attempt to Resume Playback If Necessary" as a condition to
resolve the promise? We still attempt to resume playback if necessary, but
that's not a necessary condition to resolve the promise.

If we do so, then we can simply return the promise after l.920. I might be
missing some important cases from JS's perspective though....

https://codereview.chromium.org/416333011/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.h (right):

https://codereview.chromium.org/416333011/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:71: void SetDecryptor(Decryptor*
decryptor, base::Closure done_cb);
Update comment about |done_cb|.

https://codereview.chromium.org/416333011/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:71: void SetDecryptor(Decryptor*
decryptor, base::Closure done_cb);
pass callbacks by const-ref: const base::Closure& done_cb

[jrummell, 2014-07-30 00:15:13]

Updated.

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:787:
.Run(proxy_decryptor_->GetDecryptor(), base::Closure());
On 2014/07/29 20:58:07, xhwang wrote:
> Here and below, use base::Bind(&base::DoNothing) instead of base::Closure() so
> that we can assume |done_cb| is never null. Then we can drop the done_cb
> checking in Decrypting* classes.

Done.

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:916: "The existing MediaKeys
object cannot be removed.");
On 2014/07/29 20:58:07, xhwang wrote:
> We may not have a CDM set yet. So more accurately this should be something
like:
> "Setting a null MediaKeys object not supported."

Done.

https://codereview.chromium.org/416333011/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.h (right):

https://codereview.chromium.org/416333011/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:71: void SetDecryptor(Decryptor*
decryptor, base::Closure done_cb);
On 2014/07/29 20:58:07, xhwang wrote:
> pass callbacks by const-ref: const base::Closure& done_cb

Done.

https://codereview.chromium.org/416333011/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:71: void SetDecryptor(Decryptor*
decryptor, base::Closure done_cb);
On 2014/07/29 20:58:07, xhwang wrote:
> Update comment about |done_cb|.

Done.

[ddorwin, 2014-07-30 22:35:46]

A few questions many times. (Don't worry about replying to each duplicate
question.)

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:892: // TODO(jrummell): Remove
this method once blink starts calling with 2
Better in the header?

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:916: "Setting a null MediaKeys
object not supported.");
nit: s/not/is not/.
Or, to be more concise, just drop "Setting a ".

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:928: // No player, so setting CDM
is complete.
I don't think this comment is correct. WMPI is the player.

web_cdm_ should not be NULL given line 912. That means the decryptor_ready_cb_
is null. How can that happen?

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:1355: .Run(NULL,
base::Bind(&base::DoNothing));
Don't we need to satisfy the promise in some way?

How can this happen?

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:1371:
base::Bind(&base::DoNothing));
prefixed only - so this is fine.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:1376:
decryptor_ready_cb.Run(web_cdm_->GetDecryptor(),
Maybe comment that there can be no outstanding setMediaKeys() calls if web_cdm_.

That may not actually be true. The key is that there are no outstanding calls or
ContentDecryptionModuleSet is pending and does not need to be called again.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:245: void
ContentDecryptionModuleSet(
I wonder if there is a better name that "Set"? It's really inserted/attached to
the pipeline/decoders.

https://codereview.chromium.org/416333011/diff/20001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/416333011/diff/20001/media/base/decryptor.h#n...
media/base/decryptor.h:165: // complete.
When what is complete?
When the returned Decryptor is completely inserted in the pipeline?

https://codereview.chromium.org/416333011/diff/20001/media/filters/audio_deco...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:87:
RunCallback<0>(decryptor_.get(), base::Bind(&base::DoNothing)));
Why DoNothing?

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.cc:182: done_cb.Run();
Shouldn't this reject the promise?
How can we get here?

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:70: // complete.
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:71: void SetDecryptor(Decryptor*
decryptor, const base::Closure& done_cb);
nit: That might also lead to a better name for |done_cb|.

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_audio_decoder_unittest.cc:111:
RunCallbackIfNotNull(decryptor_.get(), base::Bind(&base::DoNothing)));
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_demuxer_stream.cc:176: done_cb.Run();
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_demuxer_stream.h (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_demuxer_stream.h:71: // complete.
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_demuxer_stream_unittest.cc:123:
base::Bind(&base::DoNothing),
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_video_decoder.cc:161: done_cb.Run();
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_video_decoder.h (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_video_decoder.h:59: // complete.
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:74:
RunCallbackIfNotNull(decryptor_.get(), base::Bind(&base::DoNothing)));
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/video_deco...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:83:
RunCallback<0>(decryptor_.get(), base::Bind(&base::DoNothing)));
ditto

https://codereview.chromium.org/416333011/diff/20001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:223:
RunCallback<0>(decryptor_.get(), base::Bind(&base::DoNothing)));
ditto

[ddorwin, 2014-07-30 22:41:34]

Replied to xhwang's question.

Also, comments on the CL description:

"when setting is done."
When setMediaKeys() is called?

"The additional callback is optional."
Optional for who? It's not optional for callers - only Blink (prefixed EME),
right?

I assume the Blink side is going to queue up and serialize setMediaKeys() calls
to wait for the previous call's callback.

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:928: // No player, so setting CDM
is complete.
On 2014/07/29 20:58:07, xhwang wrote:
> This is more of a spec question, so +ddorwin for comments.
> 
> This comment here let me feel a bit of inconsistency. If there's no player,
> setting a CDM finishes immediately. If there is a player, setting a CDM will
be
> delayed until the playback resumes/starts. So setMeidaKeys + load is quite
> different from load + setMediaKeys.
> 
> This will be more complicated on Chrome for Android because we send an IPC to
> the browser to really "set" the CDM.
> 
> How about we remove "Attempt to Resume Playback If Necessary" as a condition
to
> resolve the promise? We still attempt to resume playback if necessary, but
> that's not a necessary condition to resolve the promise.
> 
> If we do so, then we can simply return the promise after l.920. I might be
> missing some important cases from JS's perspective though....

As I noted in the next PS, I'm not sure this comment is correct. Or at least
it's not obvious that the logic means "no player".

We don't need to successfully (re)start playback to resolve the promise, but the
pipeline and CDM must be successfully connected. In the no player case (handled
in Blink?), we can resolve immediately since there isn't a pipeline. load() (and
the subsequent pipeline initialization would use the CDM (make sure this is what
happens!) and fail if there are problems.

[ddorwin, 2014-07-30 22:59:24]

This DecryptorReadyCB code has always been difficult to understand. I wonder if
we can find a better solution given the changes for unprefixed EME (i.e.
asynchronous setMediaKeys() is separate from CDM instantiation).

[xhwang, 2014-07-31 00:10:24]

On 2014/07/30 22:41:34, ddorwin wrote:
> Replied to xhwang's question.
> 
> Also, comments on the CL description:
> 
> "when setting is done."
> When setMediaKeys() is called?
> 
> "The additional callback is optional."
> Optional for who? It's not optional for callers - only Blink (prefixed EME),
> right?
> 
> I assume the Blink side is going to queue up and serialize setMediaKeys()
calls
> to wait for the previous call's callback.
> 
>
https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
> File content/renderer/media/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
> content/renderer/media/webmediaplayer_impl.cc:928: // No player, so setting
CDM
> is complete.
> On 2014/07/29 20:58:07, xhwang wrote:
> > This is more of a spec question, so +ddorwin for comments.
> > 
> > This comment here let me feel a bit of inconsistency. If there's no player,
> > setting a CDM finishes immediately. If there is a player, setting a CDM will
> be
> > delayed until the playback resumes/starts. So setMeidaKeys + load is quite
> > different from load + setMediaKeys.
> > 
> > This will be more complicated on Chrome for Android because we send an IPC
to
> > the browser to really "set" the CDM.
> > 
> > How about we remove "Attempt to Resume Playback If Necessary" as a condition
> to
> > resolve the promise? We still attempt to resume playback if necessary, but
> > that's not a necessary condition to resolve the promise.
> > 
> > If we do so, then we can simply return the promise after l.920. I might be
> > missing some important cases from JS's perspective though....
> 
> As I noted in the next PS, I'm not sure this comment is correct. Or at least
> it's not obvious that the logic means "no player".
> 
> We don't need to successfully (re)start playback to resolve the promise, but
the
> pipeline and CDM must be successfully connected. In the no player case
(handled
> in Blink?), we can resolve immediately since there isn't a pipeline. load()
(and
> the subsequent pipeline initialization would use the CDM (make sure this is
what
> happens!) and fail if there are problems.

These all make sense except that it's not symmetric:
If the CDM cannot be set on the player, then:
1, load -> setMediaKeys results a rejected promise.
2, setMediaKeys -> load results a successful promise with some other kinds
   of error later.

I guess it's an implementation quality issue on how to interpret "the pipeline
and CDM must be successfully connected". There are so many layers in the media
stack, we have WebMediaPlayer, Pipeline, Renderer, Decoder. We also have the
browser side decoder and decryptor on platforms like Android.

My question is whether we really want to wait for the deepest connection happens
to resolve the promise? On Android this would require another IPC and a whole
lot
of plumbing. I wonder will it just work if we resolve the promise as long as
the CDM is set in WMP*. This is not perfect, but would be much simpler in terms
of implementation.

[xhwang, 2014-07-31 00:15:53]

On 2014/07/30 22:59:24, ddorwin wrote:
> This DecryptorReadyCB code has always been difficult to understand. I wonder
if
> we can find a better solution given the changes for unprefixed EME (i.e.
> asynchronous setMediaKeys() is separate from CDM instantiation).

The new media::Renderer interface will have a SetCdm() method. So in the future
WMPI::SetCdm would call Pipeline::SetCdm which will then call Renderer::SetCdm.

We may still need to keep DecryptorReadyCB in RendererImpl (the desktop Renderer
implementation), but that'll be much closer to where it's used.

For browser based CDM and IPC based Renderer, SetCdm() will be translated to an
IPC message directly.

[ddorwin, 2014-07-31 00:21:00]

On 2014/07/31 00:10:24, xhwang wrote:
> On 2014/07/30 22:41:34, ddorwin wrote:
> > Replied to xhwang's question.
> > 
> > Also, comments on the CL description:
> > 
> > "when setting is done."
> > When setMediaKeys() is called?
> > 
> > "The additional callback is optional."
> > Optional for who? It's not optional for callers - only Blink (prefixed EME),
> > right?
> > 
> > I assume the Blink side is going to queue up and serialize setMediaKeys()
> calls
> > to wait for the previous call's callback.
> > 
> >
>
https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
> > File content/renderer/media/webmediaplayer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/416333011/diff/1/content/renderer/media/webme...
> > content/renderer/media/webmediaplayer_impl.cc:928: // No player, so setting
> CDM
> > is complete.
> > On 2014/07/29 20:58:07, xhwang wrote:
> > > This is more of a spec question, so +ddorwin for comments.
> > > 
> > > This comment here let me feel a bit of inconsistency. If there's no
player,
> > > setting a CDM finishes immediately. If there is a player, setting a CDM
will
> > be
> > > delayed until the playback resumes/starts. So setMeidaKeys + load is quite
> > > different from load + setMediaKeys.
> > > 
> > > This will be more complicated on Chrome for Android because we send an IPC
> to
> > > the browser to really "set" the CDM.
> > > 
> > > How about we remove "Attempt to Resume Playback If Necessary" as a
condition
> > to
> > > resolve the promise? We still attempt to resume playback if necessary, but
> > > that's not a necessary condition to resolve the promise.
> > > 
> > > If we do so, then we can simply return the promise after l.920. I might be
> > > missing some important cases from JS's perspective though....
> > 
> > As I noted in the next PS, I'm not sure this comment is correct. Or at least
> > it's not obvious that the logic means "no player".
> > 
> > We don't need to successfully (re)start playback to resolve the promise, but
> the
> > pipeline and CDM must be successfully connected. In the no player case
> (handled
> > in Blink?), we can resolve immediately since there isn't a pipeline. load()
> (and
> > the subsequent pipeline initialization would use the CDM (make sure this is
> what
> > happens!) and fail if there are problems.
> 
> These all make sense except that it's not symmetric:
> If the CDM cannot be set on the player, then:
> 1, load -> setMediaKeys results a rejected promise.
> 2, setMediaKeys -> load results a successful promise with some other kinds
>    of error later.
> 
> I guess it's an implementation quality issue on how to interpret "the pipeline
> and CDM must be successfully connected". There are so many layers in the media
> stack, we have WebMediaPlayer, Pipeline, Renderer, Decoder. We also have the
> browser side decoder and decryptor on platforms like Android.

"Pipeline" and other things I said are not mentioned in the spec. The spec just
says, "Associate the CDM instance represented by mediaKeys with the media
element for decrypting media data." I would interpret "associate" as meaning it
is hooked up and ready to use. In our implementation, this includes updating the
Decryptor.
> 
> My question is whether we really want to wait for the deepest connection
happens
> to resolve the promise? On Android this would require another IPC and a whole
> lot
> of plumbing. I wonder will it just work if we resolve the promise as long as
> the CDM is set in WMP*. This is not perfect, but would be much simpler in
terms
> of implementation.

On Android, I would expect that this includes connecting the MediaDrm and
MediaCodec objects. Now that setMediaKeys() is asynchronous, I don't see a
problem waiting for the IPC.


I didn't think resuming playback is required to resolve the promise, but I see
that this is the case in the current spec:
"Run the Attempt to Resume Playback If Necessary algorithm on the media element"
is run synchronously.
I'll have to check, but I don't think this is intentional, especially since we
have events to indicate resumed playback. It's also incompatible with the
setMediaKeys() -> load case.

I agree that there is an asymmetry, but I'm not sure how to solve that. In
theory, load should never fail. I guess the behavior should be the same as
failing to initialize a codec.

[jrummell, 2014-08-01 22:09:43]

Updated callback to indicate success/failure, and updated tests to verify that
the callback is actually called.

To reduce the total comments a lot of the "ditto" comments are not responded to,
although the code has been touched.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:892: // TODO(jrummell): Remove
this method once blink starts calling with 2
On 2014/07/30 22:35:45, ddorwin wrote:
> Better in the header?

Based in comments for the blink review, we may keep this method around. Removed
TODO.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:916: "Setting a null MediaKeys
object not supported.");
On 2014/07/30 22:35:45, ddorwin wrote:
> nit: s/not/is not/.
> Or, to be more concise, just drop "Setting a ".

Done.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:928: // No player, so setting CDM
is complete.
On 2014/07/30 22:35:45, ddorwin wrote:
> I don't think this comment is correct. WMPI is the player.
> 
> web_cdm_ should not be NULL given line 912. That means the decryptor_ready_cb_
> is null. How can that happen?

Comment updated. decryptor_ready_cb_ is null if .src has not been specified yet.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:1355: .Run(NULL,
base::Bind(&base::DoNothing));
On 2014/07/30 22:35:45, ddorwin wrote:
> Don't we need to satisfy the promise in some way?
> 
> How can this happen?

Here we are cancelling the previous request. There is no promise associated with
this.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:1371:
base::Bind(&base::DoNothing));
On 2014/07/30 22:35:45, ddorwin wrote:
> prefixed only - so this is fine.

Acknowledged.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:1376:
decryptor_ready_cb.Run(web_cdm_->GetDecryptor(),
On 2014/07/30 22:35:45, ddorwin wrote:
> Maybe comment that there can be no outstanding setMediaKeys() calls if
web_cdm_.
> 
> That may not actually be true. The key is that there are no outstanding calls
or
> ContentDecryptionModuleSet is pending and does not need to be called again.

Not sure what to comment here. setContentDecryptionModule() may have called
decryptor_ready_cb_ with the decryptor and it hasn't completed, but here we are
calling a different decryptor_ready_cb (with the same decryptor). If we saved
the WebContentDecryptionModuleResult, then calling it would be required here.
But currently we're not doing that.

The other interesting thing is that this gets called twice, once per stream. So
even if we "saved" the WebContentDecryptionModuleResult, it would only get
resolved when the first stream attaches to the decryptor.

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/416333011/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:245: void
ContentDecryptionModuleSet(
On 2014/07/30 22:35:45, ddorwin wrote:
> I wonder if there is a better name that "Set"? It's really inserted/attached
to
> the pipeline/decoders.

Done.

https://codereview.chromium.org/416333011/diff/20001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/416333011/diff/20001/media/base/decryptor.h#n...
media/base/decryptor.h:165: // complete.
On 2014/07/30 22:35:46, ddorwin wrote:
> When what is complete?
> When the returned Decryptor is completely inserted in the pipeline?

Done.

https://codereview.chromium.org/416333011/diff/20001/media/filters/audio_deco...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:87:
RunCallback<0>(decryptor_.get(), base::Bind(&base::DoNothing)));
On 2014/07/30 22:35:46, ddorwin wrote:
> Why DoNothing?

Added a mock method to call and verify that they are called in pairs.

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.cc:182: done_cb.Run();
On 2014/07/30 22:35:46, ddorwin wrote:
> Shouldn't this reject the promise?
> How can we get here?

This is called with NULL if there is a previous decryptor reference pending
(https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...),
and may get called if we enable support for setting mediaKeys = null (step 3.2.3
of setMediakeys(mediaKeys) in the EME spec). Since it currently doesn't stop
this decoder from using the decryptor, it makes sense to fail the promise. Added
a bool parameter to indicate success/failure.

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/416333011/diff/20001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:71: void SetDecryptor(Decryptor*
decryptor, const base::Closure& done_cb);
On 2014/07/30 22:35:46, ddorwin wrote:
> nit: That might also lead to a better name for |done_cb|.

Acknowledged.

[ddorwin, 2014-08-04 18:59:09]

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:140: void DoNothing(bool) {
Is there a reason not to use the base:: version?

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:893: void
WebMediaPlayerImpl::setContentDecryptionModule(
This will need a new name, though. Also, we should probably DCHECK that
decryptor_ready_cb_.is_null() since the pipeine should not yet be initialized.

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:930: // No pipeline/decoder
connected, so resolve the promise. When something
On 2014/08/01 22:09:42, jrummell wrote:
> On 2014/07/30 22:35:45, ddorwin wrote:
> > I don't think this comment is correct. WMPI is the player.
> > 
> > web_cdm_ should not be NULL given line 912. That means the
decryptor_ready_cb_
> > is null. How can that happen?
> 
> Comment updated. decryptor_ready_cb_ is null if .src has not been specified
yet.

I thought WMPI did not exist until .src was specified. Is this code here to
handle the load() case we're discussing in the Blink review? If so, maybe this
will be handled by the function above.

https://codereview.chromium.org/416333011/diff/40001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/416333011/diff/40001/media/base/decryptor.h#n...
media/base/decryptor.h:164: // Callback to notify that the decryptor has been
completely inserted into the
re: "inserted": "attached" is used elsewhere. We should be consistent.

https://codereview.chromium.org/416333011/diff/40001/media/base/decryptor.h#n...
media/base/decryptor.h:165: // pipeline. Parameter indicates if the operation
succeeded of failed.
nit: more concise: ... whether the operation succeeded.

https://codereview.chromium.org/416333011/diff/40001/media/filters/audio_deco...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:61: void
SetDecryptorReadyCallbackCalled() { ++ready_called_; }
Is it okay/expected for the values to be >1 apart? i.e.:
ready_called_: 2
set_called_: 0

If so, I think lin 64 would fail. Otherwise, we can probably just
increment/decrement a single value or use a bool.

https://codereview.chromium.org/416333011/diff/40001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:109: EXPECT_CALL(*this,
SetDecryptorReadyCallback(_)).WillRepeatedly(DoAll(
This looks odd. Do we need SetDecryptorReadyCallback() if it doesn't do
anything? Can we have it call
CallbackVerifier::SetDecryptorReadyCallbackCalled() or replace it with that
function?

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:69: // Callback for
DecryptorHost::RequestDecryptor(). |insert_done_cb| is called
ditto on naming.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_audio_decoder_unittest.cc:308: EXPECT_CALL(*this,
DecryptorSet(Eq(true)));
In the unsupported audio config case, do we actually "succeed"? If not, this
should be false, right?

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:75: void
ExpectDecryptorNotification() {
Why is this extracted into a function only in this file?

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:261: EXPECT_CALL(*this,
DecryptorSet(Eq(false)));
_IF_ you're going to use the function above, this should be replaced too. That
means a bool for expected_notification_response or something like that.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:431: Destroy();
No EXPECT_CALL(DecryptorSet) ?
If that is intentional, the callback should be some "never called"
function/macro.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:436:
ExpectDecryptorNotification();
This is a successful set?

https://codereview.chromium.org/416333011/diff/40001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/pipeline_i...
media/filters/pipeline_integration_test_base.cc:310: decryptor_ready_cb.Run(
Do we need to confirm (in this function or at a higher level) that we get the
callback?

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_deco...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:53: // Used to verify that the
number of calls to SetDecryptorReadyCallback()
ditto - skipped this file.

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:96: // Used to verify that the
number of calls to SetDecryptorReadyCallback()
ditto - skipped this file.

Since we now have 3 copies of this, maybe we can add it to
media/base/test_helpers.h. We'd need to rename it (and the comments) to be more
generic. MatchingCallbackVerifier?

[jrummell, 2014-08-07 01:54:25]

Updated.

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:140: void DoNothing(bool) {
On 2014/08/04 18:59:08, ddorwin wrote:
> Is there a reason not to use the base:: version?

The base version doesn't have any parameters.

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:893: void
WebMediaPlayerImpl::setContentDecryptionModule(
On 2014/08/04 18:59:08, ddorwin wrote:
> This will need a new name, though. Also, we should probably DCHECK that
> decryptor_ready_cb_.is_null() since the pipeine should not yet be initialized.

Done.

https://codereview.chromium.org/416333011/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:930: // No pipeline/decoder
connected, so resolve the promise. When something
On 2014/08/04 18:59:08, ddorwin wrote:
> On 2014/08/01 22:09:42, jrummell wrote:
> > On 2014/07/30 22:35:45, ddorwin wrote:
> > > I don't think this comment is correct. WMPI is the player.
> > > 
> > > web_cdm_ should not be NULL given line 912. That means the
> decryptor_ready_cb_
> > > is null. How can that happen?
> > 
> > Comment updated. decryptor_ready_cb_ is null if .src has not been specified
> yet.
> 
> I thought WMPI did not exist until .src was specified. Is this code here to
> handle the load() case we're discussing in the Blink review? If so, maybe this
> will be handled by the function above.

Done.

https://codereview.chromium.org/416333011/diff/40001/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/416333011/diff/40001/media/base/decryptor.h#n...
media/base/decryptor.h:164: // Callback to notify that the decryptor has been
completely inserted into the
On 2014/08/04 18:59:08, ddorwin wrote:
> re: "inserted": "attached" is used elsewhere. We should be consistent.

Done.

https://codereview.chromium.org/416333011/diff/40001/media/base/decryptor.h#n...
media/base/decryptor.h:165: // pipeline. Parameter indicates if the operation
succeeded of failed.
On 2014/08/04 18:59:08, ddorwin wrote:
> nit: more concise: ... whether the operation succeeded.

Done.

https://codereview.chromium.org/416333011/diff/40001/media/filters/audio_deco...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:61: void
SetDecryptorReadyCallbackCalled() { ++ready_called_; }
On 2014/08/04 18:59:08, ddorwin wrote:
> Is it okay/expected for the values to be >1 apart? i.e.:
> ready_called_: 2
> set_called_: 0
> 
> If so, I think lin 64 would fail. Otherwise, we can probably just
> increment/decrement a single value or use a bool.

Done.

https://codereview.chromium.org/416333011/diff/40001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:109: EXPECT_CALL(*this,
SetDecryptorReadyCallback(_)).WillRepeatedly(DoAll(
On 2014/08/04 18:59:08, ddorwin wrote:
> This looks odd. Do we need SetDecryptorReadyCallback() if it doesn't do
> anything? Can we have it call
> CallbackVerifier::SetDecryptorReadyCallbackCalled() or replace it with that
> function?

The problem is that SetDecryptorReadyCallback() is called 0, 1, or 2 times,
depending on the test. Added an ACTION to simplify this.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_audio_decoder.h:69: // Callback for
DecryptorHost::RequestDecryptor(). |insert_done_cb| is called
On 2014/08/04 18:59:08, ddorwin wrote:
> ditto on naming.

Done.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_audio_decoder_unittest.cc:308: EXPECT_CALL(*this,
DecryptorSet(Eq(true)));
On 2014/08/04 18:59:08, ddorwin wrote:
> In the unsupported audio config case, do we actually "succeed"? If not, this
> should be false, right?

SetDecryptor() calls the cb with true as long as decryptor_ is not null, which
is the case here.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:75: void
ExpectDecryptorNotification() {
On 2014/08/04 18:59:08, ddorwin wrote:
> Why is this extracted into a function only in this file?

It was needed in 2 places. However, I've parameterized it so it can be used in
more places. Also updated the other tests to match.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:261: EXPECT_CALL(*this,
DecryptorSet(Eq(false)));
On 2014/08/04 18:59:08, ddorwin wrote:
> _IF_ you're going to use the function above, this should be replaced too. That
> means a bool for expected_notification_response or something like that.

Done.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:431: Destroy();
On 2014/08/04 18:59:08, ddorwin wrote:
> No EXPECT_CALL(DecryptorSet) ?
> If that is intentional, the callback should be some "never called"
> function/macro.

Done. I tried tracing this to figure out why the callback never happens. I think
what happens is that the SetDecryptor call is scheduled with a WeakPtr, and
since this call happens during destruction, the original object is gone by the
time decryptor_ready_cb is actually called. Thus SetDecryptor() never really
executes.

https://codereview.chromium.org/416333011/diff/40001/media/filters/decrypting...
media/filters/decrypting_video_decoder_unittest.cc:436:
ExpectDecryptorNotification();
On 2014/08/04 18:59:08, ddorwin wrote:
> This is a successful set?

Yup (at least the test passes this way).

https://codereview.chromium.org/416333011/diff/40001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/pipeline_i...
media/filters/pipeline_integration_test_base.cc:310: decryptor_ready_cb.Run(
On 2014/08/04 18:59:08, ddorwin wrote:
> Do we need to confirm (in this function or at a higher level) that we get the
> callback?

Done.

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_deco...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:53: // Used to verify that the
number of calls to SetDecryptorReadyCallback()
On 2014/08/04 18:59:08, ddorwin wrote:
> ditto - skipped this file.

Acknowledged.

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/40001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:96: // Used to verify that the
number of calls to SetDecryptorReadyCallback()
On 2014/08/04 18:59:08, ddorwin wrote:
> ditto - skipped this file.
> 
> Since we now have 3 copies of this, maybe we can add it to
> media/base/test_helpers.h. We'd need to rename it (and the comments) to be
more
> generic. MatchingCallbackVerifier?

Done.

[ddorwin, 2014-08-07 19:52:05]

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:896: void
WebMediaPlayerImpl::setContentDecryptionModuleSync(
order

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:160: virtual void
setContentDecryptionModuleSync(
reorder (to match my Blink comment)

https://codereview.chromium.org/416333011/diff/80001/media/base/test_helpers.h
File media/base/test_helpers.h (right):

https://codereview.chromium.org/416333011/diff/80001/media/base/test_helpers....
media/base/test_helpers.h:139: bool a_called_;
...without_b_
Or maybe, expecting_b_

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:29:
ACTION_P3(CallDecryptorReadyWithVerifier, decryptor, done_cb, verifier) {
This doesn't necessarily call DecryptorReady. It calls a callback. This could
probably have a generic name.

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:30: // verifier must be called
first since |done_cb| call will trigger it as well.
s/trigger/invoke/?
or call?

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:35:
ACTION_P(VerifyCallbackHappened, verifier) {
naming: This doesn't really verify that the callback happened.

https://codereview.chromium.org/416333011/diff/80001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/pipeline_i...
media/filters/pipeline_integration_test_base.cc:311: EXPECT_CALL(*this,
DecryptorAttached(Eq(true)));
Is there a reason this comes first in this case?

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_deco...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:18: using
::testing::InvokeWithoutArgs;
unused

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:29:
ACTION_P3(CallDecryptorReadyWithVerifier, decryptor, done_cb, verifier) {
ditto on naming

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:33:
ACTION_P3(CallDecryptorReadyWithVerifier, decryptor, done_cb, verifier) {
ditto

[jrummell, 2014-08-08 20:58:29]

Updated.

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:896: void
WebMediaPlayerImpl::setContentDecryptionModuleSync(
On 2014/08/07 19:52:04, ddorwin wrote:
> order

Done.

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/416333011/diff/80001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:160: virtual void
setContentDecryptionModuleSync(
On 2014/08/07 19:52:04, ddorwin wrote:
> reorder (to match my Blink comment)

Done.

https://codereview.chromium.org/416333011/diff/80001/media/base/test_helpers.h
File media/base/test_helpers.h (right):

https://codereview.chromium.org/416333011/diff/80001/media/base/test_helpers....
media/base/test_helpers.h:139: bool a_called_;
On 2014/08/07 19:52:04, ddorwin wrote:
> ...without_b_
> Or maybe, expecting_b_

Like expecting_B_. Done.

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:29:
ACTION_P3(CallDecryptorReadyWithVerifier, decryptor, done_cb, verifier) {
On 2014/08/07 19:52:04, ddorwin wrote:
> This doesn't necessarily call DecryptorReady. It calls a callback. This could
> probably have a generic name.

Done.

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:30: // verifier must be called
first since |done_cb| call will trigger it as well.
On 2014/08/07 19:52:04, ddorwin wrote:
> s/trigger/invoke/?
> or call?

Done.

https://codereview.chromium.org/416333011/diff/80001/media/filters/audio_deco...
media/filters/audio_decoder_selector_unittest.cc:35:
ACTION_P(VerifyCallbackHappened, verifier) {
On 2014/08/07 19:52:04, ddorwin wrote:
> naming: This doesn't really verify that the callback happened.

Done.

https://codereview.chromium.org/416333011/diff/80001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/pipeline_i...
media/filters/pipeline_integration_test_base.cc:311: EXPECT_CALL(*this,
DecryptorAttached(Eq(true)));
On 2014/08/07 19:52:04, ddorwin wrote:
> Is there a reason this comes first in this case?

I had assumed EXPECT... needs to be called before some code actually executes a
call to the method. But appears to not be the case, so moved it below.

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_deco...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:18: using
::testing::InvokeWithoutArgs;
On 2014/08/07 19:52:04, ddorwin wrote:
> unused

Done.

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_deco...
media/filters/video_decoder_selector_unittest.cc:29:
ACTION_P3(CallDecryptorReadyWithVerifier, decryptor, done_cb, verifier) {
On 2014/08/07 19:52:04, ddorwin wrote:
> ditto on naming

Done.

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_fram...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/80001/media/filters/video_fram...
media/filters/video_frame_stream_unittest.cc:33:
ACTION_P3(CallDecryptorReadyWithVerifier, decryptor, done_cb, verifier) {
On 2014/08/07 19:52:04, ddorwin wrote:
> ditto

Done.

[ddorwin, 2014-08-08 21:15:26]

naming nits

https://codereview.chromium.org/416333011/diff/100001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:926: // Used when loading media
and no pipeline/decoder attached yet.
this probably goes down one line

https://codereview.chromium.org/416333011/diff/100001/media/base/test_helpers.h
File media/base/test_helpers.h (right):

https://codereview.chromium.org/416333011/diff/100001/media/base/test_helpers...
media/base/test_helpers.h:135: void A();
nit: I wonder if this should be "RecordACalled" or "RecordA"

https://codereview.chromium.org/416333011/diff/100001/media/base/test_helpers...
media/base/test_helpers.h:139: bool expecting_B_;
nit: 'b'

https://codereview.chromium.org/416333011/diff/100001/media/filters/audio_dec...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/100001/media/filters/audio_dec...
media/filters/audio_decoder_selector_unittest.cc:35:
ACTION_P(CheckCallbackHappenedUsingVerifier, verifier) {
On 2014/08/08 20:58:28, jrummell wrote:
> On 2014/08/07 19:52:04, ddorwin wrote:
> > naming: This doesn't really verify that the callback happened.
> 
> Done.

naming: It's really just recording that B was called. "ReportCallback"?

The check is part of the verifier, so it may not even need to be in the name.

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_dec...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_dec...
media/filters/video_decoder_selector_unittest.cc:34:
ACTION_P(CheckCallbackHappenedUsingVerifier, verifier) {
ditto

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_fra...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_fra...
media/filters/video_frame_stream_unittest.cc:39:
ACTION_P(CheckCallbackHappenedUsingVerifier, verifier) {
ditto

[jrummell, 2014-08-08 23:40:00]

Updated.

https://codereview.chromium.org/416333011/diff/100001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/100001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:926: // Used when loading media
and no pipeline/decoder attached yet.
On 2014/08/08 21:15:26, ddorwin wrote:
> this probably goes down one line

Done.

https://codereview.chromium.org/416333011/diff/100001/media/base/test_helpers.h
File media/base/test_helpers.h (right):

https://codereview.chromium.org/416333011/diff/100001/media/base/test_helpers...
media/base/test_helpers.h:135: void A();
On 2014/08/08 21:15:26, ddorwin wrote:
> nit: I wonder if this should be "RecordACalled" or "RecordA"

Done.

https://codereview.chromium.org/416333011/diff/100001/media/base/test_helpers...
media/base/test_helpers.h:139: bool expecting_B_;
On 2014/08/08 21:15:26, ddorwin wrote:
> nit: 'b'

Done.

https://codereview.chromium.org/416333011/diff/100001/media/filters/audio_dec...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/100001/media/filters/audio_dec...
media/filters/audio_decoder_selector_unittest.cc:35:
ACTION_P(CheckCallbackHappenedUsingVerifier, verifier) {
On 2014/08/08 21:15:26, ddorwin wrote:
> On 2014/08/08 20:58:28, jrummell wrote:
> > On 2014/08/07 19:52:04, ddorwin wrote:
> > > naming: This doesn't really verify that the callback happened.
> > 
> > Done.
> 
> naming: It's really just recording that B was called. "ReportCallback"?
> 
> The check is part of the verifier, so it may not even need to be in the name.

Done.

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_dec...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_dec...
media/filters/video_decoder_selector_unittest.cc:34:
ACTION_P(CheckCallbackHappenedUsingVerifier, verifier) {
On 2014/08/08 21:15:26, ddorwin wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_fra...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/416333011/diff/100001/media/filters/video_fra...
media/filters/video_frame_stream_unittest.cc:39:
ACTION_P(CheckCallbackHappenedUsingVerifier, verifier) {
On 2014/08/08 21:15:26, ddorwin wrote:
> ditto

Acknowledged.

[ddorwin, 2014-08-08 23:47:12]

lgtm

[xhwang, 2014-08-09 00:56:04]

lgtm with nits

https://codereview.chromium.org/416333011/diff/120001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/120001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:938: result.complete();
nit: return early here so you don't need "else".

https://codereview.chromium.org/416333011/diff/120001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1336: .Run(NULL,
base::Bind(DoNothing));
nit: usually we use {} for multiple line statements.

https://codereview.chromium.org/416333011/diff/120001/media/base/test_helpers.h
File media/base/test_helpers.h (right):

https://codereview.chromium.org/416333011/diff/120001/media/base/test_helpers...
media/base/test_helpers.h:128: // Used to verify that the number of calls to A()
match the number of calls to
We are not tracking "the number of calls" since we use a boolean. Need to update
to match exactly what we are doing.

https://codereview.chromium.org/416333011/diff/120001/media/base/test_helpers...
media/base/test_helpers.h:131: class MatchingCallbackVerifier {
Naming bikeshedding.. In our use case, this is really checking all registered
callbacks are canceled. Can we do

s/MatchingCallbackVerifier/CallbackCancellationChecker
s/RecordACalled/RegisterCallback
s/RecordBCalled/CancelCallback

If we want to make this generic in the sense that it tracks a pair of calls,
then it should be something like CallbackPairChecker...

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.h:72: const DecryptorAttachedCB&
attach_done_cb);
nit: s/attach_done_cb/decryptor_attached_cb to be accurate?

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
media/filters/decrypting_video_decoder.cc:162: attach_done_cb.Run(false);
For the record, SetDecryptor is posted from main thread to media thread so here
we are not reentering WMPI. No action required here.

https://codereview.chromium.org/416333011/diff/120001/media/filters/pipeline_...
File media/filters/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/416333011/diff/120001/media/filters/pipeline_...
media/filters/pipeline_integration_test_base.cc:315: EXPECT_CALL(*this,
DecryptorAttached(Eq(true)));
Here and below... Does this work (drop the Eq() part)?

EXPECT_CALL(*this, DecryptorAttached(true));

[jrummell, 2014-08-11 21:47:15]

Thanks for the reviews.

https://codereview.chromium.org/416333011/diff/120001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/416333011/diff/120001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:938: result.complete();
On 2014/08/09 00:56:03, xhwang wrote:
> nit: return early here so you don't need "else".

Done.

https://codereview.chromium.org/416333011/diff/120001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:1336: .Run(NULL,
base::Bind(DoNothing));
On 2014/08/09 00:56:03, xhwang wrote:
> nit: usually we use {} for multiple line statements.

If only clang-format did that when it reformats :)

https://codereview.chromium.org/416333011/diff/120001/media/base/test_helpers.h
File media/base/test_helpers.h (right):

https://codereview.chromium.org/416333011/diff/120001/media/base/test_helpers...
media/base/test_helpers.h:128: // Used to verify that the number of calls to A()
match the number of calls to
On 2014/08/09 00:56:03, xhwang wrote:
> We are not tracking "the number of calls" since we use a boolean. Need to
update
> to match exactly what we are doing.

Done.

https://codereview.chromium.org/416333011/diff/120001/media/base/test_helpers...
media/base/test_helpers.h:131: class MatchingCallbackVerifier {
On 2014/08/09 00:56:03, xhwang wrote:
> Naming bikeshedding.. In our use case, this is really checking all registered
> callbacks are canceled. Can we do
> 
> s/MatchingCallbackVerifier/CallbackCancellationChecker
> s/RecordACalled/RegisterCallback
> s/RecordBCalled/CancelCallback
> 
> If we want to make this generic in the sense that it tracks a pair of calls,
> then it should be something like CallbackPairChecker...

Like CallbackPairChecker.

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.h (right):

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.h:72: const DecryptorAttachedCB&
attach_done_cb);
On 2014/08/09 00:56:03, xhwang wrote:
> nit: s/attach_done_cb/decryptor_attached_cb to be accurate?

Done.

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/416333011/diff/120001/media/filters/decryptin...
media/filters/decrypting_video_decoder.cc:162: attach_done_cb.Run(false);
On 2014/08/09 00:56:03, xhwang wrote:
> For the record, SetDecryptor is posted from main thread to media thread so
here
> we are not reentering WMPI. No action required here.

Acknowledged.

https://codereview.chromium.org/416333011/diff/120001/media/filters/pipeline_...
File media/filters/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/416333011/diff/120001/media/filters/pipeline_...
media/filters/pipeline_integration_test_base.cc:315: EXPECT_CALL(*this,
DecryptorAttached(Eq(true)));
On 2014/08/09 00:56:03, xhwang wrote:
> Here and below... Does this work (drop the Eq() part)?
> 
> EXPECT_CALL(*this, DecryptorAttached(true));

Done.

[jrummell, 2014-08-11 21:47:24]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-11 21:51:26]

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

[commit-bot: I haz the power, 2014-08-11 23:38:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[commit-bot: I haz the power, 2014-08-11 23:51:09]

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

[commit-bot: I haz the power, 2014-08-11 23:51:11]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[xhwang, 2014-08-12 16:12:59]

wmpa changes lgtm

https://codereview.chromium.org/416333011/diff/160001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/416333011/diff/160001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1515: "Null MediaKeys
object is not supported.");
return here?

https://codereview.chromium.org/416333011/diff/160001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1520: return;
Now with the |result|, we need to make sure the |result| is not leaked before we
"return". We should be able to DCHECK(web_cdm_) so that we don't have a "return"
here.

[jrummell, 2014-08-12 22:54:18]

Thanks for catching the missing return.

https://codereview.chromium.org/416333011/diff/160001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/416333011/diff/160001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1515: "Null MediaKeys
object is not supported.");
On 2014/08/12 16:12:59, xhwang wrote:
> return here?

Done.

https://codereview.chromium.org/416333011/diff/160001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1520: return;
On 2014/08/12 16:12:59, xhwang wrote:
> Now with the |result|, we need to make sure the |result| is not leaked before
we
> "return". We should be able to DCHECK(web_cdm_) so that we don't have a
"return"
> here.

Done.

[jrummell, 2014-08-12 22:54:26]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-12 23:52:01]

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

[commit-bot: I haz the power, 2014-08-13 01:46:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-13 04:01:28]

Change committed as 289197