media: Add Feature to control External Clear Key key system support

media: Add Feature to control External Clear Key key system support

External Clear Key is only used in tests. It can be enabled in tests,
but will be disabled by default in production.

BUG=581893
Committed: https://crrev.com/6bca3b7929ba2f8a7dfb03388d671a8eb0296217
Cr-Commit-Position: refs/heads/master@{#418915}

[xhwang, 2016-09-14 18:59:23]

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

[xhwang, 2016-09-14 18:59:24]

PTAL

[xhwang, 2016-09-14 18:59:33]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:00:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ddorwin, 2016-09-14 19:33:29]

LGTM % comments

https://chromiumcodereview.appspot.com/2341883002/diff/1/media/base/media_swi...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/2341883002/diff/1/media/base/media_swi...
media/base/media_switches.cc:176: // supported platforms. Currently ECK is
supported on desktop platforms when the
This second sentence is subject to rot, including in your next CL. :)

https://chromiumcodereview.appspot.com/2341883002/diff/1/media/base/media_swi...
media/base/media_switches.cc:177: // CDM is registered on command line. On
platforms that does not support ECK,
nit: s/does/do/

[commit-bot: I haz the power, 2016-09-14 20:05:36]

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

[commit-bot: I haz the power, 2016-09-14 20:05:37]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xhwang, 2016-09-14 21:28:42]

comments addressed

[xhwang, 2016-09-14 21:29:25]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 21:29:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2016-09-14 21:32:05]

Comments addressed.

Also, I forgot to update EncryptedMediaSupportedTypesExternalClearKeyTest, which
is done in the new PS.

I also added a new EncryptedMediaSupportedTypesExternalClearKeyDisabledTest so
that we can make sure ECK IS disabled by default.

Let me know if you want to take another look, otherwise I'll commit it later
today if all bots are happy.

https://chromiumcodereview.appspot.com/2341883002/diff/1/media/base/media_swi...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/2341883002/diff/1/media/base/media_swi...
media/base/media_switches.cc:176: // supported platforms. Currently ECK is
supported on desktop platforms when the
On 2016/09/14 19:33:29, ddorwin wrote:
> This second sentence is subject to rot, including in your next CL. :)

k, removed :)

https://chromiumcodereview.appspot.com/2341883002/diff/1/media/base/media_swi...
media/base/media_switches.cc:177: // CDM is registered on command line. On
platforms that does not support ECK,
On 2016/09/14 19:33:29, ddorwin wrote:
> nit: s/does/do/

Done.

[ddorwin, 2016-09-14 21:38:57]

LGTM with possible additional coverage.

https://chromiumcodereview.appspot.com/2341883002/diff/20001/chrome/browser/m...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/2341883002/diff/20001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:323:
RegisterPepperCdm(command_line, kClearKeyCdmBaseDirectory,
Except for this line, you could run this test on all platforms. Always, ECK is
not supported by default. That might affect some of the comments above.

[xhwang, 2016-09-14 23:20:22]

comments addressed

[xhwang, 2016-09-14 23:57:15]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[xhwang, 2016-09-14 23:57:57]

https://chromiumcodereview.appspot.com/2341883002/diff/20001/chrome/browser/m...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/2341883002/diff/20001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:323:
RegisterPepperCdm(command_line, kClearKeyCdmBaseDirectory,
On 2016/09/14 21:38:57, ddorwin wrote:
> Except for this line, you could run this test on all platforms. Always, ECK is
> not supported by default. That might affect some of the comments above.

Done.

[commit-bot: I haz the power, 2016-09-14 23:58:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 01:12:59]

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

[commit-bot: I haz the power, 2016-09-15 01:13:00]

Dry run: This issue passed the CQ dry run.

[ddorwin, 2016-09-15 02:43:21]

Mostly naming issues. LGTM

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:295: // By
default External Clear Key (ECK) key system is not supported, even when
nit (and only because a change is needed below): IMO this description still
focuses too much on Pepper. Perhaps something like the following, though it's
not great either.

By default, the External Clear Key (ECK) key system is not supported even if
present. This test case tests this behavior by not enabling
kExternalClearKeyForTesting.
Even registering the Pepper CDM where applicable does not enable the CDM.

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:299: class
EncryptedMediaSupportedTypesExternalClearKeyDisabledTest
s/Disabled/NotEnabled/ is wordier but more accurate.

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:601:
PepperCDMsRegisteredButAdapterNotPresent) {
This test name is incorrect. "...ButNotEnabled"? But really this is a generic
test, so just "ExternalClearKeyNotRegistered"?

[xhwang, 2016-09-15 16:55:54]

comments addressed

[xhwang, 2016-09-15 16:56:16]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 16:56:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2016-09-15 16:56:44]

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:295: // By
default External Clear Key (ECK) key system is not supported, even when
On 2016/09/15 02:43:20, ddorwin wrote:
> nit (and only because a change is needed below): IMO this description still
> focuses too much on Pepper. Perhaps something like the following, though it's
> not great either.
> 
> By default, the External Clear Key (ECK) key system is not supported even if
> present. This test case tests this behavior by not enabling
> kExternalClearKeyForTesting.
> Even registering the Pepper CDM where applicable does not enable the CDM.

Done.

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:299: class
EncryptedMediaSupportedTypesExternalClearKeyDisabledTest
On 2016/09/15 02:43:20, ddorwin wrote:
> s/Disabled/NotEnabled/ is wordier but more accurate.

Done.

https://chromiumcodereview.appspot.com/2341883002/diff/40001/chrome/browser/m...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:601:
PepperCDMsRegisteredButAdapterNotPresent) {
On 2016/09/15 02:43:20, ddorwin wrote:
> This test name is incorrect. "...ButNotEnabled"? But really this is a generic
> test, so just "ExternalClearKeyNotRegistered"?

Good catch on the wrong naming.

Since this is a basic test when ECK is not enabled, how about just "Basic"? We
do have some other "Basic" tests as well. When it's disabled, it should not be
"registered". So "NotRegistered" seems a bit redundant.

BTW, "registered" has two meanings in this file, one is "registered with
KeySystems", the other is "pepper registration"...

[ddorwin, 2016-09-15 17:56:57]

lgtm

[xhwang, 2016-09-15 17:58:39]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2016-09-15 17:58:39]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-09-15 17:59:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 18:45:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-15 18:48:46]

Description was changed from

==========
media: Add Feature to control External Clear Key key system support

External Clear Key is only used in tests. It can be enabled in tests,
but will be disabled by default in production.

BUG=581893
==========

to

==========
media: Add Feature to control External Clear Key key system support

External Clear Key is only used in tests. It can be enabled in tests,
but will be disabled by default in production.

BUG=581893

Committed: https://crrev.com/6bca3b7929ba2f8a7dfb03388d671a8eb0296217
Cr-Commit-Position: refs/heads/master@{#418915}
==========

[commit-bot: I haz the power, 2016-09-15 18:48:47]

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

[xhwang, 2016-09-15 20:19:16]

rebase only

[xhwang, 2016-09-15 20:39:08]

Patchset #5 (id:80001) has been deleted