Move EME key system knowledge from content/ to chrome/.

Move EME key system knowledge from content/ to chrome/.

Most of the knowledge about specific key systems is moved from
content/renderer/media/crypto/key_systems_info.cc to
chrome/renderer/media/chrome_key_systems.cc.

content/renderer/media/crypto/key_systems_unittest.cc is modified to test the
infrastructure rather than specific key systems, which are now tested in
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (r222647).

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

[ddorwin, 2013-09-11 01:37:41]

https://codereview.chromium.org/23452025/diff/7001/chrome/renderer/media/OWNERS
File chrome/renderer/media/OWNERS (right):

https://codereview.chromium.org/23452025/diff/7001/chrome/renderer/media/OWNE...
chrome/renderer/media/OWNERS:1: acolwell@chromium.org
I copied this from chrome/browser/media/OWNERS.

[xhwang, 2013-09-11 20:38:55]

https://codereview.chromium.org/23452025/diff/7001/chrome/renderer/media/chro...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/23452025/diff/7001/chrome/renderer/media/chro...
chrome/renderer/media/chrome_key_systems.cc:44: static void
AddWidevineForCodecs(
nit: will AddWidevineWithCodecs be better?

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:40: static const char*
const kTestParent = "com.example";
It'll be nice to have better comment and naming. E.g. kTest will be registered.

About naming: kClear is close to ClearKey. And it's unclear that kTest will be
registered. I don't have a good suggestion now. How about kUnregistered,
kUnsupported, kBad...? Or kTestSupported and kTestUnsupported?

Also, what's the relation between these and the kUnrecognized on line 202? More
comments would help.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:44: static const char*
const kExternalClearKey = "org.chromium.externalclearkey";
use "static const char Foo[]" to be consistent with the list below?

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:53: static const char
kFooAudioCodecs[] = "baraudio";
kBarAudioCodecs?

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:54: static const char
kFooVideoCodecs[] = "baraudio,barvideo";
ditto

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:120:
baraudio_codec_.push_back("baraudio");
When should we use baraudio_codec_ and when should we use kFooAudioCodecs? It's
a bit confusing to me.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:189:
KeySystemNameForUMA(WebString::fromUTF8(kClearKey)).c_str());
This pattern is used in a lot of times, worth wrapping it into a helper
function?

EXPECT_TRUE(VerifyKeySystemNameForUMA("ClearKey", kClearKey));

or just VerifyUMAName()

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:192:
EXPECT_FALSE(IsConcreteSupportedKeySystem("org.w3.clearkey"));
I see both 

IsConcreteSupportedKeySystem("org.w3.clearkey")

and 

IsConcreteSupportedKeySystem(WebString::fromUTF8(kClearKey))

Shall we consolidate them? Ideally it'll be nice is we can hide the
WebString::fromUTF8() part. It really doesn't add any value to the test but
noise.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:198: 
drop empty line.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:293: "org.example is not a
known concrete system");
nit: is there a way to use kClearParent in the log instead of using the raw
string directly? If it's hard then nevermind.

[ddorwin, 2013-09-12 00:54:18]

jochen: Please review changes in chrome/renderer.

They are small, and the new OWNERS file means we will own most of those files in
the future.

xhwang: I will address your changes tomorrow.

[ddorwin, 2013-09-12 22:35:40]

https://codereview.chromium.org/23452025/diff/7001/chrome/renderer/media/chro...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/23452025/diff/7001/chrome/renderer/media/chro...
chrome/renderer/media/chrome_key_systems.cc:44: static void
AddWidevineForCodecs(
On 2013/09/11 20:38:55, xhwang wrote:
> nit: will AddWidevineWithCodecs be better?

Done.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:40: static const char*
const kTestParent = "com.example";
On 2013/09/11 20:38:55, xhwang wrote:
> It'll be nice to have better comment and naming. E.g. kTest will be
registered.
> 
> About naming: kClear is close to ClearKey. And it's unclear that kTest will be
> registered. I don't have a good suggestion now. How about kUnregistered,
> kUnsupported, kBad...? Or kTestSupported and kTestUnsupported?
> 
> Also, what's the relation between these and the kUnrecognized on line 202?
More
> comments would help.

Done. I tried to clarify these things.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:44: static const char*
const kExternalClearKey = "org.chromium.externalclearkey";
On 2013/09/11 20:38:55, xhwang wrote:
> use "static const char Foo[]" to be consistent with the list below?

Done.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:53: static const char
kFooAudioCodecs[] = "baraudio";
On 2013/09/11 20:38:55, xhwang wrote:
> kBarAudioCodecs?

It's the codecs for the Foo container. It makes more sense at line 74. I fixed
48-49 to match.

Also, based on feedback below, I changed the codec names to match the container
name. It's weird, but easier to understand since these aren't real types.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:54: static const char
kFooVideoCodecs[] = "baraudio,barvideo";
On 2013/09/11 20:38:55, xhwang wrote:
> ditto

Same

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:120:
baraudio_codec_.push_back("baraudio");
On 2013/09/11 20:38:55, xhwang wrote:
> When should we use baraudio_codec_ and when should we use kFooAudioCodecs?
It's
> a bit confusing to me.

kFooAudioCodecs is the codecs supported by that container. These are used for
individual tests.

I could name it kRegisteredFooAudioCodecs, but that would cause extra lines.
where they are used.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:189:
KeySystemNameForUMA(WebString::fromUTF8(kClearKey)).c_str());
On 2013/09/11 20:38:55, xhwang wrote:
> This pattern is used in a lot of times, worth wrapping it into a helper
> function?
> 
> EXPECT_TRUE(VerifyKeySystemNameForUMA("ClearKey", kClearKey));
> 
> or just VerifyUMAName()

