Add type argument to pepper content decryptor method GenerateKeyRequest().

Add type argument to pepper content decryptor method GenerateKeyRequest().
    
The type argument is used to carry a MIME type that identifies the type of initialization data.
    
TEST=encrypted ExternalClearKey tests pass


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

[Tom Finegan, 2012-10-26 04:44:25]

PTAL, thanks!

[xhwang, 2012-10-26 06:19:29]

lgtm % nits

http://codereview.chromium.org/11270057/diff/1/ppapi/cpp/private/content_decr...
File ppapi/cpp/private/content_decryptor_private.cc (right):

http://codereview.chromium.org/11270057/diff/1/ppapi/cpp/private/content_decr...
ppapi/cpp/private/content_decryptor_private.cc:40: if (mime_type_var.is_string()
== false)
not sure if it's ppapi's conventon, but why not just
if (!mime_type_var.is_string)
?
also on line 36, 44 and probably more places in this file

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cdm_...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cdm_...
webkit/media/crypto/ppapi/cdm_wrapper.cc:599: mime_type.length(),
Use data() and size() to be consistent with how we treat session_id in other
methods? Also c_str() may trigger an extra copy to append the null terminator.

[ddorwin, 2012-10-26 18:37:05]

lgtm % comments

http://codereview.chromium.org/11270057/diff/1/ppapi/api/private/ppp_content_...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

http://codereview.chromium.org/11270057/diff/1/ppapi/api/private/ppp_content_...
ppapi/api/private/ppp_content_decryptor_private.idl:25: * to use. mime_type
contains the MIME type of init_data. init_data is a data
The spec (like the existing <video> spec) uses just |type|. I think we should be
consistent.

http://codereview.chromium.org/11270057/diff/1/ppapi/cpp/private/content_decr...
File ppapi/cpp/private/content_decryptor_private.cc (right):

http://codereview.chromium.org/11270057/diff/1/ppapi/cpp/private/content_decr...
ppapi/cpp/private/content_decryptor_private.cc:40: if (mime_type_var.is_string()
== false)
Just to verify, "" will pass this, right?

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/clea...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/clea...
webkit/media/crypto/ppapi/clear_key_cdm.cc:192: // TODO(tomfinegan): Use
|mime_type| to determine type of |init_data|.
I think this TODO actually belongs in AesDecryptor. Maybe just move it to 201
and say we need to extend the Decryptor interface.

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cont...
File webkit/media/crypto/ppapi/content_decryption_module.h (right):

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cont...
webkit/media/crypto/ppapi/content_decryption_module.h:206: virtual Status
GenerateKeyRequest(const char* mime_type,
I notice that we don't pass key_system in GKR(). That might be an issue. Really,
we should pass it in CreateCdmInstance(), but that must come through PPAPI as
GKR(). Would you prefer to do that in this CL or another?

http://codereview.chromium.org/11270057/diff/1/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11270057/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1622: const std::string mime_type
= "video/webm";
Let's just go with the empty string. CDMs will have to figure it out (or ignore
in the case of the current Clear Key implementation) for now.

[Tom Finegan, 2012-10-26 22:51:33]

Comments addressed, and rebased on CQ'd CL. PTAL, thanks! (and sorry for the
rebase noise)

Btw, the new stuff to look at is in CDM::GKR and CreateCdmInstance.

http://codereview.chromium.org/11270057/diff/1/ppapi/api/private/ppp_content_...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

http://codereview.chromium.org/11270057/diff/1/ppapi/api/private/ppp_content_...
ppapi/api/private/ppp_content_decryptor_private.idl:25: * to use. mime_type
contains the MIME type of init_data. init_data is a data
On 2012/10/26 18:37:06, ddorwin wrote:
> The spec (like the existing <video> spec) uses just |type|. I think we should
be
> consistent.

Done.

http://codereview.chromium.org/11270057/diff/1/ppapi/cpp/private/content_decr...
File ppapi/cpp/private/content_decryptor_private.cc (right):

http://codereview.chromium.org/11270057/diff/1/ppapi/cpp/private/content_decr...
ppapi/cpp/private/content_decryptor_private.cc:40: if (mime_type_var.is_string()
== false)
On 2012/10/26 18:37:06, ddorwin wrote:
> Just to verify, "" will pass this, right?

xhwang's comment: Done.
ddorwin's comment: Works on my machine... :)

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cdm_...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cdm_...
webkit/media/crypto/ppapi/cdm_wrapper.cc:599: mime_type.length(),
On 2012/10/26 06:19:30, xhwang wrote:
> Use data() and size() to be consistent with how we treat session_id in other
> methods? Also c_str() may trigger an extra copy to append the null terminator.

Done.

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/clea...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/clea...
webkit/media/crypto/ppapi/clear_key_cdm.cc:192: // TODO(tomfinegan): Use
|mime_type| to determine type of |init_data|.
On 2012/10/26 18:37:06, ddorwin wrote:
> I think this TODO actually belongs in AesDecryptor. Maybe just move it to 201
> and say we need to extend the Decryptor interface.

Done.

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cont...
File webkit/media/crypto/ppapi/content_decryption_module.h (right):

