Changes to the EME Pepper API

Create a Pepper-based CDM when a MediaKeys object is created.

The new version of EME needs to be able to instantiate the Pepper-based CDM when a MediaKeys object is created. Moving creation from first GenerateKeyRequest() to Initialize(), and passing in extra (extendable) data.

Also removing unused NeedKey().

BUG=250049
TEST=browser_tests --gtest_filter=EncryptedMediaIsTypeSupported*.* all pass

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

[jrummell, 2013-09-17 21:33:55]

PTAL. I will need other OWNERS, but I'll add them later once this looks like
it's on the right track.

[DaleCurtis, 2013-09-17 21:50:34]

Syntactically, lgtm.  Having a one time Initialize() like this will be much
clearer to delay than GenerateKeyRequest in my CL:

https://codereview.chromium.org/23546014/

I defer to ddorwin, xhwang about whether this works in terms of EME.

[ddorwin, 2013-09-17 23:15:42]

LGTM with description update and other comments. You'll need ppapi OWNERS
approval.

Comments on the description:
"Changes to the EME Pepper API" and "Changing the EME Pepper API:" are not very
descriptive, especially as the one-line summary.

The TEST= line seems overly broad.

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.cc
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:641: PP_DCHECK(key_system_.empty() || key_system_
== key_system);
Add && cdm_ to the second clause?

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:645: SendUnknownKeyError(key_system,
std::string());
TODO: Confirm we should fire a keyerror here.

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:655: 
remove extra line

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:667: 
remove line

https://codereview.chromium.org/24192004/diff/1/ppapi/api/private/ppp_content...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/24192004/diff/1/ppapi/api/private/ppp_content...
ppapi/api/private/ppp_content_decryptor_private.idl:31: [in] PP_Var key_system);
What about the other values we discussed? Should we add those in another CL? Or
will those just be determined just get them in CdmWrapper's Initialize?

[xhwang, 2013-09-18 00:07:17]

Looking good. Thanks! Just a few nits.

https://codereview.chromium.org/24192004/diff/1/ppapi/cpp/private/content_dec...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/24192004/diff/1/ppapi/cpp/private/content_dec...
ppapi/cpp/private/content_decryptor_private.cc:27: PP_Var key_system_arg) {
nit: one line

https://codereview.chromium.org/24192004/diff/1/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/24192004/diff/1/ppapi/proxy/ppapi_messages.h#...
ppapi/proxy/ppapi_messages.h:1038: ppapi::proxy::SerializedVar /* init_data,
ArrayBuffer */)
Shall we remove this?  I ran grep and got the following. Just make sure we
remove all NeedKeys.

