Refactor KeySystems code to call a function to populate the info.

Refactor KeySystems code to call a function to populate the info.

This allows KeySystems to call GetContentClient()->AddKeySystems() so we can
move key system information to chrome/. The new ContentClient API is defined and
called, but we still rely on key_systems_info.cc.

BUG=224793
TEST=Existing content_unittests and content_browsertests pass.

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

[ddorwin, 2013-08-30 22:49:43]

xhwang, PTAL at everything.
scherkus, PTAL at a high level.

The next CL will move all the KS details from
content/renderer/media/crypto/key_systems_info.cc to
chrome/common(?)/chrome_key_systems.cc.

This CL includes a lot of refactoring in key_systems_info.cc to make it easy to
add key systems for various platforms when the code is moved. I should be able
to move most of that file directly.

[scherkus (not reviewing), 2013-09-03 17:56:15]

approach seems fine

you should s/2012/2013/ in new files

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
content/renderer/media/crypto/key_systems.cc:127: #if
defined(ENABLE_PEPPER_CDMS)
fix indent

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_info.cc:94: AddWidevineForTypes(true, 
// WebM, VP8 & Vorbis.
considering you call this method once, how about having the #ifdefs do
push_back(...) instead of passing a bool and maintaining another set of if (...)
conditions for the push_back(...) calls?

[ddorwin, 2013-09-03 19:34:59]

jam, please review and provide OWNERS approval for all but the last four files
(media).

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
content/renderer/media/crypto/key_systems.cc:127: #if
defined(ENABLE_PEPPER_CDMS)
On 2013/09/03 17:56:16, scherkus wrote:
> fix indent

Done.

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/3002/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_info.cc:94: AddWidevineForTypes(true, 
// WebM, VP8 & Vorbis.
On 2013/09/03 17:56:16, scherkus wrote:
> considering you call this method once, how about having the #ifdefs do
> push_back(...) instead of passing a bool and maintaining another set of if
(...)
> conditions for the push_back(...) calls?

AddWidevineForTypes() is intended to be used by other platforms, such as
Android. Android will have its own knowledge and detection that will translate
into these bools.

Also, the logic to translate from the ifdef's/bools to the items added is not
straightforward, so that is encapsulated in AddWidevineForTypes(). For the ifdef
version, see original code 42-60.

[jam, 2013-09-03 20:50:52]

https://codereview.chromium.org/23678008/diff/23001/content/public/common/con...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/23678008/diff/23001/content/public/common/con...
content/public/common/content_client.h:106: virtual void
AddKeySystems(std::vector<KeySystemInfo>* key_systems) {}
why is this in ContentClient? it should only be here if it's needed in multiple
child process types (i.e. content/renderer and content/utility or
content/public).

if you only need this in the renderer, then it should be in
ContentRendererClient

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
File content/public/common/key_system_info.cc (right):

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
content/public/common/key_system_info.cc:9: KeySystemInfo::KeySystemInfo(const
std::string& key_system)
nit: if you can inline this constructor in the header and get rid of the cc
file, that's preferred. the reason you wouldn't be able to is clang, but it has
some heuristics that i don't know off the top of my head. so please try that and
see if you get green try runs.

also no need for destructor?

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
File content/public/common/key_system_info.h (right):

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
content/public/common/key_system_info.h:42: // specifies container and codec
combinations supported by |key_system|.
nit: "Specifies"

[scherkus (not reviewing), 2013-09-03 22:22:08]

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:43: bool is_mp4_supported,
fix indenting here

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:64: #if
defined(USE_PROPRIETARY_CODECS)
this seems confusing

if this isn't defined but I decide to pass in true I don't get what I expected

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:113: #endif  //
WIDEVINE_CDM_AVAILABLE
missing defined()

[scherkus (not reviewing), 2013-09-03 22:25:06]

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:105: false,  // No AVC1.
I still wonder if we can clarify this code somehow...

What irks me is it's a bag of bools requiring auxiliary comments to clarify
what's going on.

It also forces us to handle combinations that don't make sense... for example:
1) is_webm_vp8_and_vorbis_supported is always true. Can we add that parameter
when we have code that passes in false?
2) We have code supporting the following combination:
WIDEVINE_CDM_CENC_SUPPORT_AVAILABLE=1
WIDEVINE_CDM_AAC_SUPPORT_AVAILABLE=0
WIDEVINE_CDM_AVC1_SUPPORT_AVAILABLE=0

