Add PPAPI decryptor implementation.

Add PPAPI decryptor implementation.

BUG=138139
TEST=none


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

[Tom Finegan, 2012-08-10 15:33:18]

Here's the new issue for review of PPAPI decryptor implementation.

[Tom Finegan, 2012-08-14 23:04:58]

Reviewers: PTAL-- I'm testing the out of process stuff now, and early feedback
is desperately needed.

Thanks!

[xhwang, 2012-08-15 00:27:57]

Reviewed code that I understand. Didn't review ppapi implementation details and
use/conversion of ppapi variables. Didn't review proxy code either.

http://codereview.chromium.org/10827280/diff/5002/ppapi/cpp/private/content_d...
File ppapi/cpp/private/content_decryptor_private.h (right):

http://codereview.chromium.org/10827280/diff/5002/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:31: virtual bool
Decrypt(pp::Buffer_Dev encrypted_block,
Since the type is pp::Buffer_Dev, make the variable name here and in
DecryptAndDecode "encrypted_buffer"? This is also consistent with what we have
in media code. The names in Deliver* functions seem fine as we need the name to
distinguish among block, frame and samples.

http://codereview.chromium.org/10827280/diff/5002/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:46: const std::string&
default_url);  // String.
Do we need this comment "// String"?

http://codereview.chromium.org/10827280/diff/5002/webkit/media/crypto/content...
File webkit/media/crypto/content_decryptor.cc (right):

http://codereview.chromium.org/10827280/diff/5002/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:98: std::string session_id_;
Do we need to check in these test code? The CDM will handle all of these and the
CDMWrapper should only pass them through.

http://codereview.chromium.org/10827280/diff/5002/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:103: std::string key_id_;
ditto

http://codereview.chromium.org/10827280/diff/5002/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:108: std::string key_system_;
ditto

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:117: using ppapi::ArrayBufferVar;
It looks like these are alphabetically ordered, w/ the only exception of
TrackedCallback. If that's the case, should move this line up.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:299: // buffer resource, and
returns it. Returns a PP_Resource equal to 0 on
Returns an invalid PP_Resource (with a 0 ID) on failure?

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2014: // TODO(tomfinegan):
KeyError here?
The CDMWrapper should check this. Here we probably should just do DCHECK, and
pass things through.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2031: // TODO(tomfinegan):
KeyError here?
ditto.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2037: // TODO(tomfinegan):
KeyError here?
Can this happen if we implemented everything correctly? Or can this be triggered
by a malicious CDM implementation? If not, should this be a DCHECK?

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2043: // TODO(tomfinegan):
KeyError here?
ditto here and below.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2075: // base::Bind usage in the
media stack. Hash this out with xhwang.
You can simplify this to:
TODO(xhwang): Pass the decrypted block back to media stack.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2083: // EME/CDM work.
Add NOTREACHED();

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2090: // EME/CDM work.
Add NOTREACHED();

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:242: // Provides access to
PPP_ContentDecryptor_Private.
dmichael@ mentioned moving these to a delegate class, it's not urgent but we
could have a TODO here.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:242: // Provides access to
PPP_ContentDecryptor_Private.
dmichael@ mentioned moving these to a delegate class. It's not urgent but may
worth a TODO here.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:249: bool Decrypt(const
base::StringPiece& encypted_block,
FYI: this is okay for now, later on this will be replaced by const
scoped_refptr<media::DecoderBuffer>& encrypted_buffer.

http://codereview.chromium.org/10827280/diff/5002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:426: virtual void
NeedKey(PP_Instance instance,
ditto about moving into a delegate class.

[Tom Finegan, 2012-08-15 00:29:25]

I've got out of process working w/dummy calls to GenerateKeyRequest and
KeyMessage. PTAL at the TODO in interface_list.cc-- I don't know how to address
it. 

Do I need to:
1) Create interfaces_ppp_private.h, or
2) Should I just ignore the TODO since we're going to factor out the PPP
interface in the nearish future?

[dmichael (off chromium), 2012-08-15 17:44:37]

