Encrypted Media: No need to cancel NewKeyCB during tear down.

Encrypted Media: No need to cancel NewKeyCB during tear down.

This is a follow up CL of r251277

Now we invalidate the weak pointers during Decrypting*::Stop(), so all callbacks
are canceled. We don't need to manually cancel the NewKeyCB anymore.

TEST=Media unittests updated.

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

[xhwang, 2014-02-14 22:25:52]

Another follow-up CL. PTAL.

[ddorwin, 2014-02-14 23:08:29]

I don't think this is really related to the BUG=.

LG, but I have some questions.

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:93:
.WillRepeatedly(RunCallback<1>(true));
Should this really be called repeatedly? Or are there just certain reinitialize
tests that do this? Should we limit it?

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:102: EXPECT_CALL(*decryptor_,
RegisterNewKeyCB(Decryptor::kVideo, _))
Are we missing this test in the audio case?

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:103:
.WillOnce(SaveArg<1>(&key_added_cb_));
This shows you have to be careful with WillRepeatedly when you use helper
functions. :)

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:254:
TEST_F(DecryptingVideoDecoderTest, Reinitialize_Failure) {
This test doesn't actually call the Reinitialize function in its name?

[xhwang, 2014-02-14 23:30:36]

comments addressed

[xhwang, 2014-02-14 23:31:03]

Thanks! PTAL again.

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:93:
.WillRepeatedly(RunCallback<1>(true));
On 2014/02/14 23:08:32, ddorwin wrote:
> Should this really be called repeatedly? Or are there just certain
reinitialize
> tests that do this? Should we limit it?

This WillRepeatedly also covers the reinitialization case, which is confusing.
Changed to WillOnce and added one in Reinitialize().

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:102: EXPECT_CALL(*decryptor_,
RegisterNewKeyCB(Decryptor::kVideo, _))
On 2014/02/14 23:08:32, ddorwin wrote:
> Are we missing this test in the audio case?

I believe AudioDecoders don't support reinitialization yet. That's part of the
DecoderSelector/DecoderStream effort.

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:103:
.WillOnce(SaveArg<1>(&key_added_cb_));
On 2014/02/14 23:08:32, ddorwin wrote:
> This shows you have to be careful with WillRepeatedly when you use helper
> functions. :)

Done.

https://codereview.chromium.org/167333004/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder_unittest.cc:254:
TEST_F(DecryptingVideoDecoderTest, Reinitialize_Failure) {
On 2014/02/14 23:08:32, ddorwin wrote:
> This test doesn't actually call the Reinitialize function in its name?

Added a comment to explain why.

[ddorwin, 2014-02-14 23:33:39]

lgtm

Thanks!

[xhwang, 2014-02-18 21:25:28]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-02-18 21:26:39]

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

[commit-bot: I haz the power, 2014-02-18 23:21:08]

Change committed as 251876