... which has to be backed up with a DCHECK() in AddWidevineForTypes() and turns
what I assume is a compile-time failure into a run-time failure.

[ddorwin, 2013-09-04 00:41:55]

Everything is fixed except the bool/ifdef issue, which we may need to discuss
further. That is isolated to one media file, though.

https://codereview.chromium.org/23678008/diff/23001/content/public/common/con...
File content/public/common/content_client.h (right):

https://codereview.chromium.org/23678008/diff/23001/content/public/common/con...
content/public/common/content_client.h:106: virtual void
AddKeySystems(std::vector<KeySystemInfo>* key_systems) {}
On 2013/09/03 20:50:52, jam wrote:
> why is this in ContentClient? it should only be here if it's needed in
multiple
> child process types (i.e. content/renderer and content/utility or
> content/public).
> 
> if you only need this in the renderer, then it should be in
> ContentRendererClient

Done.

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
File content/public/common/key_system_info.cc (right):

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
content/public/common/key_system_info.cc:9: KeySystemInfo::KeySystemInfo(const
std::string& key_system)
On 2013/09/03 20:50:52, jam wrote:
> nit: if you can inline this constructor in the header and get rid of the cc
> file, that's preferred. the reason you wouldn't be able to is clang, but it
has
> some heuristics that i don't know off the top of my head. so please try that
and
> see if you get green try runs.
> 
> also no need for destructor?

Clang is unhappy:

../../content/public/renderer/key_system_info.h:37:3: error: [chromium-style]
Complex constructor has an inlined body.
  explicit KeySystemInfo(const std::string& key_system)
  ^
../../content/public/renderer/key_system_info.h:34:1: error: [chromium-style]
Complex class/struct needs an explicit out-of-line destructor.
struct CONTENT_EXPORT KeySystemInfo {
^

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
File content/public/common/key_system_info.h (right):

https://codereview.chromium.org/23678008/diff/23001/content/public/common/key...
content/public/common/key_system_info.h:42: // specifies container and codec
combinations supported by |key_system|.
On 2013/09/03 20:50:52, jam wrote:
> nit: "Specifies"

Done.

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:43: bool is_mp4_supported,
On 2013/09/03 22:22:08, scherkus wrote:
> fix indenting here

Done. (4 spaces since aligned makes 46 too long)

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:64: #if
defined(USE_PROPRIETARY_CODECS)
On 2013/09/03 22:22:08, scherkus wrote:
> this seems confusing
> 
> if this isn't defined but I decide to pass in true I don't get what I expected

I can add:
#else
  DCHECK(!is_mp4_supported);

OR duplicate the ifdef at line 43 and the existing ifdef mess at line 95.

The latter is probably more correct (don't compile the code) but is messier.
Preference?

I did the latter.

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:105: false,  // No AVC1.
On 2013/09/03 22:25:06, scherkus wrote:
> I still wonder if we can clarify this code somehow...
> 
> What irks me is it's a bag of bools requiring auxiliary comments to clarify
> what's going on.
> 
> It also forces us to handle combinations that don't make sense... for example:
> 1) is_webm_vp8_and_vorbis_supported is always true. Can we add that parameter
> when we have code that passes in false?
We will have this code next week (Android). (I was trying to clean up all the
logic now and just move in the next CL.)
> 2) We have code supporting the following combination:
> WIDEVINE_CDM_CENC_SUPPORT_AVAILABLE=1
> WIDEVINE_CDM_AAC_SUPPORT_AVAILABLE=0
> WIDEVINE_CDM_AVC1_SUPPORT_AVAILABLE=0
>
This is theoretically legit. It means the init data format is supported but you
don't have secure decoders (that's how we've chosen to implement canPlayType()
in Chrome).