./c/private/ppb_content_decryptor_private.h:69:  void (*NeedKey)(PP_Instance
instance,
./cpp/private/content_decryptor_private.cc:217:void
ContentDecryptor_Private::NeedKey(const std::string& key_system,
./cpp/private/content_decryptor_private.cc:225:   
get_interface<PPB_ContentDecryptor_Private>()->NeedKey(
./cpp/private/content_decryptor_private.h:61:  void NeedKey(const std::string&
key_system,
./api/private/ppb_content_decryptor_private.idl:49:  void NeedKey(
./thunk/ppb_instance_api.h:145:  virtual void NeedKey(PP_Instance instance,
./thunk/ppb_content_decryptor_private_thunk.cc:21:void NeedKey(PP_Instance
instance,
./thunk/ppb_content_decryptor_private_thunk.cc:25:  VLOG(4) <<
"PPB_ContentDecryptor_Private::NeedKey()";
./thunk/ppb_content_decryptor_private_thunk.cc:29: 
enter.functions()->NeedKey(instance, key_system, session_id, init_data);
./thunk/ppb_content_decryptor_private_thunk.cc:149:  &NeedKey,
./proxy/ppapi_messages.h:1036:IPC_MESSAGE_ROUTED4(PpapiHostMsg_PPBInstance_NeedKey,
./proxy/ppb_instance_proxy.cc:178:   
IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBInstance_NeedKey,
./proxy/ppb_instance_proxy.cc:179:                        OnHostMsgNeedKey)
./proxy/ppb_instance_proxy.cc:542:void PPB_Instance_Proxy::NeedKey(PP_Instance
instance,
./proxy/ppb_instance_proxy.cc:547:      new PpapiHostMsg_PPBInstance_NeedKey(
./proxy/ppb_instance_proxy.cc:1060:void
PPB_Instance_Proxy::OnHostMsgNeedKey(PP_Instance instance,
./proxy/ppb_instance_proxy.cc:1068:    enter.functions()->NeedKey(instance,
./proxy/ppb_instance_proxy.h:120:  virtual void NeedKey(PP_Instance instance,
./proxy/ppb_instance_proxy.h:226:  virtual void OnHostMsgNeedKey(PP_Instance
instance,
./native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:2306:static void
Pnacl_M24_PPB_ContentDecryptor_Private_NeedKey(PP_Instance instance, struct
PP_Var* key_system, struct PP_Var* session_id, struct PP_Var* init_data) {
./native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:2308: 
iface->NeedKey(instance, *key_system, *session_id, *init_data);
./native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:4636:    .NeedKey =
(void (*)(PP_Instance instance, struct PP_Var key_system, struct PP_Var
session_id, struct PP_Var
init_data))&Pnacl_M24_PPB_ContentDecryptor_Private_NeedKey,

[jrummell, 2013-09-18 21:01:15]

Updated. PTAL.

+dmichael
+tsepez for ppapi/proxy/ppapi_messages.h

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.cc
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:641: PP_DCHECK(key_system_.empty() || key_system_
== key_system);
On 2013/09/17 23:15:42, ddorwin wrote:
> Add && cdm_ to the second clause?

Done.

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:645: SendUnknownKeyError(key_system,
std::string());
On 2013/09/17 23:15:42, ddorwin wrote:
> TODO: Confirm we should fire a keyerror here.

Done.

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:655: 
On 2013/09/17 23:15:42, ddorwin wrote:
> remove extra line

Done.

https://codereview.chromium.org/24192004/diff/1/media/cdm/ppapi/cdm_wrapper.c...
media/cdm/ppapi/cdm_wrapper.cc:667: 
On 2013/09/17 23:15:42, ddorwin wrote:
> remove line

Done.

https://codereview.chromium.org/24192004/diff/1/ppapi/api/private/ppp_content...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/24192004/diff/1/ppapi/api/private/ppp_content...
ppapi/api/private/ppp_content_decryptor_private.idl:31: [in] PP_Var key_system);
On 2013/09/17 23:15:42, ddorwin wrote:
> What about the other values we discussed? Should we add those in another CL?
Or
> will those just be determined just get them in CdmWrapper's Initialize?

Added PP_KeySystemFlags.

https://codereview.chromium.org/24192004/diff/1/ppapi/cpp/private/content_dec...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/24192004/diff/1/ppapi/cpp/private/content_dec...
ppapi/cpp/private/content_decryptor_private.cc:27: PP_Var key_system_arg) {
On 2013/09/18 00:07:18, xhwang wrote:
> nit: one line

Added additional parameter, so it doesn't fit anymore.

https://codereview.chromium.org/24192004/diff/1/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/24192004/diff/1/ppapi/proxy/ppapi_messages.h#...
ppapi/proxy/ppapi_messages.h:1038: ppapi::proxy::SerializedVar /* init_data,
ArrayBuffer */)
On 2013/09/18 00:07:18, xhwang wrote:
> Shall we remove this?  I ran grep and got the following. Just make sure we
> remove all NeedKeys.
> 
> ./c/private/ppb_content_decryptor_private.h:69:  void (*NeedKey)(PP_Instance
> instance,
> ./cpp/private/content_decryptor_private.cc:217:void
> ContentDecryptor_Private::NeedKey(const std::string& key_system,
> ./cpp/private/content_decryptor_private.cc:225:   
> get_interface<PPB_ContentDecryptor_Private>()->NeedKey(
> ./cpp/private/content_decryptor_private.h:61:  void NeedKey(const std::string&
> key_system,
> ./api/private/ppb_content_decryptor_private.idl:49:  void NeedKey(
> ./thunk/ppb_instance_api.h:145:  virtual void NeedKey(PP_Instance instance,
> ./thunk/ppb_content_decryptor_private_thunk.cc:21:void NeedKey(PP_Instance
> instance,
> ./thunk/ppb_content_decryptor_private_thunk.cc:25:  VLOG(4) <<
> "PPB_ContentDecryptor_Private::NeedKey()";
> ./thunk/ppb_content_decryptor_private_thunk.cc:29: 
> enter.functions()->NeedKey(instance, key_system, session_id, init_data);
> ./thunk/ppb_content_decryptor_private_thunk.cc:149:  &NeedKey,
>
./proxy/ppapi_messages.h:1036:IPC_MESSAGE_ROUTED4(PpapiHostMsg_PPBInstance_NeedKey,
> ./proxy/ppb_instance_proxy.cc:178:   
> IPC_MESSAGE_HANDLER(PpapiHostMsg_PPBInstance_NeedKey,
> ./proxy/ppb_instance_proxy.cc:179:                        OnHostMsgNeedKey)
> ./proxy/ppb_instance_proxy.cc:542:void PPB_Instance_Proxy::NeedKey(PP_Instance
> instance,
> ./proxy/ppb_instance_proxy.cc:547:      new PpapiHostMsg_PPBInstance_NeedKey(
> ./proxy/ppb_instance_proxy.cc:1060:void
> PPB_Instance_Proxy::OnHostMsgNeedKey(PP_Instance instance,
> ./proxy/ppb_instance_proxy.cc:1068:    enter.functions()->NeedKey(instance,
> ./proxy/ppb_instance_proxy.h:120:  virtual void NeedKey(PP_Instance instance,
> ./proxy/ppb_instance_proxy.h:226:  virtual void OnHostMsgNeedKey(PP_Instance
> instance,
> ./native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:2306:static void
> Pnacl_M24_PPB_ContentDecryptor_Private_NeedKey(PP_Instance instance, struct
> PP_Var* key_system, struct PP_Var* session_id, struct PP_Var* init_data) {
> ./native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:2308: 
> iface->NeedKey(instance, *key_system, *session_id, *init_data);
> ./native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:4636:    .NeedKey =
> (void (*)(PP_Instance instance, struct PP_Var key_system, struct PP_Var
> session_id, struct PP_Var
> init_data))&Pnacl_M24_PPB_ContentDecryptor_Private_NeedKey,

Removed it in the .idl file, but looks like there are a few references
remaining. Done.

[Tom Sepez, 2013-09-18 21:03:00]

Messages LGTM.

[ddorwin, 2013-09-18 21:26:53]

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:251: 
nit: I would put this after the assignment, which is separate from Initialize()

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:256: &key_system_flags);
Why address of? It's input only.

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.h:43: const PP_KeySystemFlags
key_system_flags);
const ref (if we can)

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
ppapi/api/private/pp_content_decryptor.idl:366: * <code>PP_KeySystemFlags</code>
contains additional information for the key
nit: maybe "for CDM initialization". Same with the struct name, though I don't
have a great name.

PP_CdmConfigurationFlags?

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
ppapi/api/private/ppp_content_decryptor_private.idl:30: * contains additional
information on <code>key_system</code>.
contains configuration information.

or something like that. It doesn't necessarily describe the key system.

[DaleCurtis, 2013-09-18 21:29:29]

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:248: const
PP_KeySystemFlags key_system_flags) {
const&

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.h:43: const PP_KeySystemFlags
key_system_flags);
const&

https://codereview.chromium.org/24192004/diff/10001/media/cdm/ppapi/cdm_wrapp...
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/24192004/diff/10001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.cc:657: pp::VarArrayBuffer init_data) {
dmichael@ pointed out on my cl that every pp::VarArrayBuffer should be passed by
const& if possible. Does everything compile and work if you change all of these
to const& ? Presumably this can be extended to the pp::Buffer_Dev fields too.

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
ppapi/api/private/pp_content_decryptor.idl:375: PP_Bool can_challenge_platform;
FYI, dmichael@ I'm going to rip out the CanChallengePlatform() PPAPI in favor of
this. There's just no way for us to use this value asynchronously in a clean
manner.

[ddorwin, 2013-09-18 22:22:23]

LG other than the pointer issue and a few other minor things in my previous
message.

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:452: const_cast<const
PP_KeySystemFlags*>(&key_system_flags));
There's no way to pass structs by value or const ref?

[dmichael (off chromium), 2013-09-18 22:52:32]

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
ppapi/api/private/pp_content_decryptor.idl:375: PP_Bool can_challenge_platform;
On 2013/09/18 21:29:30, DaleCurtis wrote:
> FYI, dmichael@ I'm going to rip out the CanChallengePlatform() PPAPI in favor
of
> this. There's just no way for us to use this value asynchronously in a clean
> manner.
FWIW, if you're invoking from a background thread, you can use
PP_BlockUntilComplete to make it a synchronous call. This is probably okay too.

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
ppapi/api/private/ppp_content_decryptor_private.idl:35: [in] PP_KeySystemFlags
key_system_flags);
Do you expect there to be a lot of these? I don't really see much benefit to
using a struct for this.

If you think they'll all be bools, you could use a bitfield, which would then at
least make it ABI compatible if you add more later.

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:447: if
(!DeserializeBlockInfo(serialized_key_systems_flags, &key_system_flags))
hmm, I probably never should have lgtmed
DeserializeBlockInfo/SerializeBlockInfo. It makes it easy to pass anything as a
string, which is almost always the wrong thing to do.

In this case, you could really just pass a bool. Or use the appropriate macros
to make the struct serializable in the top of ppapi/proxy/ppapi_messages.h

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:452: const_cast<const
PP_KeySystemFlags*>(&key_system_flags));
On 2013/09/18 22:22:24, ddorwin wrote:
> There's no way to pass structs by value or const ref?
No const-ref in C. You could technically use by-value, but we default to passing
structs by const-pointer in Pepper, (a) because some are large and (b) for
consistency.

[jrummell, 2013-09-19 00:37:28]

Updated so Initialize() passes a bool rather than a struct. PTAL.

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:248: const
PP_KeySystemFlags key_system_flags) {
On 2013/09/18 21:29:30, DaleCurtis wrote:
> const&

Done.

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:251: 
On 2013/09/18 21:26:53, ddorwin wrote:
> nit: I would put this after the assignment, which is separate from
Initialize()

Done.

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.cc:256: &key_system_flags);
On 2013/09/18 21:26:53, ddorwin wrote:
> Why address of? It's input only.

The .idl generated interface is const struct PP_CdmConfigurationFlags* (in
ppapi/c/private/ppp_content_decryptor_private.h).

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/24192004/diff/10001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.h:43: const PP_KeySystemFlags
key_system_flags);
On 2013/09/18 21:29:30, DaleCurtis wrote:
> const&

Done.

https://codereview.chromium.org/24192004/diff/10001/media/cdm/ppapi/cdm_wrapp...
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/24192004/diff/10001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.cc:657: pp::VarArrayBuffer init_data) {
On 2013/09/18 21:29:30, DaleCurtis wrote:
> dmichael@ pointed out on my cl that every pp::VarArrayBuffer should be passed
by
> const& if possible. Does everything compile and work if you change all of
these
> to const& ? Presumably this can be extended to the pp::Buffer_Dev fields too.

It fails to compile as Map() is not const.

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/pp_cont...
ppapi/api/private/pp_content_decryptor.idl:366: * <code>PP_KeySystemFlags</code>
contains additional information for the key
On 2013/09/18 21:26:53, ddorwin wrote:
> nit: maybe "for CDM initialization". Same with the struct name, though I don't
> have a great name.
> 
> PP_CdmConfigurationFlags?

Done.

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
ppapi/api/private/ppp_content_decryptor_private.idl:30: * contains additional
information on <code>key_system</code>.
On 2013/09/18 21:26:53, ddorwin wrote:
> contains configuration information.
> 
> or something like that. It doesn't necessarily describe the key system.

Done.

https://codereview.chromium.org/24192004/diff/10001/ppapi/api/private/ppp_con...
ppapi/api/private/ppp_content_decryptor_private.idl:35: [in] PP_KeySystemFlags
key_system_flags);
On 2013/09/18 22:52:32, dmichael wrote:
> Do you expect there to be a lot of these? I don't really see much benefit to
> using a struct for this.
> 
> If you think they'll all be bools, you could use a bitfield, which would then
at
> least make it ABI compatible if you add more later.

Switched to a bool.

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://codereview.chromium.org/24192004/diff/10001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:447: if
(!DeserializeBlockInfo(serialized_key_systems_flags, &key_system_flags))
On 2013/09/18 22:52:32, dmichael wrote:
> hmm, I probably never should have lgtmed
> DeserializeBlockInfo/SerializeBlockInfo. It makes it easy to pass anything as
a
> string, which is almost always the wrong thing to do.
> 
> In this case, you could really just pass a bool. Or use the appropriate macros
> to make the struct serializable in the top of ppapi/proxy/ppapi_messages.h

Switched to pass a bool, and avoid the serialization issue.

[ddorwin, 2013-09-19 00:58:41]

LGTM % nits

https://codereview.chromium.org/24192004/diff/42001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/24192004/diff/42001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.h:43: const bool
can_challenge_platform);
const not necessary (Google style doesn't care whether you change a parameter's
value within the function).

https://codereview.chromium.org/24192004/diff/42001/media/cdm/ppapi/cdm_wrapp...
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/24192004/diff/42001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.cc:502: const bool can_challenge_platform) OVERRIDE;
no const

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
ppapi/cpp/private/content_decryptor_private.h:35: const bool
can_challenge_platform) = 0;
no const

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.h (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:35: const bool
can_challenge_platform);
same

[DaleCurtis, 2013-09-19 01:12:36]

lgtm % ddorwin's comments.

https://codereview.chromium.org/24192004/diff/42001/content/renderer/media/cr...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/24192004/diff/42001/content/renderer/media/cr...
content/renderer/media/crypto/ppapi_decryptor.cc:41: bool can_challenge_platform
= false;
const

[dmichael (off chromium), 2013-09-19 16:04:19]

lgtm

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
ppapi/cpp/private/content_decryptor_private.cc:40: can_challenge_platform);
PP_ToBool(can_challenge_platform)?

I think the Windows compiler might complain if you don't.

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:126:
can_challenge_platform));
PP_ToBool?

[xhwang, 2013-09-19 16:11:12]

lgtm

(did you grep to make sure we get all NeedKey under ppapi?)

[jrummell, 2013-09-19 18:15:14]

nits fixed. Thanks for the reviews.

https://codereview.chromium.org/24192004/diff/42001/content/renderer/media/cr...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/24192004/diff/42001/content/renderer/media/cr...
content/renderer/media/crypto/ppapi_decryptor.cc:41: bool can_challenge_platform
= false;
On 2013/09/19 01:12:36, DaleCurtis wrote:
> const

Done.

https://codereview.chromium.org/24192004/diff/42001/content/renderer/pepper/c...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/24192004/diff/42001/content/renderer/pepper/c...
content/renderer/pepper/content_decryptor_delegate.h:43: const bool
can_challenge_platform);
On 2013/09/19 00:58:42, ddorwin wrote:
> const not necessary (Google style doesn't care whether you change a
parameter's
> value within the function).

Done.

https://codereview.chromium.org/24192004/diff/42001/media/cdm/ppapi/cdm_wrapp...
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/24192004/diff/42001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.cc:502: const bool can_challenge_platform) OVERRIDE;
On 2013/09/19 00:58:42, ddorwin wrote:
> no const

Done.

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
ppapi/cpp/private/content_decryptor_private.cc:40: can_challenge_platform);
On 2013/09/19 16:04:20, dmichael wrote:
> PP_ToBool(can_challenge_platform)?
> 
> I think the Windows compiler might complain if you don't.

Done.

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/cpp/private/content...
ppapi/cpp/private/content_decryptor_private.h:35: const bool
can_challenge_platform) = 0;
On 2013/09/19 00:58:42, ddorwin wrote:
> no const

Done.

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:126:
can_challenge_platform));
On 2013/09/19 16:04:20, dmichael wrote:
> PP_ToBool?

Done.

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
File ppapi/proxy/ppp_content_decryptor_private_proxy.h (right):

https://codereview.chromium.org/24192004/diff/42001/ppapi/proxy/ppp_content_d...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:35: const bool
can_challenge_platform);
On 2013/09/19 00:58:42, ddorwin wrote:
> same

Done.

[commit-bot: I haz the power, 2013-09-19 18:19:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/24192004/67001

[commit-bot: I haz the power, 2013-09-19 18:47:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
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-19 19:09:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/24192004/91001

[commit-bot: I haz the power, 2013-09-19 21:36:01]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-19 21:41:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/24192004/91001

[commit-bot: I haz the power, 2013-09-20 06:34:55]

Change committed as 224306