Add unit tests for CdmAdapter

Add unit tests for CdmAdapter

BUG=510088
TEST=new unittests pass

Committed: https://crrev.com/30996193fe3e69fe9ccd52471900bf5685c54956
Cr-Commit-Position: refs/heads/master@{#359582}

[jrummell, 2015-11-05 22:42:21]

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

[jrummell, 2015-11-05 22:42:21]

PTAL

[xhwang, 2015-11-05 23:25:51]

I'll wait for the other CL to review this. Does it make sense to try reusing
AesDecryptorTests for this directly?

[jrummell, 2015-11-07 00:44:51]

Updated to add new tests, and rebase on the Create() changes.

As for using AesDecryptor tests, 16 of the 28 tests do decryption which isn't
available yet. Most of the others do things like multiple sessions,
valid/invalid JWK, etc. Valid tests, but aren't really exercising more of the
current CdmAdapter code. In the long run we should do both, but until CdmAdapter
does decryption I think it should wait. We will probably still need separate
tests to handle Create() and maybe the FileIO interface.

[xhwang, 2015-11-10 22:58:58]

looking pretty good. I just have a few comments.

https://chromiumcodereview.appspot.com/1428753010/diff/20001/media/cdm/cdm_ad...
File media/cdm/cdm_adapter_unittest.cc (right):

https://chromiumcodereview.appspot.com/1428753010/diff/20001/media/cdm/cdm_ad...
media/cdm/cdm_adapter_unittest.cc:94: ~CdmAdapterTest() override {
DeinitializeCdm(); }
Call Init and deinit in Setup() and Teardown()?

https://chromiumcodereview.appspot.com/1428753010/diff/20001/media/cdm/cdm_ad...
media/cdm/cdm_adapter_unittest.cc:207: void OnCdmCreated(PromiseResult
expected_result,
CdmAdapter::Create() takes a CdmCreatedCB instead of a promise. Can we simply
use a boolean for the expected_result here? Or if you prefer an enum for
clarity, probably just use SUCCESS and FAILURE. Resolved and rejected are really
implementation details that the test shouldn't care about.

https://chromiumcodereview.appspot.com/1428753010/diff/20001/media/cdm/cdm_ad...
media/cdm/cdm_adapter_unittest.cc:250: expected_result)));
It's odd that you pass the same |expected_result| to both OnResolve() and
OnReject(). Can we make OnResolve() and OnReject() mock methods and

if (expected to success)
  EXPECT_CALL(*this, &OnResolve);
else
  EXPECT_CALL(*this, &OnReject);

https://chromiumcodereview.appspot.com/1428753010/diff/20001/media/cdm/cdm_ad...
media/cdm/cdm_adapter_unittest.cc:309: RunUntilIdle();
Can you call RunUntilIdle() within InitializeAndExpect(),
CreateSessionAndExpect() and UpdateSessionAndExpect().

[jrummell, 2015-11-12 02:30:32]

Updated.

https://codereview.chromium.org/1428753010/diff/20001/media/cdm/cdm_adapter_u...
File media/cdm/cdm_adapter_unittest.cc (right):

https://codereview.chromium.org/1428753010/diff/20001/media/cdm/cdm_adapter_u...
media/cdm/cdm_adapter_unittest.cc:94: ~CdmAdapterTest() override {
DeinitializeCdm(); }
On 2015/11/10 22:58:58, xhwang wrote:
> Call Init and deinit in Setup() and Teardown()?

Done.

https://codereview.chromium.org/1428753010/diff/20001/media/cdm/cdm_adapter_u...
media/cdm/cdm_adapter_unittest.cc:207: void OnCdmCreated(PromiseResult
expected_result,
On 2015/11/10 22:58:58, xhwang wrote:
> CdmAdapter::Create() takes a CdmCreatedCB instead of a promise. Can we simply
> use a boolean for the expected_result here? Or if you prefer an enum for
> clarity, probably just use SUCCESS and FAILURE. Resolved and rejected are
really
> implementation details that the test shouldn't care about.

Like SUCCESS/FAILURE better, since the methods are ...AndExpect().

https://codereview.chromium.org/1428753010/diff/20001/media/cdm/cdm_adapter_u...
media/cdm/cdm_adapter_unittest.cc:250: expected_result)));
On 2015/11/10 22:58:58, xhwang wrote:
> It's odd that you pass the same |expected_result| to both OnResolve() and
> OnReject(). Can we make OnResolve() and OnReject() mock methods and
> 
> if (expected to success)
>   EXPECT_CALL(*this, &OnResolve);
> else
>   EXPECT_CALL(*this, &OnReject);
> 

