Create an interface for KeySystems, migrate WebEncryptedMediaClientImpl.

Create an interface for KeySystems, migrate WebEncryptedMediaClientImpl.

This does not move all of the previously static methods into the interface yet, just those used by WebEncryptedMediaClientImpl. The rest can be migrated as necessary.

This CL also includes a fixes for 457384 and 374751 (containers and codecs must match media type), because it was convenient along with the new interface for checking codec support.

BUG=468912, 457384, 374751
Committed: https://crrev.com/574df66641c3663eb27428fa48e940e4358a27f8
Cr-Commit-Position: refs/heads/master@{#322248}

[sandersd (OOO until July 31), 2015-03-19 22:53:06]

sandersd@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[ddorwin, 2015-03-20 00:55:04]

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:187: bool IsConcreteSupportedKeySystem(const
std::string& key_system);
const on most of these?

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:636: if (!mime_type.empty()) {
invert

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:704: return IsSupportedKeySystemWithMediaMimeType(
TODO to check/use media_type?

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:888:
//------------------------------------------------------------------------------
nit: This should either be at 893 or duplicated there.

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:955: const std::string& key_system) {
Eventually, we should make the parameters have a consistent order. I assume this
is derived from when these were called by canPlayType().

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

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.h:19: static KeySystems& GetInstance();
const ref?

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.h:85: MEDIA_EXPORT bool IsSupportedKeySystem(const
std::string& key_system);
re: The description: It would be good to at least move all the unprefixed
methods into the interface. That provides additional differentiation between
which to use.

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.cc:194: capabilities,
rename this requested_ and l197 supported_capabilities?

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
File media/blink/webencryptedmediaclient_impl.h (right):

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.h:47: void
SelectSupportedConfiguration(blink::WebEncryptedMediaRequest request,
Most of the logic in this class and in the .cc file is related to processing the
configuration. We should probably move it to its own class (and file?). That
would also avoid having to include details related to key_systems.h in
render_frame_impl.cc and html_document.cc.

The only reason we inject is for testing - otherwise we could have a member
internally. But if we're not going to test from the public functions, we may not
need to do this.

Pulling out all that logic into a separate .cc file is probably better
organization too.

[sandersd (OOO until July 31), 2015-03-20 22:53:20]

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:187: bool IsConcreteSupportedKeySystem(const
std::string& key_system);
On 2015/03/20 00:55:03, ddorwin wrote:
> const on most of these?

Done.

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:636: if (!mime_type.empty()) {
On 2015/03/20 00:55:03, ddorwin wrote:
> invert

Done.

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:704: return IsSupportedKeySystemWithMediaMimeType(
On 2015/03/20 00:55:03, ddorwin wrote:
> TODO to check/use media_type?

Done.

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.cc:955: const std::string& key_system) {
On 2015/03/20 00:55:03, ddorwin wrote:
> Eventually, we should make the parameters have a consistent order. I assume
this
> is derived from when these were called by canPlayType().

Acknowledged.

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

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.h:19: static KeySystems& GetInstance();
On 2015/03/20 00:55:03, ddorwin wrote:
> const ref?

No, we may need to provide access to methods that are not const. (In particular:
key system registration, UMAs, and test-related methods.) We can design around
it for unprefixed but it may not be worth the effort.

https://codereview.chromium.org/1023863002/diff/20001/media/base/key_systems....
media/base/key_systems.h:85: MEDIA_EXPORT bool IsSupportedKeySystem(const
std::string& key_system);
On 2015/03/20 00:55:03, ddorwin wrote:
> re: The description: It would be good to at least move all the unprefixed
> methods into the interface. That provides additional differentiation between
> which to use.

Agreed, I scoped this CL to avoid having to change too many files, but that's
the direction I am imagining.

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.cc:194: capabilities,
On 2015/03/20 00:55:03, ddorwin wrote:
> rename this requested_ and l197 supported_capabilities?

I'm not sure, this exactly matches the names from the spec. I agree that a
renaming would be more clear, but this naming makes reading the code and the
spec at the same time much easier.

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
File media/blink/webencryptedmediaclient_impl.h (right):

https://codereview.chromium.org/1023863002/diff/20001/media/blink/webencrypte...
media/blink/webencryptedmediaclient_impl.h:47: void
SelectSupportedConfiguration(blink::WebEncryptedMediaRequest request,
On 2015/03/20 00:55:03, ddorwin wrote:
> Most of the logic in this class and in the .cc file is related to processing
the
> configuration. We should probably move it to its own class (and file?). That
> would also avoid having to include details related to key_systems.h in
> render_frame_impl.cc and html_document.cc.
> 
> The only reason we inject is for testing - otherwise we could have a member
> internally. But if we're not going to test from the public functions, we may
not
> need to do this.
> 
> Pulling out all that logic into a separate .cc file is probably better
> organization too.

Acknowledged.

[ddorwin, 2015-03-23 17:22:14]

LGTM % comments

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems....
media/base/key_systems.cc:713: // TODO(sandersd): Make sure that the codecs also
match the |media_type|.
Addressed now?

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems.h
File media/base/key_systems.h (right):

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems....
media/base/key_systems.h:18: // only intended to support the unprefixed API.
... support unprefixed EME.

[sandersd (OOO until July 31), 2015-03-23 22:25:03]

Patchset #6 (id:100001) has been deleted

[sandersd (OOO until July 31), 2015-03-23 22:25:09]

Patchset #5 (id:80001) has been deleted

[sandersd (OOO until July 31), 2015-03-23 22:27:59]

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems.cc
File media/base/key_systems.cc (right):

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems....
media/base/key_systems.cc:713: // TODO(sandersd): Make sure that the codecs also
match the |media_type|.
On 2015/03/23 17:22:14, ddorwin wrote:
> Addressed now?

No, this only covers the container, not the codecs yet.

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems.h
File media/base/key_systems.h (right):

https://codereview.chromium.org/1023863002/diff/60001/media/base/key_systems....
media/base/key_systems.h:18: // only intended to support the unprefixed API.
On 2015/03/23 17:22:14, ddorwin wrote:
> ... support unprefixed EME.

Done.

[sandersd (OOO until July 31), 2015-03-24 18:32:00]

Patchset #7 (id:160001) has been deleted

[sandersd (OOO until July 31), 2015-03-24 19:07:05]

Patchset #7 (id:170006) has been deleted

[sandersd (OOO until July 31), 2015-03-24 20:14:20]

Patchset #7 (id:190001) has been deleted

[sandersd (OOO until July 31), 2015-03-24 20:14:27]

Patchset #7 (id:210001) has been deleted

[sandersd (OOO until July 31), 2015-03-25 01:02:31]

Patchset #9 (id:270001) has been deleted

[sandersd (OOO until July 31), 2015-03-25 20:17:41]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2015-03-25 20:17:41]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1023863002/#ps290001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-03-25 20:17:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023863002/290001

[commit-bot: I haz the power, 2015-03-25 21:30:35]

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

[commit-bot: I haz the power, 2015-03-25 21:30:36]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sandersd (OOO until July 31), 2015-03-25 21:31:03]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2015-03-25 21:31:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023863002/290001

[commit-bot: I haz the power, 2015-03-25 22:22:42]

Committed patchset #9 (id:290001)

[commit-bot: I haz the power, 2015-03-25 22:23:17]

Patchset 9 (id:??) landed as
https://crrev.com/574df66641c3663eb27428fa48e940e4358a27f8
Cr-Commit-Position: refs/heads/master@{#322248}