It will probably never be the case in reality, though, so we could eliminate
WIDEVINE_CDM_CENC_SUPPORT_AVAILABLE and OR the other two where necessary
(probably just is_aac_supported || is_aac_supported in the other function).
> ... which has to be backed up with a DCHECK() in AddWidevineForTypes() and
turns
> what I assume is a compile-time failure into a run-time failure.

I switched to all bools for two reasons:
 1) Replace complex macro logic with code that is easier to write and
understand.
 2) These defines really only need to apply to Pepper CDMs. Android and other
platforms don't need them (and they really would mean something else - "compile
in support" - since availability is a runtime check.

I would rather keep the ifdefs out of the above function (AddWidevineForTypes).
Maybe we should use (mask) enums so that we don't need the comments. We could
then convert them to bools inside the function so that the actual logic is still
easier to follow.

Let me know what you think.

https://codereview.chromium.org/23678008/diff/23001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:113: #endif  //
WIDEVINE_CDM_AVAILABLE
On 2013/09/03 22:22:08, scherkus wrote:
> missing defined()

Done.

[jam, 2013-09-04 21:11:45]

lgtm

[xhwang, 2013-09-04 21:48:39]

https://codereview.chromium.org/23678008/diff/47001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/23678008/diff/47001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:142: 
remove this empty line? I don't see why AddKeySystems() is special.

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:250: virtual void
AddKeySystems(std::vector<KeySystemInfo>* key_systems);
It's not clear from the signature and comment what this function is supposed to
do. Update the comment that the function is supposed to populate |key_systems|?
Also, can we make the function name more explicit? e.g. GenerateKeySystemInfo?

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/k...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/k...
content/public/renderer/key_system_info.h:5: #ifndef
CONTENT_PUBLIC_COMMON_KEY_SYSTEM_INFO_H_
s/COMMON/RENDERER ?

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/k...
content/public/renderer/key_system_info.h:35: typedef std::pair<std::string,
std::string> ContainerCodecsPair;
Add a comment that the second param can contain multiple codecs? If multiple,
what format are they in? comma separated?

https://codereview.chromium.org/23678008/diff/47001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/47001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:51: { 0xED, 0xEF, 0x8B, 0xA9,
0x79, 0xD6, 0x4A, 0xCE,
Move "{" to the previous line?

https://codereview.chromium.org/23678008/diff/47001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:107: true,  // AVC1.
Shall we define bit flags for these? e.g.
CDM_SUPPORTS_MP4 1
CDM_SUPPORTS_AAC 1 << 1
CDM_SUPPORTS_AVC1 1 << 2

This way we don't need these comments and it's more scalable.

[ddorwin, 2013-09-06 18:29:59]

I addressed feedback in PS8. You'll also want to look at PS9 of
key_systems_info.cc because I had to reimplement part of r221548 as part of the
rebase.

https://codereview.chromium.org/23678008/diff/47001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/23678008/diff/47001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:142: 
On 2013/09/04 21:48:40, xhwang wrote:
> remove this empty line? I don't see why AddKeySystems() is special.

Done.

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/c...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/c...
content/public/renderer/content_renderer_client.h:250: virtual void
AddKeySystems(std::vector<KeySystemInfo>* key_systems);
On 2013/09/04 21:48:40, xhwang wrote:
> It's not clear from the signature and comment what this function is supposed
to
> do. Update the comment that the function is supposed to populate
|key_systems|?
> Also, can we make the function name more explicit? e.g. GenerateKeySystemInfo?

The name and comment are modeled after existing methods in ContentClient:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

I tried to at least make it clear that |key_systems| should be populated. Let me
know if you have other suggestions.

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/k...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/k...
content/public/renderer/key_system_info.h:5: #ifndef
CONTENT_PUBLIC_COMMON_KEY_SYSTEM_INFO_H_
On 2013/09/04 21:48:40, xhwang wrote:
> s/COMMON/RENDERER ?