Done.

https://codereview.chromium.org/1428753010/diff/20001/media/cdm/cdm_adapter_u...
media/cdm/cdm_adapter_unittest.cc:309: RunUntilIdle();
On 2015/11/10 22:58:58, xhwang wrote:
> Can you call RunUntilIdle() within InitializeAndExpect(),
> CreateSessionAndExpect() and UpdateSessionAndExpect().

Done.

[xhwang, 2015-11-12 06:20:36]

LGTM with one last naming nit. Thanks!

https://chromiumcodereview.appspot.com/1428753010/diff/40001/media/cdm/cdm_ad...
File media/cdm/cdm_adapter_unittest.cc (right):

https://chromiumcodereview.appspot.com/1428753010/diff/40001/media/cdm/cdm_ad...
media/cdm/cdm_adapter_unittest.cc:91: enum PromiseResult { SUCCESS, FAILURE };
nit: s/PromiseResult/ExpectedResult? In the OnCdmCreated() case, there's no
"promise" involved.

[jrummell, 2015-11-12 17:56:53]

Thanks for the reviews.

https://codereview.chromium.org/1428753010/diff/40001/media/cdm/cdm_adapter_u...
File media/cdm/cdm_adapter_unittest.cc (right):

https://codereview.chromium.org/1428753010/diff/40001/media/cdm/cdm_adapter_u...
media/cdm/cdm_adapter_unittest.cc:91: enum PromiseResult { SUCCESS, FAILURE };
On 2015/11/12 06:20:36, xhwang wrote:
> nit: s/PromiseResult/ExpectedResult? In the OnCdmCreated() case, there's no
> "promise" involved.

Done.

[jrummell, 2015-11-12 17:56:57]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-11-12 17:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1428753010/#ps60001
(title: "rename enum")

[commit-bot: I haz the power, 2015-11-12 17:57:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428753010/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428753010/60001

[commit-bot: I haz the power, 2015-11-12 18:05:06]

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

[commit-bot: I haz the power, 2015-11-12 18:05:07]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jrummell, 2015-11-12 23:23:35]

xhwang@, please take another look. Changes for avoiding memory leak with
GURL::EmptyGURL(), as well as not building on Android.

[xhwang, 2015-11-13 00:03:22]

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.cc:315: *empty_gurl_.get());
Can we just create a local one here?

GURL empty_gurl;
session_message_cb_.Run(session_id, LICENSE_REQUEST, message, empty_gurl);

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:260: return new
media::ClearKeyCdm(host, key_system_string, *empty_gurl);
Can we just create a temp local GURL here instead of using a global one?

[jrummell, 2015-11-13 00:35:21]

Updated.

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.cc:315: *empty_gurl_.get());
On 2015/11/13 00:03:22, xhwang wrote:
> Can we just create a local one here?
> 
> GURL empty_gurl;
> session_message_cb_.Run(session_id, LICENSE_REQUEST, message, empty_gurl);

Done.

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/1428753010/diff/80001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:260: return new
media::ClearKeyCdm(host, key_system_string, *empty_gurl);
On 2015/11/13 00:03:22, xhwang wrote:
> Can we just create a temp local GURL here instead of using a global one?

Done.

[xhwang, 2015-11-13 01:04:28]

LGTM++

[jrummell, 2015-11-13 18:00:35]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-11-13 18:02:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428753010/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428753010/100001

[commit-bot: I haz the power, 2015-11-13 18:06:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-13 18:07:41]

Patchset 6 (id:??) landed as
https://crrev.com/30996193fe3e69fe9ccd52471900bf5685c54956
Cr-Commit-Position: refs/heads/master@{#359582}

[zhaoqin, 2015-11-16 03:37:49]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1450803002/ by zhaoqin@google.com.

The reason for reverting is: valgrind reports leaks with no-debug-symbol
callstack

BUG=556346.

[zhaoqin1, 2015-11-16 03:45:05]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1447943002/ by zhaoqin@chromium.org.

The reason for reverting is: valgrind reports leaks with no-debug-symbol
callstack

BUG=556346
TBR=jrummell@chromium.org.