I want to keep the comparison here for failure readability, but I created a
helper for WebString conversion and switched to EXPECT_EQ.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:192:
EXPECT_FALSE(IsConcreteSupportedKeySystem("org.w3.clearkey"));
On 2013/09/11 20:38:55, xhwang wrote:
> I see both 
> 
> IsConcreteSupportedKeySystem("org.w3.clearkey")
> 
> and 
> 
> IsConcreteSupportedKeySystem(WebString::fromUTF8(kClearKey))
> 
> Shall we consolidate them? Ideally it'll be nice is we can hide the
> WebString::fromUTF8() part. It really doesn't add any value to the test but
> noise.

The difference was unprefixed vs. prefixed. I've named constants to make this
clear.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:198: 
On 2013/09/11 20:38:55, xhwang wrote:
> drop empty line.

Done.

https://codereview.chromium.org/23452025/diff/7001/content/renderer/media/cry...
content/renderer/media/crypto/key_systems_unittest.cc:293: "org.example is not a
known concrete system");
On 2013/09/11 20:38:55, xhwang wrote:
> nit: is there a way to use kClearParent in the log instead of using the raw
> string directly? If it's hard then nevermind.

I think it would require #defining the constant so we can append literals or
converting the constant or literal string here to an std::string() so we can
append them. The first is undesirable; the second is a lot more code.

[xhwang, 2013-09-12 23:47:49]

lgtm % nits

https://codereview.chromium.org/23452025/diff/28001/chrome/browser/media/encr...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/23452025/diff/28001/chrome/browser/media/encr...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:98: static
const char kUprefixedClearKeyParent[] = "org.w3";
s/U/Un/

Actually, I'll be okay with just kClearKey. (unprefixed by default)

https://codereview.chromium.org/23452025/diff/28001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/23452025/diff/28001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_unittest.cc:47: static const char
kUprefixedClearKey[] = "org.w3.clearkey";
ditto

https://codereview.chromium.org/23452025/diff/28001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_unittest.cc:84: KeySystemInfo
clear(kUsesAes);
|clear| seems funny here. But uses_aes isn't great either. up to you :)

https://codereview.chromium.org/23452025/diff/28001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_unittest.cc:96: KeySystemInfo
test(kExternal);
ditto

[ddorwin, 2013-09-13 00:33:42]

jochen, OWNERS review for content/ please. Almost all the files will have new
OWNERS after this CL.

xhwang, I lost your comments but addressed all of them.

https://codereview.chromium.org/23452025/diff/64001/chrome/browser/media/encr...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):

https://codereview.chromium.org/23452025/diff/64001/chrome/browser/media/encr...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:97: static
const char kUnprefixedClearKey[] = "org.w3.clearkey";
We'll keep "Unprefixed" until it becomes the default. Otherwise, I think it
could be confusing.

https://codereview.chromium.org/23452025/diff/64001/content/renderer/media/cr...
File content/renderer/media/crypto/key_systems_unittest.cc (right):

https://codereview.chromium.org/23452025/diff/64001/content/renderer/media/cr...
content/renderer/media/crypto/key_systems_unittest.cc:96: KeySystemInfo
ext(kExternal);
"ext" isn't a good name, but it keeps 98-102,109 on one line.

[xhwang, 2013-09-13 00:44:07]

lgtm++

[ddorwin, 2013-09-13 15:13:55]

jochen, thestig: PTAL at chrome/. I have LGTM from my team.

[Lei Zhang, 2013-09-13 18:05:38]

chrome/ lgtm

[commit-bot: I haz the power, 2013-09-13 18:08:29]

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

[commit-bot: I haz the power, 2013-09-13 21:51:51]

Change committed as 223132