Done.

https://codereview.chromium.org/23678008/diff/47001/content/public/renderer/k...
content/public/renderer/key_system_info.h:35: typedef std::pair<std::string,
std::string> ContainerCodecsPair;
On 2013/09/04 21:48:40, xhwang wrote:
> Add a comment that the second param can contain multiple codecs? If multiple,
> what format are they in? comma separated?

Done.

https://codereview.chromium.org/23678008/diff/47001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/47001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:51: { 0xED, 0xEF, 0x8B, 0xA9,
0x79, 0xD6, 0x4A, 0xCE,
On 2013/09/04 21:48:40, xhwang wrote:
> Move "{" to the previous line?

Done.

https://codereview.chromium.org/23678008/diff/47001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:107: true,  // AVC1.
On 2013/09/04 21:48:40, xhwang wrote:
> Shall we define bit flags for these? e.g.
> CDM_SUPPORTS_MP4 1
> CDM_SUPPORTS_AAC 1 << 1
> CDM_SUPPORTS_AVC1 1 << 2
> 
> This way we don't need these comments and it's more scalable.

Done.

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:79:
info.supported_types.push_back(std::make_pair(kAudioMp4, kMp4a));
Note: audio/mp4 without a codec is no longer supported.

Is that what we want? The code is certainly simpler, and it's not clear what the
use case would be.

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:83:
info.supported_types.push_back(std::make_pair(kVideoMp4, video_codecs));
Note: video/mp4 without a codec or with only mp4a is no longer supported.

[scherkus (not reviewing), 2013-09-06 19:29:59]

lgtm w/ nits

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:41: WEBM_VP8_AND_VORBIS = 1,
nit: 1 << 0

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:67: const bool
is_webm_vp8_and_vorbis_supported =
if we moved to using enums for readability, why not inline the & checks inside
the if statements?

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:79:
info.supported_types.push_back(std::make_pair(kAudioMp4, kMp4a));
On 2013/09/06 18:30:00, ddorwin wrote:
> Note: audio/mp4 without a codec is no longer supported.
> 
> Is that what we want? The code is certainly simpler, and it's not clear what
the
> use case would be.

SGTM

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:83:
info.supported_types.push_back(std::make_pair(kVideoMp4, video_codecs));
On 2013/09/06 18:30:00, ddorwin wrote:
> Note: video/mp4 without a codec or with only mp4a is no longer supported.

SGTM

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:162:
AddAndroidWidevine(key_systems_info);
does this compile?

it doesn't appear to be in any function scope

[ddorwin, 2013-09-06 21:34:50]

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:41: WEBM_VP8_AND_VORBIS = 1,
On 2013/09/06 19:29:59, scherkus wrote:
> nit: 1 << 0

Done.

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:67: const bool
is_webm_vp8_and_vorbis_supported =
On 2013/09/06 19:29:59, scherkus wrote:
> if we moved to using enums for readability, why not inline the & checks inside
> the if statements?

I find it easier to read (and we avoid duplicating the logic for AAC), but I
changed it.

https://codereview.chromium.org/23678008/diff/83001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:162:
AddAndroidWidevine(key_systems_info);
On 2013/09/06 19:29:59, scherkus wrote:
> does this compile?
> 
> it doesn't appear to be in any function scope

Oops. Accidental paste.

[xhwang, 2013-09-06 22:47:27]

lgtm

https://codereview.chromium.org/23678008/diff/88001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_info.cc (right):

https://codereview.chromium.org/23678008/diff/88001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_info.cc:40: enum SupportedCodecs {
Strictly speaking MP4 and WEBM are container names, not codec. But the name will
be too long if we add container here...

[commit-bot: I haz the power, 2013-09-06 22:55:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/23678008/88001

[commit-bot: I haz the power, 2013-09-07 11:36:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-07 18:32:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ddorwin@chromium.org/23678008/88001

[commit-bot: I haz the power, 2013-09-07 23:28:29]

Change committed as 221932