Have a few more files to look at after lunch...

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
File ppapi/cpp/private/content_decryptor_private.cc (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.cc:148: const std::string&
init_data) {
Why use string here if the underlying type is ArrayBuffer? You're forcing an
unnecessary memcpy below.

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
File ppapi/cpp/private/content_decryptor_private.h (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:13: #include
"ppapi/c/private/ppp_content_decryptor_private.h"
nit: include order. The /c/ stuff should come first, and cpp/dev should come in
alphabetical order, too.

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:26: virtual bool
GenerateKeyRequest(const std::string& key_system,
Re-thinking it, if you just have to turn around and pass the key_system back to
another call, it may make sense to leave it as pp::Var. It's slightly less
efficient to do it this way, because we'll end up making a new Var and copying
the contents of key_system (same for session_id and other string var arguments
that get passed right back to the browser).

No need to change it for now, just something to keep in mind as you write to
this API. It's kind of a tradeoff, since std::string is certainly friendlier.

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/interface_list.cc
File ppapi/proxy/interface_list.cc (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/interface_list....
ppapi/proxy/interface_list.cc:77: #include
"ppapi/c/private/ppp_content_decryptor_private.h"
nit: this should be with the other private stuff

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/interface_list....
ppapi/proxy/interface_list.cc:251: // supposed to put this?
This is OK for now. This will probably go away (and get rolled in to your PPB
proxy) once you change the PPP to be passed in to the PPB interface. Maybe
change your TODO to something about that; feel free to be vague, because I'm not
entirely sure what it will look like yet. E.g., "Figure out where to put these
once we move the PPP struct to be passed to PPP_ContentDecryptor_Private at
startup."

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
File ppapi/proxy/ppp_content_decryptor_private_proxy.h (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:9: #include
"ppapi/c/pp_instance.h"
nit: include order

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:20:
PPP_ContentDecryptor_Private_Proxy(Dispatcher* dispatcher);
nit: explicit

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:26: virtual bool
OnMessageReceived(const IPC::Message& msg);
This could probably be private.

http://codereview.chromium.org/10827280/diff/6023/ppapi/shared_impl/api_id.h
File ppapi/shared_impl/api_id.h (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/shared_impl/api_id.h#...
ppapi/shared_impl/api_id.h:57: API_ID_PPP_CONTENT_DECRYPTOR_PRIVATE,
Add a TODO to remove this later, maybe?

http://codereview.chromium.org/10827280/diff/6023/ppapi/tests/all_c_includes.h
File ppapi/tests/all_c_includes.h (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/tests/all_c_includes....
ppapi/tests/all_c_includes.h:119: #include
"ppapi/c/private/ppp_content_decryptor_private.h"
nit: order; this should be with other private things.

http://codereview.chromium.org/10827280/diff/6023/ppapi/thunk/ppb_content_dec...
File ppapi/thunk/ppb_content_decryptor_private_thunk.cc (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/thunk/ppb_content_dec...
ppapi/thunk/ppb_content_decryptor_private_thunk.cc:8: #include
"ppapi/thunk/ppb_instance_api.h"
nit: include order

[dmichael (off chromium), 2012-08-15 19:01:00]

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:168:
SerializedVarReceiveInput init_data) {
I think you might need to AddRef your key_system and init_data vars.
SerializedVarReceiveInput decrements the ref-count upon exiting this function,
but the way your C++ wrapper is written, it looks like the CDM expects to
receive a Var with ref-count of 1.

http://codereview.chromium.org/10827280/diff/6023/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10827280/diff/6023/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:299: // buffer resource, and
returns it. Returns a an invalid PP_Resource with an ID
note that the reterned reference count is 1?

http://codereview.chromium.org/10827280/diff/6023/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1376: ScopedPPResource
encrypted_resource(ScopedPPResource::PassRef(),
I don't think you want the PassRef version here, do you? At least, you don't use
it in Decrypt...

[ddorwin, 2012-08-15 22:03:22]

Just skimmed a few files.

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/DEPS
File webkit/media/crypto/DEPS (right):

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/DEPS#ne...
webkit/media/crypto/DEPS:1: include_rules = [
Does it make sense (especially with xhwang's pending files) to have a subdir for
ppapi stuff?

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/content...
File webkit/media/crypto/content_decryptor.cc (right):

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Filename is misleading. It should be cdm_wrapper or something like that.
CDMWrapper isn't exactly descriptive, but nothing obvious comes to mind. It's
really an abstraction of Pepper for the CDM (abstract away the Pepper details).

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:5: #include <cassert>
Why not use DCHECK? Can we not use it in plugins? I think there is a PP_DCHECK
or something.

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:6: #include <cstdio>   // stderr;
debugging via fprintf.
TODO: Remove some of these.

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:50: // A wrapper class responsible for
managing interaction between the browser and
Maybe something like:
... for abstracting away PPAPI interaction and threading for a Content
Decryption Module (CDM).

http://codereview.chromium.org/10827280/diff/6023/webkit/media/crypto/content...
webkit/media/crypto/content_decryptor.cc:192: fprintf(stderr,
Use the PPB_Console_Dev interface instead?

http://codereview.chromium.org/10827280/diff/6023/webkit/media/webkit_media.gypi
File webkit/media/webkit_media.gypi (right):

http://codereview.chromium.org/10827280/diff/6023/webkit/media/webkit_media.g...
webkit/media/webkit_media.gypi:88: 'type': 'none',
FYI, after xhwang's CLs, this should be a static_library.

[Tom Finegan, 2012-08-16 03:10:48]

With the exception of CancelKeyRequest and NeedKey the proxy is working. I'll
look into those tomorrow.

Most review comments are addressed excluding the one's just added by ddorwin.
I'll make another pass through in the morning.

https://chromiumcodereview.appspot.com/10827280/diff/5002/ppapi/cpp/private/c...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/5002/ppapi/cpp/private/c...
ppapi/cpp/private/content_decryptor_private.h:31: virtual bool
Decrypt(pp::Buffer_Dev encrypted_block,
On 2012/08/15 00:27:57, xhwang wrote:
> Since the type is pp::Buffer_Dev, make the variable name here and in
> DecryptAndDecode "encrypted_buffer"? This is also consistent with what we have
> in media code. The names in Deliver* functions seem fine as we need the name
to
> distinguish among block, frame and samples.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/ppapi/cpp/private/c...
ppapi/cpp/private/content_decryptor_private.h:46: const std::string&
default_url);  // String.
On 2012/08/15 00:27:57, xhwang wrote:
> Do we need this comment "// String"?

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/media/crypto...
File webkit/media/crypto/content_decryptor.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:98: std::string session_id_;
On 2012/08/15 00:27:57, xhwang wrote:
> Do we need to check in these test code? The CDM will handle all of these and
the
> CDMWrapper should only pass them through.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:103: std::string key_id_;
On 2012/08/15 00:27:57, xhwang wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:108: std::string key_system_;
On 2012/08/15 00:27:57, xhwang wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:117: using ppapi::ArrayBufferVar;
On 2012/08/15 00:27:57, xhwang wrote:
> It looks like these are alphabetically ordered, w/ the only exception of
> TrackedCallback. If that's the case, should move this line up.

Yeah, wasn't sure what to do-- ArrayBufferVar next to Var made sense, but
sorting based on prevailing order also makes sense. dmichael? :)

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:299: // buffer resource, and
returns it. Returns a PP_Resource equal to 0 on
On 2012/08/15 00:27:57, xhwang wrote:
> Returns an invalid PP_Resource (with a 0 ID) on failure?

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2014: // TODO(tomfinegan):
KeyError here?
On 2012/08/15 00:27:57, xhwang wrote:
> The CDMWrapper should check this. Here we probably should just do DCHECK, and
> pass things through.

No DCHECKs-- per dmichael we're supposed to treat the plugin as untrusted.

Removed the checks, though.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2031: // TODO(tomfinegan):
KeyError here?
On 2012/08/15 00:27:57, xhwang wrote:
> ditto.

Done/removed checks.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2037: // TODO(tomfinegan):
KeyError here?
On 2012/08/15 00:27:57, xhwang wrote:
> Can this happen if we implemented everything correctly? Or can this be
triggered
> by a malicious CDM implementation? If not, should this be a DCHECK? 

Initial question: presumably yes, at the very least out of memory could trigger
it. Otoh we're just going to crash if we're oom. :)

See above re DCHECK.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2043: // TODO(tomfinegan):
KeyError here?
On 2012/08/15 00:27:57, xhwang wrote:
> ditto here and below.

Done/removed checks.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2075: // base::Bind usage in the
media stack. Hash this out with xhwang.
On 2012/08/15 00:27:57, xhwang wrote:
> You can simplify this to:
> TODO(xhwang): Pass the decrypted block back to media stack.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2083: // EME/CDM work.
On 2012/08/15 00:27:57, xhwang wrote:
> Add NOTREACHED();

dmichael said not to use things that can bring down the browser here.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2090: // EME/CDM work.
On 2012/08/15 00:27:57, xhwang wrote:
> Add NOTREACHED();

Ditto.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.h:242: // Provides access to
PPP_ContentDecryptor_Private.
On 2012/08/15 00:27:57, xhwang wrote:
> dmichael@ mentioned moving these to a delegate class. It's not urgent but may
> worth a TODO here.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/5002/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.h:426: virtual void
NeedKey(PP_Instance instance,
On 2012/08/15 00:27:57, xhwang wrote:
> ditto about moving into a delegate class.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
ppapi/cpp/private/content_decryptor_private.cc:148: const std::string&
init_data) {
On 2012/08/15 17:44:37, dmichael wrote:
> Why use string here if the underlying type is ArrayBuffer? You're forcing an
> unnecessary memcpy below.

Done/changed to VarArrayBuffer.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
ppapi/cpp/private/content_decryptor_private.h:13: #include
"ppapi/c/private/ppp_content_decryptor_private.h"
On 2012/08/15 17:44:37, dmichael wrote:
> nit: include order. The /c/ stuff should come first, and cpp/dev should come
in
> alphabetical order, too.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
ppapi/cpp/private/content_decryptor_private.h:26: virtual bool
GenerateKeyRequest(const std::string& key_system,
On 2012/08/15 17:44:37, dmichael wrote:
> Re-thinking it, if you just have to turn around and pass the key_system back
to
> another call, it may make sense to leave it as pp::Var. It's slightly less
> efficient to do it this way, because we'll end up making a new Var and copying
> the contents of key_system (same for session_id and other string var arguments
> that get passed right back to the browser).
> 
> No need to change it for now, just something to keep in mind as you write to
> this API. It's kind of a tradeoff, since std::string is certainly friendlier.

It's going to require extraction to a string within the wrapper anyway--
decryption module wrapped by the plugin will be expecting a string (I think).

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/interfa...
File ppapi/proxy/interface_list.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/interfa...
ppapi/proxy/interface_list.cc:77: #include
"ppapi/c/private/ppp_content_decryptor_private.h"
On 2012/08/15 17:44:37, dmichael wrote:
> nit: this should be with the other private stuff

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/interfa...
ppapi/proxy/interface_list.cc:251: // supposed to put this?
On 2012/08/15 17:44:37, dmichael wrote:
> This is OK for now. This will probably go away (and get rolled in to your PPB
> proxy) once you change the PPP to be passed in to the PPB interface. Maybe
> change your TODO to something about that; feel free to be vague, because I'm
not
> entirely sure what it will look like yet. E.g., "Figure out where to put these
> once we move the PPP struct to be passed to PPP_ContentDecryptor_Private at
> startup."

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/ppp_con...
File ppapi/proxy/ppp_content_decryptor_private_proxy.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/ppp_con...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:9: #include
"ppapi/c/pp_instance.h"
On 2012/08/15 17:44:37, dmichael wrote:
> nit: include order

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/ppp_con...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:20:
PPP_ContentDecryptor_Private_Proxy(Dispatcher* dispatcher);
On 2012/08/15 17:44:37, dmichael wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/ppp_con...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:26: virtual bool
OnMessageReceived(const IPC::Message& msg);
On 2012/08/15 17:44:37, dmichael wrote:
> This could probably be private.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/shared_impl/a...
File ppapi/shared_impl/api_id.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/shared_impl/a...
ppapi/shared_impl/api_id.h:57: API_ID_PPP_CONTENT_DECRYPTOR_PRIVATE,
On 2012/08/15 17:44:37, dmichael wrote:
> Add a TODO to remove this later, maybe?

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/tests/all_c_i...
File ppapi/tests/all_c_includes.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/tests/all_c_i...
ppapi/tests/all_c_includes.h:119: #include
"ppapi/c/private/ppp_content_decryptor_private.h"
On 2012/08/15 17:44:37, dmichael wrote:
> nit: order; this should be with other private things.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/thunk/ppb_con...
File ppapi/thunk/ppb_content_decryptor_private_thunk.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/thunk/ppb_con...
ppapi/thunk/ppb_content_decryptor_private_thunk.cc:8: #include
"ppapi/thunk/ppb_instance_api.h"
On 2012/08/15 17:44:37, dmichael wrote:
> nit: include order

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
File webkit/media/crypto/content_decryptor.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:6: #include <cstdio>   // stderr;
debugging via fprintf.
On 2012/08/15 22:03:22, ddorwin wrote:
> TODO: Remove some of these.

Done.

[Tom Finegan, 2012-08-16 16:15:45]

ddorwin, dmichael, xhwang, PTAL! Almost everything works...

I'm hunting down a problem with CancelKeyRequest. For a reason I don't yet
understand, the PP_Instance is invalid in the CPP wrapper when it gets the
CancelKeyRequest call. Probably something derpish I've done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
File webkit/media/crypto/DEPS (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
webkit/media/crypto/DEPS:1: include_rules = [
On 2012/08/15 22:03:22, ddorwin wrote:
> Does it make sense (especially with xhwang's pending files) to have a subdir
for
> ppapi stuff?

Done, well, assuming you meant for me to move DEPS and content_decryptor.cc to
webkit/media/crypto/ppapi.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
File webkit/media/crypto/content_decryptor.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/08/15 22:03:22, ddorwin wrote:
> Filename is misleading. It should be cdm_wrapper or something like that.
> CDMWrapper isn't exactly descriptive, but nothing obvious comes to mind. It's
> really an abstraction of Pepper for the CDM (abstract away the Pepper
details).

Renamed to cdm_wrapper.cc; moved file to ppapi subdir in webkit/media/crypto.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:5: #include <cassert>
On 2012/08/15 22:03:22, ddorwin wrote:
> Why not use DCHECK? Can we not use it in plugins? I think there is a PP_DCHECK
> or something.

Removed.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:50: // A wrapper class responsible for
managing interaction between the browser and
On 2012/08/15 22:03:22, ddorwin wrote:
> Maybe something like:
> ... for abstracting away PPAPI interaction and threading for a Content
> Decryption Module (CDM). 

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/media/crypto...
webkit/media/crypto/content_decryptor.cc:192: fprintf(stderr,
On 2012/08/15 22:03:22, ddorwin wrote:
> Use the PPB_Console_Dev interface instead?

Mainly because the output sent to PPB_Console_Dev goes to the JS console, not my
terminal (maybe they go both places; I'm basing this on the interface comments
and not experience). 

Removed the fprintf's anyway-- they were just there while I figured out what was
up with various proxy addition related problems.

[dmichael (off chromium), 2012-08-16 17:02:28]

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
File ppapi/cpp/private/content_decryptor_private.h (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:26: virtual bool
GenerateKeyRequest(const std::string& key_system,
On 2012/08/16 03:10:48, tomf wrote:
> On 2012/08/15 17:44:37, dmichael wrote:
> > Re-thinking it, if you just have to turn around and pass the key_system back
> to
> > another call, it may make sense to leave it as pp::Var. It's slightly less
> > efficient to do it this way, because we'll end up making a new Var and
copying
> > the contents of key_system (same for session_id and other string var
arguments
> > that get passed right back to the browser).
> > 
> > No need to change it for now, just something to keep in mind as you write to
> > this API. It's kind of a tradeoff, since std::string is certainly
friendlier.
> 
> It's going to require extraction to a string within the wrapper anyway--
> decryption module wrapped by the plugin will be expecting a string (I think).
My point was that some of these strings will *also* be sent back to the browser
as Vars. So you could extract the string within GenerateKeyRequest to send to
the CDM, but send the same Var back to the browser, so that we don't have an
extra string->Var conversion. The performance savings would probably be pretty
slight, though, so the way you have it is good.

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/10827280/diff/6023/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:168:
SerializedVarReceiveInput init_data) {
On 2012/08/15 19:01:00, dmichael wrote:
> I think you might need to AddRef your key_system and init_data vars.
> SerializedVarReceiveInput decrements the ref-count upon exiting this function,
> but the way your C++ wrapper is written, it looks like the CDM expects to
> receive a Var with ref-count of 1.
ping? See PPP_Messaging_Proxy for an example. As-is, you might not be getting
any obvious side effects if you only use the var within the scope of
GenerateKeyRequest. But if the CDM tries to AddRef the Var and keep it around
after returning from GenerateKeyRequest, it will probably disappear out from
under it. Comment applies all places where you use SerializedVarReceiveInput in
the PPP proxy.

http://codereview.chromium.org/10827280/diff/6023/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10827280/diff/6023/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:299: // buffer resource, and
returns it. Returns a an invalid PP_Resource with an ID
On 2012/08/15 19:01:00, dmichael wrote:
> note that the reterned reference count is 1?
Maybe note that the returned Buffer resource has a reference count of 1?

http://codereview.chromium.org/10827280/diff/10003/ppapi/proxy/ppp_content_de...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/10827280/diff/10003/ppapi/proxy/ppp_content_de...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:54:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
It's a little stupid the way we've copied/pasted this code around... can you put
a TODO here with my name on it? Something like:
TODO(dmichael): Refactor so this handle sharing code is in one place.

http://codereview.chromium.org/10827280/diff/10003/ppapi/proxy/ppp_content_de...
File ppapi/proxy/ppp_content_decryptor_private_proxy.h (right):

http://codereview.chromium.org/10827280/diff/10003/ppapi/proxy/ppp_content_de...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:40: PPPDecryptor_Buffer
encrypted_buffer,
const &?

[Tom Finegan, 2012-08-16 18:10:58]

ddorwin, xhwang, dmichael: I think we're ready to land. Please take a quick look
at the TODOs and LOG(INFO) abuse in ppapi_plugin_instance.cc-- they should be
moved out as soon as the revision with real decryption lands, but for now I
think they need to stay to avoid breaking builds.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/cpp/private/c...
ppapi/cpp/private/content_decryptor_private.h:26: virtual bool
GenerateKeyRequest(const std::string& key_system,
On 2012/08/16 17:02:28, dmichael wrote:
> On 2012/08/16 03:10:48, tomf wrote:
> > On 2012/08/15 17:44:37, dmichael wrote:
> > > Re-thinking it, if you just have to turn around and pass the key_system
back
> > to
> > > another call, it may make sense to leave it as pp::Var. It's slightly less
> > > efficient to do it this way, because we'll end up making a new Var and
> copying
> > > the contents of key_system (same for session_id and other string var
> arguments
> > > that get passed right back to the browser).
> > > 
> > > No need to change it for now, just something to keep in mind as you write
to
> > > this API. It's kind of a tradeoff, since std::string is certainly
> friendlier.
> > 
> > It's going to require extraction to a string within the wrapper anyway--
> > decryption module wrapped by the plugin will be expecting a string (I
think).
> My point was that some of these strings will *also* be sent back to the
browser
> as Vars. So you could extract the string within GenerateKeyRequest to send to
> the CDM, but send the same Var back to the browser, so that we don't have an
> extra string->Var conversion. The performance savings would probably be pretty
> slight, though, so the way you have it is good.

Added a TODO re this being an optimization.

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/ppp_con...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/ppapi/proxy/ppp_con...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:168:
SerializedVarReceiveInput init_data) {
On 2012/08/16 17:02:28, dmichael wrote:
> On 2012/08/15 19:01:00, dmichael wrote:
> > I think you might need to AddRef your key_system and init_data vars.
> > SerializedVarReceiveInput decrements the ref-count upon exiting this
function,
> > but the way your C++ wrapper is written, it looks like the CDM expects to
> > receive a Var with ref-count of 1.
> ping? See PPP_Messaging_Proxy for an example. As-is, you might not be getting
> any obvious side effects if you only use the var within the scope of
> GenerateKeyRequest. But if the CDM tries to AddRef the Var and keep it around
> after returning from GenerateKeyRequest, it will probably disappear out from
> under it. Comment applies all places where you use SerializedVarReceiveInput
in
> the PPP proxy.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:299: // buffer resource, and
returns it. Returns a an invalid PP_Resource with an ID
On 2012/08/16 17:02:28, dmichael wrote:
> On 2012/08/15 19:01:00, dmichael wrote:
> > note that the reterned reference count is 1?
> Maybe note that the returned Buffer resource has a reference count of 1?

Done; sorry, missed this one too.

https://chromiumcodereview.appspot.com/10827280/diff/6023/webkit/plugins/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1376: ScopedPPResource
encrypted_resource(ScopedPPResource::PassRef(),
On 2012/08/15 19:01:00, dmichael wrote:
> I don't think you want the PassRef version here, do you? At least, you don't
use
> it in Decrypt...

Done.

https://chromiumcodereview.appspot.com/10827280/diff/10003/ppapi/proxy/ppp_co...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

https://chromiumcodereview.appspot.com/10827280/diff/10003/ppapi/proxy/ppp_co...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:54:
reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
On 2012/08/16 17:02:28, dmichael wrote:
> It's a little stupid the way we've copied/pasted this code around... can you
put
> a TODO here with my name on it? Something like:
> TODO(dmichael): Refactor so this handle sharing code is in one place.

Done.

https://chromiumcodereview.appspot.com/10827280/diff/10003/ppapi/proxy/ppp_co...
File ppapi/proxy/ppp_content_decryptor_private_proxy.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/10003/ppapi/proxy/ppp_co...
ppapi/proxy/ppp_content_decryptor_private_proxy.h:40: PPPDecryptor_Buffer
encrypted_buffer,
On 2012/08/16 17:02:28, dmichael wrote:
> const &?

Done.

[dmichael (off chromium), 2012-08-16 18:51:00]

lgtm

If you really don't *want* the logging in there for now (i.e., it's *only* there
to avoid compile errors), I'd rather you took it out and maybe just remove or
comment out stuff to shut the compiler up. (E.g., you could comment out the
param names and remove the function body).

If the LOG stuff is useful to you, though, go ahead and leave it for now. Since
only your CDM code should hit the LOGs, it's not a big deal.

http://codereview.chromium.org/10827280/diff/14005/ppapi/cpp/private/content_...
File ppapi/cpp/private/content_decryptor_private.h (right):

http://codereview.chromium.org/10827280/diff/14005/ppapi/cpp/private/content_...
ppapi/cpp/private/content_decryptor_private.h:28: // that way the same var can
be reused when replying to the browser.
You can leave out this TODO. If you want to keep it, maybe rephrase to a
complete sentence like "This could be optimized..." to make it clear that you're
not talking about an existing optimization

[Tom Finegan, 2012-08-16 19:08:05]

ddorwin, xhwang: Do you two want to take one last look?

Fixed TODO and removed the extra LOG messages.

https://chromiumcodereview.appspot.com/10827280/diff/14005/ppapi/cpp/private/...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://chromiumcodereview.appspot.com/10827280/diff/14005/ppapi/cpp/private/...
ppapi/cpp/private/content_decryptor_private.h:28: // that way the same var can
be reused when replying to the browser.
On 2012/08/16 18:51:00, dmichael wrote:
> You can leave out this TODO. If you want to keep it, maybe rephrase to a
> complete sentence like "This could be optimized..." to make it clear that
you're
> not talking about an existing optimization

Done; reworded it-- it'll be nice to have the reminder of the optimization.

[commit-bot: I haz the power, 2012-08-16 19:10:30]

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

[commit-bot: I haz the power, 2012-08-16 19:10:44]

Presubmit check for 10827280-13004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/media

Presubmit checks took 2.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[ddorwin, 2012-08-16 19:32:32]

lgtm

webkit/media LGTM.

[commit-bot: I haz the power, 2012-08-16 19:32:51]

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

[commit-bot: I haz the power, 2012-08-16 19:54:47]

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

[commit-bot: I haz the power, 2012-08-16 20:19:58]

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

[commit-bot: I haz the power, 2012-08-16 20:38:08]

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

[commit-bot: I haz the power, 2012-08-16 20:56:33]

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

[commit-bot: I haz the power, 2012-08-16 21:10:59]

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

[commit-bot: I haz the power, 2012-08-16 21:25:15]

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

[commit-bot: I haz the power, 2012-08-17 00:18:45]

Change committed as 152004