http://codereview.chromium.org/11270057/diff/1/webkit/media/crypto/ppapi/cont...
webkit/media/crypto/ppapi/content_decryption_module.h:206: virtual Status
GenerateKeyRequest(const char* mime_type,
On 2012/10/26 18:37:06, ddorwin wrote:
> I notice that we don't pass key_system in GKR(). That might be an issue.
Really,
> we should pass it in CreateCdmInstance(), but that must come through PPAPI as
> GKR(). Would you prefer to do that in this CL or another?

Done.

http://codereview.chromium.org/11270057/diff/1/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11270057/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1622: const std::string mime_type
= "video/webm";
On 2012/10/26 18:37:06, ddorwin wrote:
> Let's just go with the empty string. CDMs will have to figure it out (or
ignore
> in the case of the current Clear Key implementation) for now.

Done.

[ddorwin, 2012-10-26 23:00:11]

Thanks. I need to look at the last 5 files again, but let's address these issues
first.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/content_decryption_module.h (right):

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/content_decryption_module.h:31: const char*
key_system,
I think these should be first.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/content_decryption_module.h:218: virtual Status
GenerateKeyRequest(const char* key_system,
I don't think we need key_system here. cdm_wrapper splits GKR into
CreateCdmInstance() and this call. The key_system goes with the former. That is
also more like the OO version of the API.

[xhwang, 2012-10-26 23:08:09]

Please also update the CL descryption.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:443: const std::string& mime_type,
drop mime_

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:553: type.size(),
nit: it's okay to have *.data() and *.size() on the same line since those two
parameter are closely related.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:120: const std::string
kExternalClearKey = "org.chromium.externalclearkey";
how about defining this as file scoped static const char[] ?

[Tom Finegan, 2012-10-26 23:30:18]

PTAL.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:443: const std::string& mime_type,
On 2012/10/26 23:08:09, xhwang wrote:
> drop mime_

Done.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:553: type.size(),
On 2012/10/26 23:08:09, xhwang wrote:
> nit: it's okay to have *.data() and *.size() on the same line since those two
> parameter are closely related.

Done.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:120: const std::string
kExternalClearKey = "org.chromium.externalclearkey";
On 2012/10/26 23:08:09, xhwang wrote:
> how about defining this as file scoped static const char[] ?

Done.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/content_decryption_module.h (right):

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/content_decryption_module.h:31: const char*
key_system,
On 2012/10/26 23:00:11, ddorwin wrote:
> I think these should be first.

Done.

http://codereview.chromium.org/11270057/diff/9003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/content_decryption_module.h:218: virtual Status
GenerateKeyRequest(const char* key_system,
On 2012/10/26 23:00:11, ddorwin wrote:
> I don't think we need key_system here. cdm_wrapper splits GKR into
> CreateCdmInstance() and this call. The key_system goes with the former. That
is
> also more like the OO version of the API.

Done.

[xhwang, 2012-10-26 23:39:15]

lgtm % tiny tiny nits

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:121: const std::string
kExternalClearKeyString = kExternalClearKey;
does this work at all? 
DCHECK_EQ(key_system, kExternalClearKey)

If not then nevermind.

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:211:
decryptor_.GenerateKeyRequest(key_system, init_data, init_data_size);
nit: pass in kExternalClearKey directly?

[ddorwin, 2012-10-26 23:47:46]

lgtm % comments

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:121: const std::string
kExternalClearKeyString = kExternalClearKey;
On 2012/10/26 23:39:15, xhwang wrote:
> does this work at all? 
> DCHECK_EQ(key_system, kExternalClearKey)
> 
> If not then nevermind.

Whatever the case, we shouldn't create a std::string.

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:122: DCHECK_EQ(key_system,
kExternalClearKeyString);
As discussed offline.

[Tom Finegan, 2012-10-26 23:52:24]

dmichael,

Please do an owners review of this CL. Thanks!

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:121: const std::string
kExternalClearKeyString = kExternalClearKey;
On 2012/10/26 23:39:15, xhwang wrote:
> does this work at all? 
> DCHECK_EQ(key_system, kExternalClearKey)
> 
> If not then nevermind.

Didn't even try... compiles/works fine, done.

http://codereview.chromium.org/11270057/diff/6014/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:211:
decryptor_.GenerateKeyRequest(key_system, init_data, init_data_size);
On 2012/10/26 23:39:15, xhwang wrote:
> nit: pass in kExternalClearKey directly?

Done.

[dmichael (off chromium), 2012-10-29 17:13:51]

lgtm for ppapi stuff.

And obligatory nags:
- Do you have any tests?
- Don't forget to refactor the PPP interface as we've discussed previously.

[Tom Finegan, 2012-10-29 17:23:37]

On 2012/10/29 17:13:51, dmichael wrote:
> lgtm for ppapi stuff.
> 
> And obligatory nags:
> - Do you have any tests?

The interface is tested using the ExternalClearKey* tests in
content_browsertests. I was going to look into ppapi api tests after the
interface refactor.

> - Don't forget to refactor the PPP interface as we've discussed previously.

This will should be done before M25.

[commit-bot: I haz the power, 2012-10-29 18:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11270057/24003

[commit-bot: I haz the power, 2012-10-29 22:50:26]

Change committed as 164757