Add decrypt-only external clear key browser tests.

Add decrypt-only external clear key browser tests.

These tests cover the code path where a CDM is used in decrypt-only mode.

BUG=321420
TEST=none

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

[xhwang, 2013-11-20 23:54:04]

After more thinking, I felt adding a new key_system is cleaner than manipulating
the key.

Note that FrameSizeChangeVideo test isn't passing for decrypt-only. I'll
investigate that more. Meanwhile, please take a look at this CL to see if you
like this approach.

[xhwang, 2013-11-21 01:19:39]

This failing test will be fixed in: https://codereview.chromium.org/78003005/

[ddorwin, 2013-11-21 04:10:58]

minor stuff

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:37: const char
kDecryptOnlyExternalClearKeyKeySystem[] =
Start with a common prefix?
kExternalClearKeyKeySystemDecryptOnly

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:77: key_system ==
kDecryptOnlyExternalClearKeyKeySystem;
Just check if the first length of kExternalClearKeyKeySystem bytes is
kExternalClearKeyKeySystem?

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:154: key_system);
fits?

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:159: kWidevineKeySystem);
change here too?

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:306:
INSTANTIATE_TEST_CASE_P(SRC_DecryptOnlyExternalClearKey, EncryptedMediaTest,
This is going to add a lot of test time (in addition to coverage). :(
We should at least file a bug to cut down the test duration, most likely by
shortening the video run time.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:391:
"org.chromium.externalclearkey.initializefail",
Move this up with all other known "modes".

https://codereview.chromium.org/79903002/diff/1/chrome/renderer/media/chrome_...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/79903002/diff/1/chrome/renderer/media/chrome_...
chrome/renderer/media/chrome_key_systems.cc:99: info.key_system =
kDecryptOnlyExternalClearKeyKeySystem;
This and 95 should be consistent.

https://codereview.chromium.org/79903002/diff/1/chrome/test/data/media/encryp...
File chrome/test/data/media/encrypted_media_utils.js (right):

https://codereview.chromium.org/79903002/diff/1/chrome/test/data/media/encryp...
chrome/test/data/media/encrypted_media_utils.js:18: var
DECRYPT_ONLY_EXTERNAL_CLEAR_KEY_KEY_SYSTEM =
we don't need this (see below)

https://codereview.chromium.org/79903002/diff/1/chrome/test/data/media/encryp...
chrome/test/data/media/encrypted_media_utils.js:111: e.keySystem !=
DECRYPT_ONLY_EXTERNAL_CLEAR_KEY_KEY_SYSTEM) {
Just check the beginning.

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm.cc
File media/cdm/ppapi/clear_key_cdm.cc (right):

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm...
media/cdm/ppapi/clear_key_cdm.cc:139: DVLOG(1) << "Unsupported key system.";
...system: " << key_system_string;

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm.h
File media/cdm/ppapi/clear_key_cdm.h (right):

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm...
media/cdm/ppapi/clear_key_cdm.h:153: bool decrypt_only_;
is_
const?

[xhwang, 2013-11-22 01:44:28]

comments addressed

[xhwang, 2013-11-22 01:47:36]

PTAL again.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:37: const char
kDecryptOnlyExternalClearKeyKeySystem[] =
On 2013/11/21 04:10:58, ddorwin wrote:
> Start with a common prefix?
> kExternalClearKeyKeySystemDecryptOnly

Done.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:77: key_system ==
kDecryptOnlyExternalClearKeyKeySystem;
On 2013/11/21 04:10:58, ddorwin wrote:
> Just check if the first length of kExternalClearKeyKeySystem bytes is
> kExternalClearKeyKeySystem?

Done.

This is not super accurate. The correct way should be:
key_system == kExternalClearKeyKeySystem or "org.chromium.externalclearkey_._"
is a prefix of |key_system|. Basically org.chromium.externalclearkeyfoo is not
external clear key.

But I feel a simple prefix check should be sufficient for our test purpose.
WDYT?

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:154: key_system);
On 2013/11/21 04:10:58, ddorwin wrote:
> fits?

Done.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:159: kWidevineKeySystem);
On 2013/11/21 04:10:58, ddorwin wrote:
> change here too?

