Convert Widevine and Android platform key systems to KeySystemProperties

Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438
Committed: https://crrev.com/bd92f38eb9ef9cad8e3b1d57b9d8458d44ea9e11
Cr-Commit-Position: refs/heads/master@{#390571}

[halliwell, 2016-04-27 03:20:21]

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

[commit-bot: I haz the power, 2016-04-27 03:20:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926533002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926533002/20001

[halliwell, 2016-04-27 03:20:37]

Description was changed from

==========
Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438
==========

to

==========
Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438
==========

[halliwell, 2016-04-27 03:20:37]

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

[commit-bot: I haz the power, 2016-04-27 03:24:57]

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

[commit-bot: I haz the power, 2016-04-27 03:24:58]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[xhwang, 2016-04-27 19:28:39]

lgtm % nits

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chrome/renderer/...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chrome/renderer/...
chrome/renderer/chrome_content_renderer_client.cc:1249: void
ChromeContentRendererClient::AddSupportedKeySystems(
It might be worth mentioning in the content client API or somewhere else that
AddKeySystems() and AddSupportedKeySystems() are complementing each other today.
Otherwise the change in chrome_key_systems.cc would be a bit confusing since
externalclearkey uses the old way but widevine is using the new one.

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
File chromecast/renderer/cast_content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
chromecast/renderer/cast_content_renderer_client.h:38: std::vector<
::media::KeySystemInfo>* key_systems) override;
key_systems_info to be consistent?

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
File chromecast/renderer/key_systems_cast.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
chromecast/renderer/key_systems_cast.cc:60:
::media::EmeRobustness::HW_SECURE_ALL,          // Max audio robustness.
OOC, how did this work before without ::media::?

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
File chromecast/renderer/key_systems_cast.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
chromecast/renderer/key_systems_cast.h:22: std::vector<::media::KeySystemInfo>*
concrete_key_systems);
nit: It seems this can be a static function in the .cc file.

[halliwell, 2016-04-27 21:58:50]

halliwell@chromium.org changed reviewers:
+ michaelbai@chromium.org, sky@chromium.org

[halliwell, 2016-04-27 22:20:13]

+michaelbai for android_webview/
+sky for chrome/

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chrome/renderer/...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chrome/renderer/...
chrome/renderer/chrome_content_renderer_client.cc:1249: void
ChromeContentRendererClient::AddSupportedKeySystems(
On 2016/04/27 19:28:39, xhwang wrote:
> It might be worth mentioning in the content client API or somewhere else that
> AddKeySystems() and AddSupportedKeySystems() are complementing each other
today.
> Otherwise the change in chrome_key_systems.cc would be a bit confusing since
> externalclearkey uses the old way but widevine is using the new one.

It is already mentioned that the KeySystemInfo API is deprecated, with a TODO to
move everything to new API.  I think I only need one more CL to finish the move.

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
File chromecast/renderer/cast_content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
chromecast/renderer/cast_content_renderer_client.h:38: std::vector<
::media::KeySystemInfo>* key_systems) override;
On 2016/04/27 19:28:39, xhwang wrote:
> key_systems_info to be consistent?

Done.

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
File chromecast/renderer/key_systems_cast.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
chromecast/renderer/key_systems_cast.cc:60:
::media::EmeRobustness::HW_SECURE_ALL,          // Max audio robustness.
On 2016/04/27 19:28:39, xhwang wrote:
> OOC, how did this work before without ::media::?

Oh, there are some using statements at the top.  I think the ::media got
introduced here because I initially put this code directly into the content
client code, then I moved it back here.  I removed the ::media now to take
advantage of the existing using statements.

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
File chromecast/renderer/key_systems_cast.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/20001/chromecast/rende...
chromecast/renderer/key_systems_cast.h:22: std::vector<::media::KeySystemInfo>*
concrete_key_systems);
On 2016/04/27 19:28:39, xhwang wrote:
> nit: It seems this can be a static function in the .cc file.

Good catch.  It was here originally for us to register PlayReady by a variety of
key system names in the old prefixed EME days.  That code is deleted now.

[michaelbai, 2016-04-27 22:34:16]

android_webview lgtm

[ddorwin, 2016-04-27 22:41:53]

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

