Initial implementation of Encrypted Media Extensions in Chrome.

Initial implementation of Encrypted Media Extensions in Chrome.

The new encrypted-media layout tests pass, but the key is not used by the media stack yet.

BUG=119668
TEST=LayoutTests/media/encrypted-media/

R=scherkus@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132973

[scherkus (not reviewing), 2012-04-12 20:18:41]

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/glue/simple_web...
File webkit/glue/simple_webmimeregistry_impl.cc (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/glue/simple_web...
webkit/glue/simple_webmimeregistry_impl.cc:60: const WebString& keySystem) {
key_system

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/glue/simple_web...
webkit/glue/simple_webmimeregistry_impl.cc:73:
net::ParseCodecString(ToASCIIOrEmpty(codecs).c_str(),
does this need to be c_str()? the calling function accepts std::string and
ToASCIIOrEmpty() also returns an std::string

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/glue/simple_web...
webkit/glue/simple_webmimeregistry_impl.cc:78:
ToASCIIOrEmpty(mime_type).c_str(),
ditto here

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
File webkit/media/supported_key_systems.cc (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.cc:53: if
(!IsSupportedKeySystemWithContainerAndCodec(mime_type,
nit: move all these params to next line so it looks less spaced out :)

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.cc:67: i <
arraysize(supported_format_key_system_combinations);
nit: this should be aligned to the (

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.cc:71: if (combination.mime_type == 
mime_type &&
we're comparing a const char* to an std::string

as a sanity check are we sure this is doing the right comparison?

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.cc:72: combination.codec ==  codec &&
nit: remove extra space you have after == here + above + below

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
File webkit/media/supported_key_systems.h (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.h:8: //#include "base/compiler_specific.h"
??

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.h:18: class SupportedKeySystems {
this might as well be a namespace for now as the class provides no real value.
I'd possible even rename this file + namespace to mime_util considering its
intended usage.

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/supported...
webkit/media/supported_key_systems.h:28: static bool
IsSupportedKeySystemWithContainerAndCodec(
nit: might as well move this into the .cc

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:675:
WebMediaPlayerImpl::generateKeyRequest(const WebKit::WebString& keySystem,
chromium style unix_hacker here + below

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:681: // Every request call creates a unique
ID.
how unique is unique supposed to be?

unique per page? unique per domain? globally unique during the lifetime of the
browser? globally unique of all time?

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:682: // Increment the ID here to avoid race
conditions in task threads.
which race conditions are you referring to?

if it's per-page uniqueness, then there are no race conditions to worry about
here as this code and the tasks you're posting are running on a single thread

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:683: static uint32_t nextAvailableSessionId
= 1;
unix_hacker style

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:686: MessageLoop::current()->PostTask(
nit: our preferred style for PostTask is:

PostTask(FROM_HERE, base::Bind(
____&Foo::Bar, arg1, arg2, ...));

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:990: void
WebMediaPlayerImpl::generateKeyRequestTask(
you don't need any of these methods

considering they're temporary, I'd fold them inside your existing methods and
PostTask() a base::Bind() on the WebMediaPlayerClient

for example in gKR() you can have:

MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
    &WebMediaPlayerClient::keyMessage, base::Unretained(GetClient()),
    key_system, session_id, message, message_length));

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:995: WebKit::WebString
sessionIdSring(base::UintToString16(sessionId));
Sring -> String?

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:1020:
static_cast<size_t>(keyLength)).c_str() << ": "
why c_str()?

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:1033: if (keyLength !=
kSupportedKeyLengthBytes) {
these constants seemed overly verbose for temporary code -- why not

kSupportedKeyLength = 16;  // 128-bit key.
key_length != kSupportedKeyLength?

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.cc:1043: void
WebMediaPlayerImpl::cancelKeyRequestTask(
remove -- add this method when we have an impl

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
File webkit/media/webmediaplayer_impl.h (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/media/webmediap...
webkit/media/webmediaplayer_impl.h:188: const WebKit::WebString& keySystem,
chromium style unix_hacker here + below

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/tools/test_shel...
File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/tools/test_shel...
webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:39: const
WebKit::WebString& keySystem) {
key_system

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/tools/test_shel...
webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:48: return
SimpleWebMimeRegistryImpl::supportsMediaMIMEType(mime_type,
nit: I'd just drop all args to next line + indent by 4

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/tools/test_shel...
File webkit/tools/test_shell/test_shell_webmimeregistry_impl.h (right):

https://chromiumcodereview.appspot.com/10020053/diff/1/webkit/tools/test_shel...
webkit/tools/test_shell/test_shell_webmimeregistry_impl.h:33: const
WebKit::WebString& keySystem) OVERRIDE;
nit: remove arg name

[ddorwin, 2012-04-12 23:41:23]

http://codereview.chromium.org/10020053/diff/1/webkit/glue/simple_webmimeregi...
File webkit/glue/simple_webmimeregistry_impl.cc (right):

http://codereview.chromium.org/10020053/diff/1/webkit/glue/simple_webmimeregi...
webkit/glue/simple_webmimeregistry_impl.cc:60: const WebString& keySystem) {
On 2012/04/12 20:18:41, scherkus wrote:
> key_system

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/glue/simple_webmimeregi...
webkit/glue/simple_webmimeregistry_impl.cc:73:
net::ParseCodecString(ToASCIIOrEmpty(codecs).c_str(),
On 2012/04/12 20:18:41, scherkus wrote:
> does this need to be c_str()? the calling function accepts std::string and
> ToASCIIOrEmpty() also returns an std::string

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/glue/simple_webmimeregi...
webkit/glue/simple_webmimeregistry_impl.cc:78:
ToASCIIOrEmpty(mime_type).c_str(),
On 2012/04/12 20:18:41, scherkus wrote:
> ditto here

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
File webkit/media/supported_key_systems.cc (right):

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.cc:53: if
(!IsSupportedKeySystemWithContainerAndCodec(mime_type,
On 2012/04/12 20:18:41, scherkus wrote:
> nit: move all these params to next line so it looks less spaced out :)

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.cc:67: i <
arraysize(supported_format_key_system_combinations);
On 2012/04/12 20:18:41, scherkus wrote:
> nit: this should be aligned to the (

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.cc:71: if (combination.mime_type == 
mime_type &&
On 2012/04/12 20:18:41, scherkus wrote:
> we're comparing a const char* to an std::string
> 
> as a sanity check are we sure this is doing the right comparison?

Handled by basic_string::operator==(const charT* s1, ...).

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.cc:72: combination.codec ==  codec &&
On 2012/04/12 20:18:41, scherkus wrote:
> nit: remove extra space you have after == here + above + below

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
File webkit/media/supported_key_systems.h (right):

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.h:8: //#include "base/compiler_specific.h"
On 2012/04/12 20:18:41, scherkus wrote:
> ??

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.h:18: class SupportedKeySystems {
On 2012/04/12 20:18:41, scherkus wrote:
> this might as well be a namespace for now as the class provides no real value.
> I'd possible even rename this file + namespace to mime_util considering its
> intended usage.

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/supported_key_sys...
webkit/media/supported_key_systems.h:28: static bool
IsSupportedKeySystemWithContainerAndCodec(
On 2012/04/12 20:18:41, scherkus wrote:
> nit: might as well move this into the .cc

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:675:
WebMediaPlayerImpl::generateKeyRequest(const WebKit::WebString& keySystem,
On 2012/04/12 20:18:41, scherkus wrote:
> chromium style unix_hacker here + below

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:681: // Every request call creates a unique
ID.
On 2012/04/12 20:18:41, scherkus wrote:
> how unique is unique supposed to be?
> 
> unique per page? unique per domain? globally unique during the lifetime of the
> browser? globally unique of all time?

Whatever is necessary for the CDM to "associate calls related to a key/license
lifetime".

For an in-renderer CDM, this works since it is unique across the process. For an
out-of-process CDM, it would need to be global. This solution only works for
in-process Clear Key. I've added a TODO.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:682: // Increment the ID here to avoid race
conditions in task threads.
On 2012/04/12 20:18:41, scherkus wrote:
> which race conditions are you referring to?
> 
> if it's per-page uniqueness, then there are no race conditions to worry about
> here as this code and the tasks you're posting are running on a single thread

For now (see above), per-page uniqueness is fine. For some reason I was thinking
this might post to a thread pool. Changed.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:683: static uint32_t nextAvailableSessionId
= 1;
On 2012/04/12 20:18:41, scherkus wrote:
> unix_hacker style

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:686: MessageLoop::current()->PostTask(
On 2012/04/12 20:18:41, scherkus wrote:
> nit: our preferred style for PostTask is:
> 
> PostTask(FROM_HERE, base::Bind(
> ____&Foo::Bar, arg1, arg2, ...));

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:990: void
WebMediaPlayerImpl::generateKeyRequestTask(
On 2012/04/12 20:18:41, scherkus wrote:
> you don't need any of these methods
> 
> considering they're temporary, I'd fold them inside your existing methods and
> PostTask() a base::Bind() on the WebMediaPlayerClient
> 
> for example in gKR() you can have:
> 
> MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
>     &WebMediaPlayerClient::keyMessage, base::Unretained(GetClient()),
>     key_system, session_id, message, message_length));
I kept the first two functions but removed the third.

I don't think they are necessarily temporary, though someday we may end up
posting to the decoder thread.

Also, the first two functions don't just call the client callback. This function
now creates the ID. addKeyTask() has more than one branch, and will be calling a
decrypter function in the next CL.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:995: WebKit::WebString
sessionIdSring(base::UintToString16(sessionId));
On 2012/04/12 20:18:41, scherkus wrote:
> Sring -> String?

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:1020:
static_cast<size_t>(keyLength)).c_str() << ": "
On 2012/04/12 20:18:41, scherkus wrote:
> why c_str()?

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:1033: if (keyLength !=
kSupportedKeyLengthBytes) {
On 2012/04/12 20:18:41, scherkus wrote:
> these constants seemed overly verbose for temporary code -- why not
> 
> kSupportedKeyLength = 16;  // 128-bit key.
> key_length != kSupportedKeyLength?

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:1043: void
WebMediaPlayerImpl::cancelKeyRequestTask(
On 2012/04/12 20:18:41, scherkus wrote:
> remove -- add this method when we have an impl

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/10020053/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.h:188: const WebKit::WebString& keySystem,
On 2012/04/12 20:18:41, scherkus wrote:
> chromium style unix_hacker here + below

Done.
Too much copying of code from WebKit. :)

http://codereview.chromium.org/10020053/diff/1/webkit/tools/test_shell/test_s...
File webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc (right):

http://codereview.chromium.org/10020053/diff/1/webkit/tools/test_shell/test_s...
webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:39: const
WebKit::WebString& keySystem) {
On 2012/04/12 20:18:41, scherkus wrote:
> key_system

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/tools/test_shell/test_s...
webkit/tools/test_shell/test_shell_webmimeregistry_impl.cc:48: return
SimpleWebMimeRegistryImpl::supportsMediaMIMEType(mime_type,
On 2012/04/12 20:18:41, scherkus wrote:
> nit: I'd just drop all args to next line + indent by 4

Done.

http://codereview.chromium.org/10020053/diff/1/webkit/tools/test_shell/test_s...
File webkit/tools/test_shell/test_shell_webmimeregistry_impl.h (right):

http://codereview.chromium.org/10020053/diff/1/webkit/tools/test_shell/test_s...
webkit/tools/test_shell/test_shell_webmimeregistry_impl.h:33: const
WebKit::WebString& keySystem) OVERRIDE;
On 2012/04/12 20:18:41, scherkus wrote:
> nit: remove arg name

Done.

[scherkus (not reviewing), 2012-04-13 02:43:59]

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.cc
File webkit/media/mime_util.cc (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.cc#n...
webkit/media/mime_util.cc:6: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebString.h"
one blank line separating first header + rest

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.cc#n...
webkit/media/mime_util.cc:21: 
nit: remove extra blank line

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h
File webkit/media/mime_util.h (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h#ne...
webkit/media/mime_util.h:5: #ifndef WEBKIT_MEDIA_SUPPORTED_KEY_SYSTEMS_IMPL_H_
header guard is incorrect

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h#ne...
webkit/media/mime_util.h:19: bool isKeySystemSupported(const WebKit::WebString&
keySystem);
docs on these methods and unix_hacker args

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h#ne...
webkit/media/mime_util.h:30: #endif  //
WEBKIT_MEDIA_SUPPORTED_KEY_SYSTEMS_IMPL_H_
don't forget this one!

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:682:
&WebMediaPlayerImpl::generateKeyRequestTask,
this (and rest of args) should be indented 4 spaces from "M" (i.e., 6 spaces
from start of line)

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:977: void
WebMediaPlayerImpl::generateKeyRequestTask(
per email discussion just nuke these tasks for now

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:983: // create their own IDs and since CDMs
supporting multiple renderer processes
I do have to say this is one area that makes me think may require spec
changes/clarifications

what are the restrictions on session ids, if any?

[ddorwin, 2012-04-13 21:48:49]

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.cc
File webkit/media/mime_util.cc (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.cc#n...
webkit/media/mime_util.cc:6: #include
"third_party/WebKit/Source/WebKit/chromium/public/platform/WebString.h"
On 2012/04/13 02:43:59, scherkus wrote:
> one blank line separating first header + rest

Done.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.cc#n...
webkit/media/mime_util.cc:21: 
On 2012/04/13 02:43:59, scherkus wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h
File webkit/media/mime_util.h (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h#ne...
webkit/media/mime_util.h:5: #ifndef WEBKIT_MEDIA_SUPPORTED_KEY_SYSTEMS_IMPL_H_
On 2012/04/13 02:43:59, scherkus wrote:
> header guard is incorrect

Done.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h#ne...
webkit/media/mime_util.h:19: bool isKeySystemSupported(const WebKit::WebString&
keySystem);
On 2012/04/13 02:43:59, scherkus wrote:
> docs on these methods and unix_hacker args

Done.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/mime_util.h#ne...
webkit/media/mime_util.h:30: #endif  //
WEBKIT_MEDIA_SUPPORTED_KEY_SYSTEMS_IMPL_H_
On 2012/04/13 02:43:59, scherkus wrote:
> don't forget this one!

Done.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:682:
&WebMediaPlayerImpl::generateKeyRequestTask,
On 2012/04/13 02:43:59, scherkus wrote:
> this (and rest of args) should be indented 4 spaces from "M" (i.e., 6 spaces
> from start of line)

Done.
I misinterpreted your preferred style comment.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:977: void
WebMediaPlayerImpl::generateKeyRequestTask(
On 2012/04/13 02:43:59, scherkus wrote:
> per email discussion just nuke these tasks for now

Done.

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:983: // create their own IDs and since CDMs
supporting multiple renderer processes
On 2012/04/13 02:43:59, scherkus wrote:
> I do have to say this is one area that makes me think may require spec
> changes/clarifications
> 
> what are the restrictions on session ids, if any?

Filing a bug on this is on my task list (also affects the WebKit
implementation). I'll file it after I get this uploaded.

[ddorwin, 2012-04-13 22:29:03]

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10020053/diff/7015/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:983: // create their own IDs and since CDMs
supporting multiple renderer processes
On 2012/04/13 21:48:49, ddorwin wrote:
> On 2012/04/13 02:43:59, scherkus wrote:
> > I do have to say this is one area that makes me think may require spec
> > changes/clarifications
> > 
> > what are the restrictions on session ids, if any?
> 
> Filing a bug on this is on my task list (also affects the WebKit
> implementation). I'll file it after I get this uploaded.

Filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16739

[scherkus (not reviewing), 2012-04-16 19:18:14]

LGTM w/ nits

http://codereview.chromium.org/10020053/diff/12001/webkit/media/mime_util.h
File webkit/media/mime_util.h (right):

http://codereview.chromium.org/10020053/diff/12001/webkit/media/mime_util.h#n...
webkit/media/mime_util.h:19: // Returns whether key_sytem is supported at all.
nit: we surround args references in comments with |foo|

http://codereview.chromium.org/10020053/diff/12001/webkit/media/mime_util.h#n...
webkit/media/mime_util.h:21: // key_system supports a specific type of media.
here too

[ddorwin, 2012-04-16 20:02:35]

atwilson, please do an OWNERS review of content/worker.

darin, please do an OWNERS review of webkit/tools and webkit/glue.

Thanks!

http://codereview.chromium.org/10020053/diff/12001/webkit/media/mime_util.h
File webkit/media/mime_util.h (right):

http://codereview.chromium.org/10020053/diff/12001/webkit/media/mime_util.h#n...
webkit/media/mime_util.h:19: // Returns whether key_sytem is supported at all.
On 2012/04/16 19:18:15, scherkus wrote:
> nit: we surround args references in comments with |foo|

Done.

http://codereview.chromium.org/10020053/diff/12001/webkit/media/mime_util.h#n...
webkit/media/mime_util.h:21: // key_system supports a specific type of media.
On 2012/04/16 19:18:15, scherkus wrote:
> here too

Done.

[Andrew T Wilson (Slow), 2012-04-17 00:36:16]

worker stuff LGTM

[darin (slow to review), 2012-04-17 22:41:54]

http://codereview.chromium.org/10020053/diff/20020/webkit/media/mime_util.h
File webkit/media/mime_util.h (right):

http://codereview.chromium.org/10020053/diff/20020/webkit/media/mime_util.h#n...
webkit/media/mime_util.h:17: namespace mime_util {
I think it would be better to make a more specific class for these methods.  It
seems like something based on the term "key system" would be helpful.  I
wouldn't reuse mime_util here.  In general, *util* is overused in Chromium, and
it ends up becoming an unstructured dumping ground.  Let's avoid *util* files!

http://codereview.chromium.org/10020053/diff/20020/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10020053/diff/20020/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:715: const WebKit::WebString& key_system,
nit: consider adding using directives for WebKit API types.  at least it seems
like a 'using WebKit::WebString' would be helpful.

[ddorwin, 2012-04-17 23:25:37]

Thanks.

Renamed mime_util to key_systems. The original (class) name was
SupportedKeySystems, but that seems long (and restrictive) for a namespace.

http://codereview.chromium.org/10020053/diff/20020/webkit/media/mime_util.h
File webkit/media/mime_util.h (right):

http://codereview.chromium.org/10020053/diff/20020/webkit/media/mime_util.h#n...
webkit/media/mime_util.h:17: namespace mime_util {
On 2012/04/17 22:41:54, darin wrote:
> I think it would be better to make a more specific class for these methods. 
It
> seems like something based on the term "key system" would be helpful.  I
> wouldn't reuse mime_util here.  In general, *util* is overused in Chromium,
and
> it ends up becoming an unstructured dumping ground.  Let's avoid *util* files!

Done.

http://codereview.chromium.org/10020053/diff/20020/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10020053/diff/20020/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:715: const WebKit::WebString& key_system,
On 2012/04/17 22:41:54, darin wrote:
> nit: consider adding using directives for WebKit API types.  at least it seems
> like a 'using WebKit::WebString' would be helpful.

Done.

[scherkus (not reviewing), 2012-04-17 23:30:53]

still LGTM but with more nits!

http://codereview.chromium.org/10020053/diff/25005/webkit/media/key_systems.h
File webkit/media/key_systems.h (right):

http://codereview.chromium.org/10020053/diff/25005/webkit/media/key_systems.h...
webkit/media/key_systems.h:22: bool isKeySystemSupported(const
WebKit::WebString& key_system);
is -> Is


OCD nit: this functions aren't consistent in their noun-verbage

IsNounVerb()
IsVerbNounWithNoun()

in other words... why not have:
IsSupportedKeySystem()
IsSupportedKeySystemWithMimeType()

[ddorwin, 2012-04-18 05:17:01]

Thanks.

http://codereview.chromium.org/10020053/diff/25005/webkit/media/key_systems.h
File webkit/media/key_systems.h (right):

http://codereview.chromium.org/10020053/diff/25005/webkit/media/key_systems.h...
webkit/media/key_systems.h:22: bool isKeySystemSupported(const
WebKit::WebString& key_system);
On 2012/04/17 23:30:53, scherkus wrote:
> is -> Is
> 
> 
> OCD nit: this functions aren't consistent in their noun-verbage
> 
> IsNounVerb()
> IsVerbNounWithNoun()
> 
> in other words... why not have:
> IsSupportedKeySystem()
> IsSupportedKeySystemWithMimeType()

Done.

The parameter types are still inconsistent, but this avoids conversion at all
the call sites. I guess you could look at it as using WebKit types except for
the next function where all the types are STL where it is used.

[darin (slow to review), 2012-04-18 23:57:40]

OK, LGTM

Note, I'm really not sure about any of this.  I think it is OK to land this
(with nits fixed) and then iterate.  I'm missing a lot of background on this
feature.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.cc
File webkit/media/key_systems.cc (right):

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.c...
webkit/media/key_systems.cc:15: const char kClearKeyKeySystem[] =
"webkit-org.w3.clearkey";
nit: should we be concerned about claiming "webkit.org" namespaced entities in
the chromium repository?  i'm not sure what this really means, who decides what
this string means, or how it is specified by content authors.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.c...
webkit/media/key_systems.cc:27: // build a list using ParseCodecString() like
MimeUtil. Therfore, the
does "MimeUtil" in this comment refer to net/base/mime_util.cc?  may be good to
clarify.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.c...
webkit/media/key_systems.cc:30: { "video/webm", "vorbis", kClearKeyKeySystem },
do you intend for there to be other key_system values in the future?  this
currently seems like gratuitous infrastructure when the only valid key_system is
kClearKeyKeySystem.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.h
File webkit/media/key_systems.h (right):

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.h...
webkit/media/key_systems.h:17: namespace key_systems {
nit: why do you even need this namespace?  you already have the webkit_media
namespace.  that should be sufficient scoping right?

[ddorwin, 2012-04-19 00:52:53]

Thanks. Happy to fill in some background at your convenience.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.cc
File webkit/media/key_systems.cc (right):

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.c...
webkit/media/key_systems.cc:15: const char kClearKeyKeySystem[] =
"webkit-org.w3.clearkey";
On 2012/04/18 23:57:40, darin wrote:
> nit: should we be concerned about claiming "webkit.org" namespaced entities in
> the chromium repository?  i'm not sure what this really means, who decides
what
> this string means, or how it is specified by content authors.

This is really a "webkit-" prefix to "org.w3.clearkey". The latter is part of
the EME proposal but I prefixed it (similar to the methods) since this yet an
official draft.

The WebKit layout tests use this string. However, the implementation of the key
systems is (likely to be) in the ports.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.c...
webkit/media/key_systems.cc:27: // build a list using ParseCodecString() like
MimeUtil. Therfore, the
On 2012/04/18 23:57:40, darin wrote:
> does "MimeUtil" in this comment refer to net/base/mime_util.cc?  may be good
to
> clarify.

Done.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.c...
webkit/media/key_systems.cc:30: { "video/webm", "vorbis", kClearKeyKeySystem },
On 2012/04/18 23:57:40, darin wrote:
> do you intend for there to be other key_system values in the future?  this
> currently seems like gratuitous infrastructure when the only valid key_system
is
> kClearKeyKeySystem.

Yes. We will add to this table when we add new key systems/CDMs.

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.h
File webkit/media/key_systems.h (right):

http://codereview.chromium.org/10020053/diff/30007/webkit/media/key_systems.h...
webkit/media/key_systems.h:17: namespace key_systems {
On 2012/04/18 23:57:40, darin wrote:
> nit: why do you even need this namespace?  you already have the webkit_media
> namespace.  that should be sufficient scoping right?

Done.

[scherkus (not reviewing), 2012-04-19 00:55:32]

LGTM+++++

[commit-bot: I haz the power, 2012-04-19 00:56:48]

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

[commit-bot: I haz the power, 2012-04-19 04:23:50]

Try job failure for 10020053-33001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-19 06:10:39]

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

[commit-bot: I haz the power, 2012-04-19 09:45:58]

Change committed as 132973