Done.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:306:
INSTANTIATE_TEST_CASE_P(SRC_DecryptOnlyExternalClearKey, EncryptedMediaTest,
On 2013/11/21 04:10:58, ddorwin wrote:
> This is going to add a lot of test time (in addition to coverage). :(
> We should at least file a bug to cut down the test duration, most likely by
> shortening the video run time.

The decrypt only path is orthogonal to SRC/MSE. Dropped SRC to reduce test time.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:391:
"org.chromium.externalclearkey.initializefail",
On 2013/11/21 04:10:58, ddorwin wrote:
> Move this up with all other known "modes".

Done.

https://codereview.chromium.org/79903002/diff/1/chrome/renderer/media/chrome_...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/79903002/diff/1/chrome/renderer/media/chrome_...
chrome/renderer/media/chrome_key_systems.cc:99: info.key_system =
kDecryptOnlyExternalClearKeyKeySystem;
On 2013/11/21 04:10:58, ddorwin wrote:
> This and 95 should be consistent.

Done.

https://codereview.chromium.org/79903002/diff/1/chrome/test/data/media/encryp...
File chrome/test/data/media/encrypted_media_utils.js (right):

https://codereview.chromium.org/79903002/diff/1/chrome/test/data/media/encryp...
chrome/test/data/media/encrypted_media_utils.js:18: var
DECRYPT_ONLY_EXTERNAL_CLEAR_KEY_KEY_SYSTEM =
On 2013/11/21 04:10:58, ddorwin wrote:
> we don't need this (see below)

Done.

https://codereview.chromium.org/79903002/diff/1/chrome/test/data/media/encryp...
chrome/test/data/media/encrypted_media_utils.js:111: e.keySystem !=
DECRYPT_ONLY_EXTERNAL_CLEAR_KEY_KEY_SYSTEM) {
On 2013/11/21 04:10:58, ddorwin wrote:
> Just check the beginning.

Done.

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm.cc
File media/cdm/ppapi/clear_key_cdm.cc (right):

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm...
media/cdm/ppapi/clear_key_cdm.cc:139: DVLOG(1) << "Unsupported key system.";
On 2013/11/21 04:10:58, ddorwin wrote:
> ...system: " << key_system_string;

Done.

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm.h
File media/cdm/ppapi/clear_key_cdm.h (right):

https://codereview.chromium.org/79903002/diff/1/media/cdm/ppapi/clear_key_cdm...
media/cdm/ppapi/clear_key_cdm.h:153: bool decrypt_only_;
On 2013/11/21 04:10:58, ddorwin wrote:
> is_
> const?

Done.

[ddorwin, 2013-11-22 04:28:58]

lgtm

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:77: key_system ==
kDecryptOnlyExternalClearKeyKeySystem;
On 2013/11/22 01:47:36, xhwang wrote:
> On 2013/11/21 04:10:58, ddorwin wrote:
> > Just check if the first length of kExternalClearKeyKeySystem bytes is
> > kExternalClearKeyKeySystem?
> 
> Done.
> 
> This is not super accurate. The correct way should be:
> key_system == kExternalClearKeyKeySystem or "org.chromium.externalclearkey_._"
> is a prefix of |key_system|. Basically org.chromium.externalclearkeyfoo is not
> external clear key.
> 
> But I feel a simple prefix check should be sufficient for our test purpose.
> WDYT?

Yeah, I know. Agreed.
You could add:
&& (key_system.size() = eck_prefix.size() || key_system[eck_prefix.size()] ==
'.')

[xhwang, 2013-11-22 07:44:26]

ddorwin: I pulled key_system_names.h so that I don't need to redefine
IsExternalClearKey(). PTAL again.

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/1/chrome/browser/media/encrypte...
chrome/browser/media/encrypted_media_browsertest.cc:77: key_system ==
kDecryptOnlyExternalClearKeyKeySystem;
On 2013/11/22 04:28:59, ddorwin wrote:
> On 2013/11/22 01:47:36, xhwang wrote:
> > On 2013/11/21 04:10:58, ddorwin wrote:
> > > Just check if the first length of kExternalClearKeyKeySystem bytes is
> > > kExternalClearKeyKeySystem?
> > 
> > Done.
> > 
> > This is not super accurate. The correct way should be:
> > key_system == kExternalClearKeyKeySystem or
"org.chromium.externalclearkey_._"
> > is a prefix of |key_system|. Basically org.chromium.externalclearkeyfoo is
not
> > external clear key.
> > 
> > But I feel a simple prefix check should be sufficient for our test purpose.
> > WDYT?
> 
> Yeah, I know. Agreed.
> You could add:
> && (key_system.size() = eck_prefix.size() || key_system[eck_prefix.size()] ==
> '.')

Done.

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_browsertest.cc:14: #include
"media/cdm/key_system_names.h"
We could use key system names defined in this file, but I'd like to do that in
another CL.

[ddorwin, 2013-11-22 18:04:00]

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_browsertest.cc:14: #include
"media/cdm/key_system_names.h"
On 2013/11/22 07:44:26, xhwang wrote:
> We could use key system names defined in this file, but I'd like to do that in
> another CL.

I hope to remove that file - at least for non-test purposes or from content.
Maybe it should be in chrome/.

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_browsertest.cc:150: if
(media::IsExternalClearKey(key_system)) {
Since I want to get rid of that file, I'm not sure we should make this change.

https://codereview.chromium.org/79903002/diff/310001/media/cdm/key_system_nam...
File media/cdm/key_system_names.cc (right):

https://codereview.chromium.org/79903002/diff/310001/media/cdm/key_system_nam...
media/cdm/key_system_names.cc:15: static bool IsParentKeySystem(const
std::string& parent_key_system,
The function name doesn't accurately describe what's happening, but I can't
think of a better name. Adding "Of" to the end might be better and imply the
ordering.

[xhwang, 2013-11-22 18:16:49]

comments addressed

[xhwang, 2013-11-22 18:18:22]

PTAL again.

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_browsertest.cc:14: #include
"media/cdm/key_system_names.h"
On 2013/11/22 18:04:01, ddorwin wrote:
> On 2013/11/22 07:44:26, xhwang wrote:
> > We could use key system names defined in this file, but I'd like to do that
in
> > another CL.
> 
> I hope to remove that file - at least for non-test purposes or from content.
> Maybe it should be in chrome/.

SG, removed.

https://codereview.chromium.org/79903002/diff/310001/chrome/browser/media/enc...
chrome/browser/media/encrypted_media_browsertest.cc:150: if
(media::IsExternalClearKey(key_system)) {
On 2013/11/22 18:04:01, ddorwin wrote:
> Since I want to get rid of that file, I'm not sure we should make this change.

Done.

https://codereview.chromium.org/79903002/diff/310001/media/cdm/key_system_nam...
File media/cdm/key_system_names.cc (right):

https://codereview.chromium.org/79903002/diff/310001/media/cdm/key_system_nam...
media/cdm/key_system_names.cc:15: static bool IsParentKeySystem(const
std::string& parent_key_system,
On 2013/11/22 18:04:01, ddorwin wrote:
> The function name doesn't accurately describe what's happening, but I can't
> think of a better name. Adding "Of" to the end might be better and imply the
> ordering.

Done.

[ddorwin, 2013-11-22 18:27:15]

lgtm

Thanks.

[commit-bot: I haz the power, 2013-11-22 18:28:20]

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

[commit-bot: I haz the power, 2013-11-22 20:31:37]

Change committed as 236818