[ddorwin, 2016-04-27 22:41:54]

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.cc:227:
AddExternalClearKey(key_systems_info);
Should we just convert ECK in this CL. Then we wouldn't need both methods in
ChromeContentRendererClient.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chromecast/rende...
File chromecast/renderer/key_systems_cast.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chromecast/rende...
chromecast/renderer/key_systems_cast.cc:53: #if defined(WIDEVINE_CDM_AVAILABLE)
The header that defines this was removed. Does chromecast always define it? If
so, this wouldn't be necessary. Or maybe it is conditionally defined in the
build files?

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chromecast/rende...
chromecast/renderer/key_systems_cast.cc:75:
AddKeySystemWithCodecs(media::kChromecastPlayreadyKeySystem,
If ECK is converted, this would be the only reason both methods are necessary in
the public interface.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
File components/cdm/renderer/android_key_systems.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:39: : name_(name),
supported_codecs_(supported_codecs) {}
In the future, we'll need some value other than the name to pass across Mojo.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:46: // |init_data_type| x
|container| pairings.
What does this mean?

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:47: if (init_data_type ==
EmeInitDataType::WEBM)
Can we force the caller to make this assumption instead? I guess that's
independent of this CL.
See also https://bugs.chromium.org/p/chromium/issues/detail?id=589521 (and
https://bugs.chromium.org/p/chromium/issues/detail?id=589515).

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:50: if (init_data_type ==
EmeInitDataType::CENC)
Use a switch statement instead?
Then explicitly list KEYIDS.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:82: std::string name_;
nit: can be consts

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/widevine_key_system_properties.cc:72: if (init_data_type
== EmeInitDataType::WEBM)
We should *really* fix this for Widevine. Again, a separate CL.
https://bugs.chromium.org/p/chromium/issues/detail?id=589521

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/widevine_key_system_properties.h:8: #include
"media/base/key_system_properties.h"
#include "build/build_config.h"

key_system_properties.h needs this too.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/widevine_key_system_properties.h:27: // Gets the name of
this key system.
As in a previous CL, do we need all these comments for overrides?
If anything, you would want to describe unique properties of this class's
implementation.

[halliwell, 2016-04-27 23:50:46]

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.cc:227:
AddExternalClearKey(key_systems_info);
On 2016/04/27 22:41:54, ddorwin wrote:
> Should we just convert ECK in this CL. Then we wouldn't need both methods in
> ChromeContentRendererClient.

We could, was just trying to break up the changes to keep reviews more
manageable in size.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chromecast/rende...
File chromecast/renderer/key_systems_cast.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chromecast/rende...
chromecast/renderer/key_systems_cast.cc:53: #if defined(WIDEVINE_CDM_AVAILABLE)
On 2016/04/27 22:41:54, ddorwin wrote:
> The header that defines this was removed. Does chromecast always define it? If
> so, this wouldn't be necessary. Or maybe it is conditionally defined in the
> build files?

Good point, restored the header for now (we might be able to make it
unconditional soon).

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chromecast/rende...
chromecast/renderer/key_systems_cast.cc:75:
AddKeySystemWithCodecs(media::kChromecastPlayreadyKeySystem,
On 2016/04/27 22:41:54, ddorwin wrote:
> If ECK is converted, this would be the only reason both methods are necessary
in
> the public interface.

Yep, I was planning to do ECK and Chromecast PR next, then can remove
KeySystemInfo and everything related.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
File components/cdm/renderer/android_key_systems.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:39: : name_(name),
supported_codecs_(supported_codecs) {}
On 2016/04/27 22:41:54, ddorwin wrote:
> In the future, we'll need some value other than the name to pass across Mojo.

Acknowledged.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:46: // |init_data_type| x
|container| pairings.
On 2016/04/27 22:41:54, ddorwin wrote:
> What does this mean?

This is a copy-paste from key_systems.cc.  Do you or xhwang@ know? :)

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:47: if (init_data_type ==
EmeInitDataType::WEBM)
On 2016/04/27 22:41:54, ddorwin wrote:
> Can we force the caller to make this assumption instead? I guess that's
> independent of this CL.
> See also https://bugs.chromium.org/p/chromium/issues/detail?id=589521 (and
> https://bugs.chromium.org/p/chromium/issues/detail?id=589515).

Ack, follow up on those bugs separately?

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:50: if (init_data_type ==
EmeInitDataType::CENC)
On 2016/04/27 22:41:54, ddorwin wrote:
> Use a switch statement instead?
> Then explicitly list KEYIDS.

Done.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/android_key_systems.cc:82: std::string name_;
On 2016/04/27 22:41:54, ddorwin wrote:
> nit: can be consts

Done.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
File components/cdm/renderer/widevine_key_system_properties.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/widevine_key_system_properties.cc:72: if (init_data_type
== EmeInitDataType::WEBM)
On 2016/04/27 22:41:54, ddorwin wrote:
> We should *really* fix this for Widevine. Again, a separate CL.
> https://bugs.chromium.org/p/chromium/issues/detail?id=589521

Acknowledged.

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
File components/cdm/renderer/widevine_key_system_properties.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/widevine_key_system_properties.h:8: #include
"media/base/key_system_properties.h"
On 2016/04/27 22:41:54, ddorwin wrote:
> #include "build/build_config.h"
> 
> key_system_properties.h needs this too.

Both done (I guess we were picking them up accidentally through
key_system_info.h)

https://chromiumcodereview.appspot.com/1926533002/diff/40001/components/cdm/r...
components/cdm/renderer/widevine_key_system_properties.h:27: // Gets the name of
this key system.
On 2016/04/27 22:41:54, ddorwin wrote:
> As in a previous CL, do we need all these comments for overrides?
> If anything, you would want to describe unique properties of this class's
> implementation.

Agreed, removed.

[ddorwin, 2016-04-28 01:05:59]

lgtm

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://chromiumcodereview.appspot.com/1926533002/diff/40001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.cc:227:
AddExternalClearKey(key_systems_info);
On 2016/04/27 23:50:45, halliwell wrote:
> On 2016/04/27 22:41:54, ddorwin wrote:
> > Should we just convert ECK in this CL. Then we wouldn't need both methods in
> > ChromeContentRendererClient.
> 
> We could, was just trying to break up the changes to keep reviews more
> manageable in size.

Thanks for considering that. In this case, replacing the functions might have
been more beneficial than whatever additional lines that would have added.

[halliwell, 2016-04-28 03:36:16]

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

[commit-bot: I haz the power, 2016-04-28 03:36:34]

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

[commit-bot: I haz the power, 2016-04-28 04:03:00]

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

[commit-bot: I haz the power, 2016-04-28 04:03:01]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[sky, 2016-04-28 15:30:09]

LGTM

https://chromiumcodereview.appspot.com/1926533002/diff/80001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/80001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.h:12: #include
"media/base/key_system_properties.h"
nit: forward declare classes.

https://chromiumcodereview.appspot.com/1926533002/diff/80001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.h:14: void AddChromeKeySystemsInfo(
This may mean something to you, but it means nothing to me. Please add
descriptions as to what these functions do.

[halliwell, 2016-04-28 22:55:36]

https://chromiumcodereview.appspot.com/1926533002/diff/80001/chrome/renderer/...
File chrome/renderer/media/chrome_key_systems.h (right):

https://chromiumcodereview.appspot.com/1926533002/diff/80001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.h:12: #include
"media/base/key_system_properties.h"
On 2016/04/28 15:30:09, sky wrote:
> nit: forward declare classes.

Done.

https://chromiumcodereview.appspot.com/1926533002/diff/80001/chrome/renderer/...
chrome/renderer/media/chrome_key_systems.h:14: void AddChromeKeySystemsInfo(
On 2016/04/28 15:30:09, sky wrote:
> This may mean something to you, but it means nothing to me. Please add
> descriptions as to what these functions do.

Done.

[halliwell, 2016-04-28 22:55:47]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2016-04-28 22:55:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
michaelbai@chromium.org, ddorwin@chromium.org, sky@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1926533002/#ps100001 (title: "sky nits")

[commit-bot: I haz the power, 2016-04-28 22:56:06]

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

[commit-bot: I haz the power, 2016-04-29 00:15:54]

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

[commit-bot: I haz the power, 2016-04-29 00:15:56]

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

[halliwell, 2016-04-29 00:45:30]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2016-04-29 00:45:53]

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

[commit-bot: I haz the power, 2016-04-29 01:55:52]

Description was changed from

==========
Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438
==========

to

==========
Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438
==========

[commit-bot: I haz the power, 2016-04-29 01:55:54]

Committed patchset #6 (id:100001)

[Justin Donnelly, 2016-04-29 02:41:28]

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

The reason for reverting is: Broke Google Chrome Mac bot..

[Justin Donnelly, 2016-04-29 02:41:51]

On 2016/04/29 02:41:28, Justin Donnelly wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/1932893004/ by mailto:jdonnelly@chromium.org.
> 
> The reason for reverting is: Broke Google Chrome Mac bot..

See
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/b...

[commit-bot: I haz the power, 2016-04-30 17:23:44]

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

[commit-bot: I haz the power, 2016-04-30 17:59:02]

Description was changed from

==========
Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438
==========

to

==========
Convert Widevine and Android platform key systems to KeySystemProperties

WidevineKeySystemProperties replaces the use of KeySystemInfo +
InfoBasedKeySystemProperties for widevine (logic is essentially copied).
Also updated Android platform key systems as it's simpler to convert the
APIs in android_key_systems together.

BUG=457438

Committed: https://crrev.com/bd92f38eb9ef9cad8e3b1d57b9d8458d44ea9e11
Cr-Commit-Position: refs/heads/master@{#390571}
==========