Add PpapiDecryptor which wraps a CDM plugin.

Add PpapiDecryptor which wraps a CDM plugin.

BUG=138139
TEST=none


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

[xhwang, 2012-07-17 00:57:23]

Hello ddorwin and tomf,

This is a preliminary CL that adds the PpapiDecryptor. It's not finished and
hence not ready for review. Please take a look to see if I am on the right
track. I still need to figure out some issues (see comments). Any suggestion on
those would also be appreciated.

xhwang

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
webkit/media/crypto/ppapi_decryptor.cc:33: //   
!cdm_plugin_->GenerateKeyRequest(key_system,
This call (and call to AddKey, CancelKeyRequest) are made from the main thread.
Need to double check here.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
webkit/media/crypto/ppapi_decryptor.cc:65: // TODO(xhwang): Need to figure out
thread safety about PPP calls.
I'll need to find a way to post this function to the main thread.

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

http://codereview.chromium.org/10704241/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:135: base::Unretained(this))) {
This "Unretained" is ugly but should work. The callback passed in here will only
be called by the PpapiDecrypor which is owned by the ProxyDecryptor, which is
then owned by WMPI. If WMPI is destroyed, PpapiDecryptor should also be
destroyed and will not call this callback.

Even so, I'll think about better ways to do this. Please let me know about your
ideas.

http://codereview.chromium.org/10704241/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:1050:
WebMediaPlayerImpl::CreatePluginInstance(const std::string& plugin_type) {
We can replace this function with something like 
base::Bind(WebMediaPlayerClient::createHelperPlugin, GetClient(), frame_). The
only problem is that this requires we change the order of parameters of
createHelperPlugin.

[ddorwin, 2012-07-17 21:31:28]

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
File webkit/media/crypto/key_systems.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:17: const char kExternalClearKeyKeySystem[] =
"webkit-org.w3.external-clearkey";
I haven't thought this through entirely, but it seems maybe it should be
org.chromium... since this will never be an org.w3 thing.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:25: struct KeySystemPluginNamePair {
MimeType here and below.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:43: { "audio/webm", "", kClearKeyKeySystem }
Need to update these tables too.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:48: { kExternalClearKeyKeySystem,
"application/x-ppapi-example" }
FYI, the same TODO as in the above table would apply here as well.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:71: if (key_system == kClearKeyKeySystem)
Need to add new key system(s). Maybe we should base this on a table too.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:104: const CreatePluginCB& create_plugin_cb)
{
It's unexpected that this cb isn't always called. Can we make the callback more
generic and call it for all cases?

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:107: else if (key_system ==
kExternalClearKeyKeySystem)
I think we should GetPlugingMimeType() and if not NULL then create
PpapiDecryptor with that value.
We don't need to duplicate string comparisons.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_systems.h
File webkit/media/crypto/key_systems.h (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.h:12: #include
"webkit/plugins/ppapi/ppapi_plugin_instance.h"
We should really try to avoid this include in this header. It leaks to other
code that shouldn't care about ppapi.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.h:36: const char* GetPluginName(const
std::string& key_system);
I think it is really PluginMimeType.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.h:40: typedef
base::Callback<scoped_refptr<webkit::ppapi::PluginInstance>
Why is there a PI exposed? Seems wrong for this file to know about PIs.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
webkit/media/crypto/ppapi_decryptor.cc:30: cdm_plugin_ =
create_plugin_cb_.Run(plugin_name);
I think we should try to do this at creation time since the object doesn't make
sense without a plugin. Also, the fact that GKR creates the object is just a EME
API detail.

One option is a factory method to create this object and initialize the plugin.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
webkit/media/crypto/ppapi_decryptor.cc:67: //    encrypted,
base::Bind(&PpapiDecryptor::DataReady, this, decrypt_cb))) {
indentation?

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/proxy_decr...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:31: decryptor_ =
CreateDecryptor(key_system, client_, create_plugin_cb_);
As mentioned in ppapi_decryptor.cc, I think we should separate initialization
from GKR.
One additional advantage is we can avoid storing this CB.
If we do it in a Create() or Init() method, we could pass in frame_, etc. to
that method and not need a create_plugin_cb, which is specific to one type of
Decryptor.

From there, we can try to solve this call. Maybe it creates the appropriate
object, and we call Init() with client and frame_and it does the right thing.

We have to pass some extra types around, but at least the pattern is the same
for all types and PluginInstance is not exposed. The only other option seems to
be moving creation to this class, which would separate the key string code or
require exposing "GetDecryptorTypeForKeySystem()".

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

http://codereview.chromium.org/10704241/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:1050:
WebMediaPlayerImpl::CreatePluginInstance(const std::string& plugin_type) {
On 2012/07/17 00:57:23, xhwang wrote:
> We can replace this function with something like 
> base::Bind(WebMediaPlayerClient::createHelperPlugin, GetClient(), frame_). The
> only problem is that this requires we change the order of parameters of
> createHelperPlugin.

If we replaced it, where would the checks and casting below go?

[xhwang, 2012-07-18 19:43:17]

Hello ddorwin,

Revised based on comments and discussion. PTAL!

xhwang

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
File webkit/media/crypto/key_systems.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:17: const char kExternalClearKeyKeySystem[] =
"webkit-org.w3.external-clearkey";
On 2012/07/17 21:31:28, ddorwin wrote:
> I haven't thought this through entirely, but it seems maybe it should be
> org.chromium... since this will never be an org.w3 thing.

Done.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:25: struct KeySystemPluginNamePair {
On 2012/07/17 21:31:28, ddorwin wrote:
> MimeType here and below.

Use PluginType as discussed.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:43: { "audio/webm", "", kClearKeyKeySystem }
On 2012/07/17 21:31:28, ddorwin wrote:
> Need to update these tables too.

Done.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:71: if (key_system == kClearKeyKeySystem)
On 2012/07/17 21:31:28, ddorwin wrote:
> Need to add new key system(s). Maybe we should base this on a table too.

Done. I'll keep "if" here instead of using table as we only have two entries
now.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:104: const CreatePluginCB& create_plugin_cb)
{
On 2012/07/17 21:31:28, ddorwin wrote:
> It's unexpected that this cb isn't always called. Can we make the callback
more
> generic and call it for all cases?

Removed CreatePluginCB and just passing the client and frame. This whole
function is moved into ProxyDecryptor.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.cc:107: else if (key_system ==
kExternalClearKeyKeySystem)
On 2012/07/17 21:31:28, ddorwin wrote:
> I think we should GetPlugingMimeType() and if not NULL then create
> PpapiDecryptor with that value.
> We don't need to duplicate string comparisons.

Moved this check and related logic into PpapiDecryptor::Init().

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_systems.h
File webkit/media/crypto/key_systems.h (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.h:12: #include
"webkit/plugins/ppapi/ppapi_plugin_instance.h"
On 2012/07/17 21:31:28, ddorwin wrote:
> We should really try to avoid this include in this header. It leaks to other
> code that shouldn't care about ppapi.

Done.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.h:36: const char* GetPluginName(const
std::string& key_system);
On 2012/07/17 21:31:28, ddorwin wrote:
> I think it is really PluginMimeType.

Done.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/key_system...
webkit/media/crypto/key_systems.h:40: typedef
base::Callback<scoped_refptr<webkit::ppapi::PluginInstance>
On 2012/07/17 21:31:28, ddorwin wrote:
> Why is there a PI exposed? Seems wrong for this file to know about PIs.

Done.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
webkit/media/crypto/ppapi_decryptor.cc:30: cdm_plugin_ =
create_plugin_cb_.Run(plugin_name);
On 2012/07/17 21:31:28, ddorwin wrote:
> I think we should try to do this at creation time since the object doesn't
make
> sense without a plugin. Also, the fact that GKR creates the object is just a
EME
> API detail.
> 
> One option is a factory method to create this object and initialize the
plugin.

Moved plugin creation into PpapiDecryptor::Init().

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/ppapi_decr...
webkit/media/crypto/ppapi_decryptor.cc:67: //    encrypted,
base::Bind(&PpapiDecryptor::DataReady, this, decrypt_cb))) {
On 2012/07/17 21:31:28, ddorwin wrote:
> indentation?

Done.

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/proxy_decr...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/1/webkit/media/crypto/proxy_decr...
webkit/media/crypto/proxy_decryptor.cc:31: decryptor_ =
CreateDecryptor(key_system, client_, create_plugin_cb_);
On 2012/07/17 21:31:28, ddorwin wrote:
> As mentioned in ppapi_decryptor.cc, I think we should separate initialization
> from GKR.
> One additional advantage is we can avoid storing this CB.
> If we do it in a Create() or Init() method, we could pass in frame_, etc. to
> that method and not need a create_plugin_cb, which is specific to one type of
> Decryptor.
> 
> From there, we can try to solve this call. Maybe it creates the appropriate
> object, and we call Init() with client and frame_and it does the right thing.
> 
> We have to pass some extra types around, but at least the pattern is the same
> for all types and PluginInstance is not exposed. The only other option seems
to
> be moving creation to this class, which would separate the key string code or
> require exposing "GetDecryptorTypeForKeySystem()".

Done.

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

http://codereview.chromium.org/10704241/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:1050:
WebMediaPlayerImpl::CreatePluginInstance(const std::string& plugin_type) {
On 2012/07/17 21:31:28, ddorwin wrote:
> On 2012/07/17 00:57:23, xhwang wrote:
> > We can replace this function with something like 
> > base::Bind(WebMediaPlayerClient::createHelperPlugin, GetClient(), frame_).
The
> > only problem is that this requires we change the order of parameters of
> > createHelperPlugin.
> 
> If we replaced it, where would the checks and casting below go?

All of these are done in PpapiDecryptor::Init() now.

[ddorwin, 2012-07-19 01:12:34]

Thanks.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
File webkit/media/crypto/key_systems.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:14: const char kExternalClearKeyKeySystem[] =
"org.chromium.external-clearkey";
Not sure whether we should use a '-'. I guess it's fine.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:74: if (key_system == kClearKeyKeySystem ||
We _could_ replace this with:
  if (CanUseAesDecryptor || GetPluginType()).
This avoids code duplication and maintenance costs. Thoughts?

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
File webkit/media/crypto/key_systems.h (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.h:35: const char* GetPluginType(const
std::string& key_system);
Who owns the return value? Should we just use std::string and empty string to
indicate none?

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:112: 
Do we need KeyMessage, etc. here too?

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.h (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.h:60:
scoped_refptr<webkit::ppapi::PluginInstance> cdm_plugin_;
Does this need to be a refptr?

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:93: // For now, we only support
AesDecryptor and PpapiDecryptor. So if we cannot
You can remove "For now, ".

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:97: if (ppapi_decryptor->Init(key_system,
web_media_player_client_, web_frame_))
Since we are creating and initializing a PpapiDecryptor here. We could just call
CreateHelperPlugin() here and pass the result to the PpapiDecryptor constructor.
That would separate the latter from WMPI more. You could do all the
initialization such that only a webkit::ppapi::PluginInstance needs to be passed
in. (I would do the work to get there in a private method rather than here.

[xhwang, 2012-07-19 15:54:34]

PTAL!

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
File webkit/media/crypto/key_systems.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:14: const char kExternalClearKeyKeySystem[] =
"org.chromium.external-clearkey";
On 2012/07/19 01:12:34, ddorwin wrote:
> Not sure whether we should use a '-'. I guess it's fine.

Done.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:27: static const MediaFormatAndKeySystem
We have both anonymous namespace and static function in this file. I plan to
remove anonymous namespace and use static exclusively in this file. Please let
me know if it sounds good.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:74: if (key_system == kClearKeyKeySystem ||
On 2012/07/19 01:12:34, ddorwin wrote:
> We _could_ replace this with:
>   if (CanUseAesDecryptor || GetPluginType()).
> This avoids code duplication and maintenance costs. Thoughts?

Done.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
File webkit/media/crypto/key_systems.h (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.h:35: const char* GetPluginType(const
std::string& key_system);
On 2012/07/19 01:12:34, ddorwin wrote:
> Who owns the return value? Should we just use std::string and empty string to
> indicate none?

Ok, search "const char* Get" gives me 216 results and "std::string Get" gives me
788 results. So "std::string" wins :)

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:112: 
On 2012/07/19 01:12:34, ddorwin wrote:
> Do we need KeyMessage, etc. here too?

Are we going to pass those KeyMessage etc into the plugin? Can we just use
base::Bind(&DecryptorClient::KeyMessage, client_)?

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.h (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.h:60:
scoped_refptr<webkit::ppapi::PluginInstance> cdm_plugin_;
On 2012/07/19 01:12:34, ddorwin wrote:
> Does this need to be a refptr?

PluginInstance is ref counted:
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppapi...

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:93: // For now, we only support
AesDecryptor and PpapiDecryptor. So if we cannot
On 2012/07/19 01:12:34, ddorwin wrote:
> You can remove "For now, ".

Done.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:97: if (ppapi_decryptor->Init(key_system,
web_media_player_client_, web_frame_))
On 2012/07/19 01:12:34, ddorwin wrote:
> Since we are creating and initializing a PpapiDecryptor here. We could just
call
> CreateHelperPlugin() here and pass the result to the PpapiDecryptor
constructor.
> That would separate the latter from WMPI more. You could do all the
> initialization such that only a webkit::ppapi::PluginInstance needs to be
passed
> in. (I would do the work to get there in a private method rather than here.

Done.

[ddorwin, 2012-07-19 20:50:09]

lgtm

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
File webkit/media/crypto/key_systems.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:27: static const MediaFormatAndKeySystem
On 2012/07/19 15:54:34, xhwang wrote:
> We have both anonymous namespace and static function in this file. I plan to
> remove anonymous namespace and use static exclusively in this file. Please let
> me know if it sounds good.

SG. I guess both are allowed (last paragraph):
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:112: 
On 2012/07/19 15:54:34, xhwang wrote:
> On 2012/07/19 01:12:34, ddorwin wrote:
> > Do we need KeyMessage, etc. here too?
> 
> Are we going to pass those KeyMessage etc into the plugin? Can we just use
> base::Bind(&DecryptorClient::KeyMessage, client_)?

SG, but we have to wrap that in this class, right? Or are we exposing
DecryptorClient (and the client) to the PluginInstance?

http://codereview.chromium.org/10704241/diff/16001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/16001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.cc:123: return scoped_ptr<Decryptor>(new
PpapiDecryptor(client_, plugin_instance));
media:: appears with Decryptor in most places but not here. Why?

[xhwang, 2012-07-19 21:54:03]

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
File webkit/media/crypto/key_systems.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/key_sys...
webkit/media/crypto/key_systems.cc:27: static const MediaFormatAndKeySystem
On 2012/07/19 20:50:09, ddorwin wrote:
> On 2012/07/19 15:54:34, xhwang wrote:
> > We have both anonymous namespace and static function in this file. I plan to
> > remove anonymous namespace and use static exclusively in this file. Please
let
> > me know if it sounds good.
> 
> SG. I guess both are allowed (last paragraph):
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

Done.

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/6001/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:112: 
On 2012/07/19 20:50:09, ddorwin wrote:
> On 2012/07/19 15:54:34, xhwang wrote:
> > On 2012/07/19 01:12:34, ddorwin wrote:
> > > Do we need KeyMessage, etc. here too?
> > 
> > Are we going to pass those KeyMessage etc into the plugin? Can we just use
> > base::Bind(&DecryptorClient::KeyMessage, client_)?
> 
> SG, but we have to wrap that in this class, right? Or are we exposing
> DecryptorClient (and the client) to the PluginInstance?

I didn't pass it in anywhere because I don't know how to pass in yet. Added a
TODO.

http://codereview.chromium.org/10704241/diff/16001/webkit/media/crypto/proxy_...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/10704241/diff/16001/webkit/media/crypto/proxy_...
webkit/media/crypto/proxy_decryptor.cc:123: return scoped_ptr<Decryptor>(new
PpapiDecryptor(client_, plugin_instance));
On 2012/07/19 20:50:09, ddorwin wrote:
> media:: appears with Decryptor in most places but not here. Why?

Since ProxyDecryptor inherits media::Decryptor, so within this class, it knows
Decryptor is media::Decryptor. But I agree it's inconsistent and misleading.
Done.

[ddorwin, 2012-07-19 22:58:01]

lgtm

Thanks.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10704241/19001

[commit-bot: I haz the power, 2012-07-19 23:27:51]

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

[commit-bot: I haz the power, 2012-07-23 21:53:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/10704241/24001

[commit-bot: I haz the power, 2012-07-23 23:35:27]

Change committed as 147992