Add Promises for EME (Chromium side)

Add Promises for EME (Chromium side)

Now that the EME-WD spec has changed to specify promises, implement
them on the Chromium side. They currently get converted back to
events before passing them to blink:: until the blink interface gets
changed.

Additional changes in this CL:
1. Pass a reference to the promise though PPAPI.
2. Roll DEPS for CDM.h to include CDM_5.
3. Update cdm_adapter to use CDM_5 (in addition to existing CDM_4).
4. Change External Clear Key to use CDM_5.
5. Since CDM_5 references sessions by the actual session id (a string,
   web_session_id), switch to using it rather than the previously used
   session_id (which is an integer, and used as a reference).

BUG=358271
TEST=all existing encrypted media layout and browser tests pass

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

[jrummell, 2014-05-02 18:26:38]

PTAL. This is targeted for M36, but might as well get some initial comments now.

[prabhur, 2014-05-05 17:57:29]

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:42: void
ProxyMediaKeys::CreateSession(
Do we need a bool return value as before to indicate failure?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:51: if (init_data_type ==
"audio/mp4" || init_data_type == "video/mp4") {
Is the value expected to be always lower case? i.e is "AUDIO/mp4" an invalid
data_type ?

[ddorwin, 2014-05-05 18:35:41]

Feedback on a sampling of files.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:52:
WebContentDecryptionModuleSessionImpl* CdmSessionAdapter::CreateSession(
This will probably be replaced with a resolved promise that passes the new
object to Blink, right?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.h:44: // created session.
Why was the following removed? Is it no longer true?
RemoveSession() must be called when destroying it.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:18: #include
"media/base/media_keys_session_promise.h"
Not used.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:13: #include
"media/base/media_keys.h"
duplicate with .h

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:37: void
WebContentDecryptionModuleSessionImpl::initializeNewSession(
I wonder if we'll eventually want to move session creation to webcdm_impl.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:38: const
blink::WebString& init_data_type,
We might want to DLOG(WARNING) if this includes a '/', which would likely be a
MIME type.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:45:
OnSessionError("InvalidCharacterError",
This is a Chromium choice, not a spec issue, so we should just report
NotSupportedError.

Ditto with the text - use what's in the spec.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:53:
media::PromiseResolvedCB(),
This is weird. As noted in the promise header, we shouldn't have two resolve
promises.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:54:
base::Bind(&WebContentDecryptionModuleSessionImpl::setSessionId,
I think we're going to need to do more than set the session ID - we'll need to
tell Blink to create the session. The name should reflect this.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:56:
base::Bind(&WebContentDecryptionModuleSessionImpl::OnSessionError,
Even if we rely on the same code in the interim, the types of errors are
different and this CB should reflect that.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:68:
scoped_ptr<media::MediaKeysSessionPromise> promise(
You might consider helper functions for creating the sessions. It will make this
code cleaner.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:112: // back
into blink:: (as the code there will handle the names).
Not if it is "CDM4ClientError"

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:113: if
(error_name == "CDM4ClientError") {
What about output error?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:52: // Helpers
nit: Unnecessary.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:65: // NOTE:
Since promises will last until they are fired, keep a weak reference
nits:
Remove "NOTE: "
s/last/live/
_they_ keep? <- This comment is documenting the WPF, so it should explain it. We
don't keep a weak reference here.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:42: // Must be kept in sync with
incomplete

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:56: // |promise| is fired.
s/fired/resolved/. (It would not be okay to call after rejecting. And the API
Blink API will prevent it.)

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:65: // |promise| is fired.
ditto

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
File media/base/media_keys_session_promise.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:27: // session actions. These may be
called synchronously or asynchronously.
Sync is fine as long as we enforce async, which we probably do by posting a task
somewhere.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:30: class MEDIA_EXPORT
MediaKeysSessionPromise {
The non-session promises are on MKS, but the session promises are on MK. Maybe
you can use that or what they do as a difference in the names (see below).

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:32:
MediaKeysSessionPromise(PromiseResolvedCB resolve_cb,
It's odd to be passing two different resolve options when only one can ever be
called by the receiver. Same with the two methods below. We should have separate
classes, use templates, or something.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:35: virtual ~MediaKeysSessionPromise();
None of these need to be virtual.

[ddorwin, 2014-05-05 18:37:48]

Let's discuss error_name and using an enum.

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

https://codereview.chromium.org/265993002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:1103: : error_name(error_name),
How will we transition from CDM_5 to prefixed? There's nothing forcing the
error_name to be recognizable.

I wonder whether we shouldn't just use an enum for error_name, especially since
that would allow Blink to ensure we use the right string.

[ddorwin, 2014-05-05 18:44:45]

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
File media/base/media_keys_session_promise.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:30: class MEDIA_EXPORT
MediaKeysSessionPromise {
On 2014/05/05 18:35:42, ddorwin wrote:
> The non-session promises are on MKS, but the session promises are on MK. Maybe
> you can use that or what they do as a difference in the names (see below).

FYI, MediaKeys.setServerCertificate() is now an exception to this.

[xhwang, 2014-05-05 20:46:41]

I only reviewed a subset of the files. Let's get the new Promise class sorted
out before we do more detailed reviews.(I put some nit comments whenever I saw
something, but you can ignore then for now.)

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:39:
manager_->DestroyCdm(cdm_id_);
We should check session_id_to_promise_map_, reject and destroy any leftover
promises.

Imagine that we call createSession, but the CDM never resolve/reject the
promise. Then user closes the tab...

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:52: session_content_type =
CREATE_SESSION_TYPE_MP4;
I don't feel "session_content_type" is an improvement from "session_type". Maybe
"create_session_content_type" to be accurate?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:61: "Unsupported EME
CreateSession content type of " + init_data_type);
s/content/init data/?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:62: promise.reset();
No need to reset because it'll go out of scope?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:81:
promise->reject("NotSupportedError", 0, "LoadSession() is not implemented.");
Hmm, I feel we are exposing Blink details to chromium by having
"NotSupportedError" here. Maybe we should have some standard error strings in
chromium, e.g. in media_keys.h

+ddorwin for comments.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:82: promise.reset();
ditto about reset.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:130:
session_ready_cb_.Run(web_session_id);
OnSessionReady() should always be the result of an Update() call, so could this
"else" happen? Maybe I am missing something. If so, please add a comment.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:144: }
We need to comment why the "else" is a legit case (e.g. the session can be
closed by the CDM?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:159: }
ditto about adding a comment, that the sessionError can be a result of a CDM
call, or just something bad happens in the CDM.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:43:
scoped_ptr<media::MediaKeysSessionPromise> promise) OVERRIDE;
Now "cdm" is the standard term in chromium and MediaKeys should only live in
Blink. (We have "MediaKeys" for historical reasons but we plan to remove them.
See http://crbug.com/309237).

Also, I am not sure about "MediaKeysSession". It's not "MediaKeys", nor
"MediaKeySession". Do you mean that this class can be used by both MediaKeys
(for createSession()) and MediaKeySession (for update() etc)?

Maybe we should just use CdmPromise here, which will handle all kinds of
Promises used in the CDM (as in spec) implementation. WDYT?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:79: // Helper functions to
keep track of session_ids <-> web_session_ids.
Please add a comment that we are still keeping SessionIds for xxx reasons. And
add a TODO to remove them.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:89: void AddPromise(uint32_t
session_id,
To be consistent to CdmSessionAdapter, how about RegisterPromise()?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:91:
scoped_ptr<media::MediaKeysSessionPromise> LookupPromise(uint32_t session_id);
"Lookup" usually is a const operation, but this isn't. How about
RetrievePromise?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:105: std::map<std::string,
uint32_t> web_session_to_session_id_map_;
Since we use string as the key, use hash_map for better performance.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:107:
session_id_to_promise_map_;
Please comment that this map owns the promises.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.h:28: // Forwards the session
ID-based callbacks of the MediaKeys interface to the
s/session/web session/

This seems a pretty big change. Can you put it in the CL description and explain
why we make this change?

Also, when we totally get rid of session ID, probably we can rename
web_session_id back to session_id?

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.h:91: typedef std::map<std::string,
Since the key is now a string, we should use hash_map for better performance.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:126: persistent ?
media::MediaKeys::SessionType::kPersistent
The "SessionType::" part isn't needed.

https://codereview.chromium.org/265993002/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.h:192:
scoped_ptr<media::MediaKeysSessionPromise> FindPromise(uint32_t promise_id);
"find" usually doesn't touch the container. Here we are removing it from the
container. 

Brainstorming: take, get, retrieve...?

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:45: kPersistent = 1
We should use UPPER_CASE for enums. Sorry that KeyError above provides a bad
example :(

Then we should have something like SESSION_TYPE_TEMPORARY...

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
File media/base/media_keys_session_promise.cc (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.cc:22:
MediaKeysSessionPromise::~MediaKeysSessionPromise() {
We should be able to DCHECK that the promise is already resolved or rejected
upon destruction. Then we can make sure there are no outstanding promises.

To do this, we can either have a flag "is_pending" to indicate whether this
promise is still "pending", or by using ResetAndReturn(&xxx_cb_).Run() when we
resolve or reject the promises. Since we have multiple callbacks, having a flag
might be easier/cleaner.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.cc:29: reject("InvalidStateError", 0,
std::string());
When could this happen? Shall we just DCHECK(!resolve_cb_.is_null())?

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
File media/base/media_keys_session_promise.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:43: virtual void resolve(const
std::string& web_session_id);
It's odd to pass Promises with two resolve() functions and only one should be
used. Maybe we should split this to two Promise classes.

https://codereview.chromium.org/265993002/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:269: promise.reset();
The |promise| will go out of scope automatically, so no need to reset().

[jrummell, 2014-05-08 23:37:44]

I think I've addressed most comments. Major changes is this PS:

- cdm_promise is now a template (so only 1 resolve method).
- CDM now passes back an exception code rather than a string. Requires another
change to the CDM.h, which I'll do later once we decide on the list of
exceptions needed by a CDM.

Also need to verify Android compilation.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:39:
manager_->DestroyCdm(cdm_id_);
On 2014/05/05 20:46:42, xhwang wrote:
> We should check session_id_to_promise_map_, reject and destroy any leftover
> promises.
> 
> Imagine that we call createSession, but the CDM never resolve/reject the
> promise. Then user closes the tab...

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:42: void
ProxyMediaKeys::CreateSession(
On 2014/05/05 17:57:29, prabhur wrote:
> Do we need a bool return value as before to indicate failure? 

Nope. The promise will be resolved or rejected, possibly asynchronously.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:51: if (init_data_type ==
"audio/mp4" || init_data_type == "video/mp4") {
On 2014/05/05 17:57:29, prabhur wrote:
> Is the value expected to be always lower case? i.e is "AUDIO/mp4" an invalid
> data_type ?

The spec states "Initialization data type strings are always matched
case-sensitively. It is recommended that initialization data type strings are
lower-case ASCII strings." We use lower-case.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:52: session_content_type =
CREATE_SESSION_TYPE_MP4;
On 2014/05/05 20:46:42, xhwang wrote:
> I don't feel "session_content_type" is an improvement from "session_type".
Maybe
> "create_session_content_type" to be accurate?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:61: "Unsupported EME
CreateSession content type of " + init_data_type);
On 2014/05/05 20:46:42, xhwang wrote:
> s/content/init data/?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:62: promise.reset();
On 2014/05/05 20:46:42, xhwang wrote:
> No need to reset because it'll go out of scope?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:82: promise.reset();
On 2014/05/05 20:46:42, xhwang wrote:
> ditto about reset.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:130:
session_ready_cb_.Run(web_session_id);
On 2014/05/05 20:46:42, xhwang wrote:
> OnSessionReady() should always be the result of an Update() call, so could
this
> "else" happen? Maybe I am missing something. If so, please add a comment.

Added comment.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:144: }
On 2014/05/05 20:46:42, xhwang wrote:
> We need to comment why the "else" is a legit case (e.g. the session can be
> closed by the CDM?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.cc:159: }
On 2014/05/05 20:46:42, xhwang wrote:
> ditto about adding a comment, that the sessionError can be a result of a CDM
> call, or just something bad happens in the CDM.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:79: // Helper functions to
keep track of session_ids <-> web_session_ids.
On 2014/05/05 20:46:42, xhwang wrote:
> Please add a comment that we are still keeping SessionIds for xxx reasons. And
> add a TODO to remove them.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:89: void AddPromise(uint32_t
session_id,
On 2014/05/05 20:46:42, xhwang wrote:
> To be consistent to CdmSessionAdapter, how about RegisterPromise()?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:91:
scoped_ptr<media::MediaKeysSessionPromise> LookupPromise(uint32_t session_id);
On 2014/05/05 20:46:42, xhwang wrote:
> "Lookup" usually is a const operation, but this isn't. How about
> RetrievePromise?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:105: std::map<std::string,
uint32_t> web_session_to_session_id_map_;
On 2014/05/05 20:46:42, xhwang wrote:
> Since we use string as the key, use hash_map for better performance.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/andro...
content/renderer/media/android/proxy_media_keys.h:107:
session_id_to_promise_map_;
On 2014/05/05 20:46:42, xhwang wrote:
> Please comment that this map owns the promises.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:52:
WebContentDecryptionModuleSessionImpl* CdmSessionAdapter::CreateSession(
On 2014/05/05 18:35:42, ddorwin wrote:
> This will probably be replaced with a resolved promise that passes the new
> object to Blink, right?

We currently don't have any blink:: types here, so I think the promise will get
resolved in the caller (WebContentDecryptionModuleImpl::createSession).

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.h:28: // Forwards the session
ID-based callbacks of the MediaKeys interface to the
On 2014/05/05 20:46:42, xhwang wrote:
> s/session/web session/
> 
> This seems a pretty big change. Can you put it in the CL description and
explain
> why we make this change?
> 
> Also, when we totally get rid of session ID, probably we can rename
> web_session_id back to session_id?

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.h:44: // created session.
On 2014/05/05 18:35:42, ddorwin wrote:
> Why was the following removed? Is it no longer true?
> RemoveSession() must be called when destroying it.

Added back (with explanation).

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.h:91: typedef std::map<std::string,
On 2014/05/05 20:46:42, xhwang wrote:
> Since the key is now a string, we should use hash_map for better performance.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:126: persistent ?
media::MediaKeys::SessionType::kPersistent
On 2014/05/05 20:46:42, xhwang wrote:
> The "SessionType::" part isn't needed.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:18: #include
"media/base/media_keys_session_promise.h"
On 2014/05/05 18:35:42, ddorwin wrote:
> Not used.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:13: #include
"media/base/media_keys.h"
On 2014/05/05 18:35:42, ddorwin wrote:
> duplicate with .h

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:38: const
blink::WebString& init_data_type,
On 2014/05/05 18:35:42, ddorwin wrote:
> We might want to DLOG(WARNING) if this includes a '/', which would likely be a
> MIME type.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:45:
OnSessionError("InvalidCharacterError",
On 2014/05/05 18:35:42, ddorwin wrote:
> This is a Chromium choice, not a spec issue, so we should just report
> NotSupportedError.
> 
> Ditto with the text - use what's in the spec.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:53:
media::PromiseResolvedCB(),
On 2014/05/05 18:35:42, ddorwin wrote:
> This is weird. As noted in the promise header, we shouldn't have two resolve
> promises.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:54:
base::Bind(&WebContentDecryptionModuleSessionImpl::setSessionId,
On 2014/05/05 18:35:42, ddorwin wrote:
> I think we're going to need to do more than set the session ID - we'll need to
> tell Blink to create the session. The name should reflect this.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:56:
base::Bind(&WebContentDecryptionModuleSessionImpl::OnSessionError,
On 2014/05/05 18:35:42, ddorwin wrote:
> Even if we rely on the same code in the interim, the types of errors are
> different and this CB should reflect that.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:68:
scoped_ptr<media::MediaKeysSessionPromise> promise(
On 2014/05/05 18:35:42, ddorwin wrote:
> You might consider helper functions for creating the sessions. It will make
this
> code cleaner.

Done as a macro. If everybody is OK with it, I'll repeat it in the other classes
that create promises.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:112: // back
into blink:: (as the code there will handle the names).
On 2014/05/05 18:35:42, ddorwin wrote:
> Not if it is "CDM4ClientError"

True. Changes with exception_code.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:113: if
(error_name == "CDM4ClientError") {
On 2014/05/05 18:35:42, ddorwin wrote:
> What about output error?

Client doesn't have a output error. Only unknown and client.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:52: // Helpers
On 2014/05/05 18:35:42, ddorwin wrote:
> nit: Unnecessary.

Done.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:65: // NOTE:
Since promises will last until they are fired, keep a weak reference
On 2014/05/05 18:35:42, ddorwin wrote:
> nits:
> Remove "NOTE: "
> s/last/live/
> _they_ keep? <- This comment is documenting the WPF, so it should explain it.
We
> don't keep a weak reference here.

Comment changed.

https://codereview.chromium.org/265993002/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.h:192:
scoped_ptr<media::MediaKeysSessionPromise> FindPromise(uint32_t promise_id);
On 2014/05/05 20:46:42, xhwang wrote:
> "find" usually doesn't touch the container. Here we are removing it from the
> container. 
> 
> Brainstorming: take, get, retrieve...?

Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:42: // Must be kept in sync with
On 2014/05/05 18:35:42, ddorwin wrote:
> incomplete

Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:45: kPersistent = 1
On 2014/05/05 20:46:42, xhwang wrote:
> We should use UPPER_CASE for enums. Sorry that KeyError above provides a bad
> example :(
> 
> Then we should have something like SESSION_TYPE_TEMPORARY...

Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:56: // |promise| is fired.
On 2014/05/05 18:35:42, ddorwin wrote:
> s/fired/resolved/. (It would not be okay to call after rejecting. And the API
> Blink API will prevent it.)

Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:65: // |promise| is fired.
On 2014/05/05 18:35:42, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
File media/base/media_keys_session_promise.cc (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.cc:22:
MediaKeysSessionPromise::~MediaKeysSessionPromise() {
On 2014/05/05 20:46:42, xhwang wrote:
> We should be able to DCHECK that the promise is already resolved or rejected
> upon destruction. Then we can make sure there are no outstanding promises.
> 
> To do this, we can either have a flag "is_pending" to indicate whether this
> promise is still "pending", or by using ResetAndReturn(&xxx_cb_).Run() when we
> resolve or reject the promises. Since we have multiple callbacks, having a
flag
> might be easier/cleaner.

Good idea. Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.cc:29: reject("InvalidStateError", 0,
std::string());
On 2014/05/05 20:46:42, xhwang wrote:
> When could this happen? Shall we just DCHECK(!resolve_cb_.is_null())?

Not needed anymore now that the classes are done by a template.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
File media/base/media_keys_session_promise.h (right):

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:32:
MediaKeysSessionPromise(PromiseResolvedCB resolve_cb,
On 2014/05/05 18:35:42, ddorwin wrote:
> It's odd to be passing two different resolve options when only one can ever be
> called by the receiver. Same with the two methods below. We should have
separate
> classes, use templates, or something.

Done.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:35: virtual ~MediaKeysSessionPromise();
On 2014/05/05 18:35:42, ddorwin wrote:
> None of these need to be virtual.

The promises are overridden in a couple of places (tests, for example). I get
errors if they're not virtual.

https://codereview.chromium.org/265993002/diff/1/media/base/media_keys_sessio...
media/base/media_keys_session_promise.h:43: virtual void resolve(const
std::string& web_session_id);
On 2014/05/05 20:46:42, xhwang wrote:
> It's odd to pass Promises with two resolve() functions and only one should be
> used. Maybe we should split this to two Promise classes.

Done.

https://codereview.chromium.org/265993002/diff/1/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:269: promise.reset();
On 2014/05/05 20:46:42, xhwang wrote:
> The |promise| will go out of scope automatically, so no need to reset().

Done.

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

https://codereview.chromium.org/265993002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:1103: : error_name(error_name),
On 2014/05/05 18:37:49, ddorwin wrote:
> How will we transition from CDM_5 to prefixed? There's nothing forcing the
> error_name to be recognizable.
> 
> I wonder whether we shouldn't just use an enum for error_name, especially
since
> that would allow Blink to ensure we use the right string.

Done.

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:46: }
Need to do the second list of promises as well.

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:29: // Forwards the wwb session
ID-based callbacks of the MediaKeys interface to the
spelling.

https://codereview.chromium.org/265993002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:1172: void
ContentDecryptorDelegate::SatisfyAllPendingCallbacksOnError() {
Need to reject any pending promises here.

https://codereview.chromium.org/265993002/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:292: PP_ExceptionCodeType
MediaKeysExceptionTypeToPpExceptionType(
Will remove this.

[jrummell, 2014-05-09 00:55:23]

Fixing the things I noticed.

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:46: }
On 2014/05/08 23:37:46, jrummell wrote:
> Need to do the second list of promises as well.

Done.

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/265993002/diff/20001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:29: // Forwards the wwb session
ID-based callbacks of the MediaKeys interface to the
On 2014/05/08 23:37:46, jrummell wrote:
> spelling.

Done.

https://codereview.chromium.org/265993002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:1172: void
ContentDecryptorDelegate::SatisfyAllPendingCallbacksOnError() {
On 2014/05/08 23:37:46, jrummell wrote:
> Need to reject any pending promises here.

Done.

https://codereview.chromium.org/265993002/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:292: PP_ExceptionCodeType
MediaKeysExceptionTypeToPpExceptionType(
On 2014/05/08 23:37:46, jrummell wrote:
> Will remove this.

Done.

[jrummell, 2014-05-13 00:47:53]

Updated. Changes to line up with the latest version of CDM.h, plus typedef'ing
the promise templates.

[ddorwin, 2014-05-13 22:43:57]

Feedback on roughly the same set of files.
One comment in PS1.

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:52:
WebContentDecryptionModuleSessionImpl* CdmSessionAdapter::CreateSession(
On 2014/05/08 23:37:45, jrummell wrote:
> On 2014/05/05 18:35:42, ddorwin wrote:
> > This will probably be replaced with a resolved promise that passes the new
> > object to Blink, right?
> 
> We currently don't have any blink:: types here, so I think the promise will
get
> resolved in the caller (WebContentDecryptionModuleImpl::createSession).

Well, WCDMI could create the promise and thus handle the resolution. The point
is that we shouldn't create the actual session object until the promise is
resolved.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:71: typedef
base::hash_map<std::string, uint32_t> SessionMap;
Is this a SessionId to Promise ID map?

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:72: typedef std::map<uint32_t,
media::CdmChangeSessionPromise*> VoidPromiseMap;
Can the pointer type just be CdmPromise?

It's odd that Void is in the map name but not the promise name.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:88: // TODO(jrummell): Remove
this once the Android API changes to support
Do you mean the Chromium code or the actual Android platform APIs?

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:119: uint32_t
next_session_id_;
Do we want them to be unique per process (static)?

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:135:
media::MediaKeys::MediaKeysException exception_code,
nit: MediaKeys in MediaKeysException is a bit redundant. (We previously talked
about the naming, and I think this makes it clear we should change it.)

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:251:
media::MediaKeys::MEDIA_KEYS_EXCEPTION_NOT_SUPPORTED_ERROR,
I see a mix of enum naming styles. (This is the Chromium-not-Google style.)
Which does media code use? kValue is more desirable in this case since we can
use the exception names exactly (plus a 'k').
Also, I don't see a requirement to prefix the name with the enum type, though I
know that's used in some places. Especially since this is scoped to a class, it
seems unnecessary.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:265: switch (exception_code) {
The switch should convert the exception then there should be one call to the cb.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:18: #define
CreatePromise(type, resolve_method, reject_method)               \
Macros aren't allowed. Can we use a function template instead?

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:109: // back
into blink:: (as blink:: will have it's own error definition).
nit: its

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:138: // Pass
back to blink:: as a ready event.
All of these are "For now,".

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:147:
OnSessionError(exception_code, system_code, error_message);
All the Failed functions do the same thing. Why do we need multiple?

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:53: void
SessionCreateFailed(media::MediaKeys::MediaKeysException exception_code,
Are these temporary until Blink is updated? Otherwise, I don't see why we need
separate functions.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.h:96: PP_ExceptionCodeType
exception_code,
Is there a reason this has "Type"?

https://codereview.chromium.org/265993002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.h:192:
scoped_ptr<media::CdmChangeSessionPromise> RetrieveVoidPromise(
"Take" is probably a better term (and used elsewhere).

https://codereview.chromium.org/265993002/diff/80001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/80001/media/base/cdm_promise.h...
media/base/cdm_promise.h:33: // Used to indicate that promise has successfully
resolved.
Be consistent with reject comment: i.e. operation succeeded.

https://codereview.chromium.org/265993002/diff/80001/media/base/cdm_promise.h...
media/base/cdm_promise.h:56: CdmPromise(base::Callback<void()> resolve_cb,
PromiseRejectedCB rejected_cb);
Use base::Closure? Or maybe that's more confusing because it doesn't look like
above.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:24: typedef CdmPromise<void> CdmChangeSessionPromise;
As earlier, "ChangeSession" is odd, and maybe not necessary at all.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:46: // Must be kept in sync with cdm::MediaKeyException
(enforced in
This shouldn't be necessary. This must be a superset of those values, but all
values supported here don't necessarily need to be supported there.

Most importantly, if we use a switch statement instead of casting, we'll ensure
we do the right thing if this list or CDM.h is changed.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:60: MEDIA_KEYS_EXCEPTION_UNKNOWN_ERROR = 30,
We don't actually need fixed integers for these since they aren't used in a
binary-compatible interface.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:70: // Must be kept in sync with the values specified in
the spec:
What do you mean in sync? The integer values shouldn't matter. Of course we need
to support the various enums, but that applies to any spec-related feature.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:72: // Must be kept in sync with cdm::SessionType
(enforced in clear_key_cdm.cc).
ditto

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:239: // For now, the AesDecryptor does not care about
|init_data_type| or
TODO: Fix this.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:267: if (valid_sessions_.find(web_session_id) ==
valid_sessions_.end()) {
Why is this no longer a DCHECK? Can't happen in WD, and it appears prefixed must
have been preventing it. Or maybe we lost the prefixed code that handled it?

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:269: MEDIA_KEYS_EXCEPTION_INVALID_STATE_ERROR, 0,
"Session does not exist.");
This is really InvalidAccessError - invalid parameter.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:278:
promise->reject(MEDIA_KEYS_EXCEPTION_SYNTAX_ERROR,
Probably InvalidAccessError. Maybe DataError, but I don't think we'll introduce
that to EME spec text.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:286:
promise->reject(MEDIA_KEYS_EXCEPTION_SYNTAX_ERROR,
InvalidAccessError

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:297: MEDIA_KEYS_EXCEPTION_SYNTAX_ERROR, 0, "Invalid
key length.");
InvalidAccessError

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:301:
promise->reject(MEDIA_KEYS_EXCEPTION_INVALID_MODIFICATION_ERROR,
I think this is also InvalidAccessError, since it would probably only happen
with a wrong-sized key.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:325: if (it == valid_sessions_.end()) {
ditto on DCHECK and exception.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:430: if (!decryption_key) {
This can't happen. We should remove it.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:435: if (!decryption_key->Init()) {
That leaves this.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:237: case cdm::kSyntaxError:
I missed that this was still in CDM.h. I don't think it should be possible above
the Blink level.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:354: // Initialize() doesn't report an error, so
CreateSession() can be called
This should all be prevented now that we have MediaKeys.create().
Or is this for prefixed? Still, we should try to avoid it if we can.
TODO?

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:374: // Initialize() doesn't report an error, so
LoadSession() can be called
ditto

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:395: if (!response_ptr || response_size <= 0) {
This really should not happen. We should try to PP_DCHECK/NOTREACHED it.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:396: RejectPromise(promise_id, cdm::kSyntaxError,
0, "response is empty.");
Really this is an InvalidAccessError.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:705: // cdm::Host_4 methods
It seems odd to have 4 after 5.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.h:106: bool has_new_key);
has_additional_usable_key

[jrummell, 2014-05-15 22:38:08]

Updated. Few renames generated a lot of churn.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:71: typedef
base::hash_map<std::string, uint32_t> SessionMap;
On 2014/05/13 22:44:02, ddorwin wrote:
> Is this a SessionId to Promise ID map?

Nope. This goes with the Android API comment below.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:72: typedef std::map<uint32_t,
media::CdmChangeSessionPromise*> VoidPromiseMap;
On 2014/05/13 22:44:02, ddorwin wrote:
> Can the pointer type just be CdmPromise?
> 
> It's odd that Void is in the map name but not the promise name.

CdmPromise is currently just a template -- there is no base class CdmPromise.

Changed to remove void. That was before I had the typedefs, so it mapped
CdmPromise<void>*.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:88: // TODO(jrummell): Remove
this once the Android API changes to support
On 2014/05/13 22:44:02, ddorwin wrote:
> Do you mean the Chromium code or the actual Android platform APIs?

The Chromium code for Android. Reworded.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:119: uint32_t
next_session_id_;
On 2014/05/13 22:44:02, ddorwin wrote:
> Do we want them to be unique per process (static)?

I don't think it matters. We are looking them up in the maps this class owns, so
per process won't affect it.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:135:
media::MediaKeys::MediaKeysException exception_code,
On 2014/05/13 22:44:02, ddorwin wrote:
> nit: MediaKeys in MediaKeysException is a bit redundant. (We previously talked
> about the naming, and I think this makes it clear we should change it.)

Was changed for cdm::. Now done here.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:251:
media::MediaKeys::MEDIA_KEYS_EXCEPTION_NOT_SUPPORTED_ERROR,
On 2014/05/13 22:44:02, ddorwin wrote:
> I see a mix of enum naming styles. (This is the Chromium-not-Google style.)
> Which does media code use? kValue is more desirable in this case since we can
> use the exception names exactly (plus a 'k').
> Also, I don't see a requirement to prefix the name with the enum type, though
I
> know that's used in some places. Especially since this is scoped to a class,
it
> seems unnecessary.

Looks like ENUM_NAME is the preferred style. Removed the prefix, since that
looks like a blink:: convention.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:265: switch (exception_code) {
On 2014/05/13 22:44:02, ddorwin wrote:
> The switch should convert the exception then there should be one call to the
cb.

Done.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:18: #define
CreatePromise(type, resolve_method, reject_method)               \
On 2014/05/13 22:44:02, ddorwin wrote:
> Macros aren't allowed. Can we use a function template instead?

Tried, but looks like the callers would need to pass in the Callbacks. Then all
this function would do is abstract "new media:CdmPromise<T>", which hardly seems
worth it (and the calls below would be just as long as they are now).

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:109: // back
into blink:: (as blink:: will have it's own error definition).
On 2014/05/13 22:44:02, ddorwin wrote:
> nit: its

Done.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:138: // Pass
back to blink:: as a ready event.
On 2014/05/13 22:44:02, ddorwin wrote:
> All of these are "For now,".

Done.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:147:
OnSessionError(exception_code, system_code, error_message);
On 2014/05/13 22:44:02, ddorwin wrote:
> All the Failed functions do the same thing. Why do we need multiple?

Fixed.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:53: void
SessionCreateFailed(media::MediaKeys::MediaKeysException exception_code,
On 2014/05/13 22:44:02, ddorwin wrote:
> Are these temporary until Blink is updated? Otherwise, I don't see why we need
> separate functions.

Sort of. I did it this way in case there was specific code needed for the
different cases when blink:: passes in promises. Will get cleaned up when we
actually call back to blink::.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.h:96: PP_ExceptionCodeType
exception_code,
On 2014/05/13 22:44:02, ddorwin wrote:
> Is there a reason this has "Type"?

I just looked at the last entry in pp_content_decryptor.idl and assumed it was
the convention. But looks like it's not necessary, so it's removed.

https://codereview.chromium.org/265993002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.h:192:
scoped_ptr<media::CdmChangeSessionPromise> RetrieveVoidPromise(
On 2014/05/13 22:44:02, ddorwin wrote:
> "Take" is probably a better term (and used elsewhere).

Done.

https://codereview.chromium.org/265993002/diff/80001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/80001/media/base/cdm_promise.h...
media/base/cdm_promise.h:33: // Used to indicate that promise has successfully
resolved.
On 2014/05/13 22:44:02, ddorwin wrote:
> Be consistent with reject comment: i.e. operation succeeded.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/base/cdm_promise.h...
media/base/cdm_promise.h:56: CdmPromise(base::Callback<void()> resolve_cb,
PromiseRejectedCB rejected_cb);
On 2014/05/13 22:44:02, ddorwin wrote:
> Use base::Closure? Or maybe that's more confusing because it doesn't look like
> above.

Changed it to be more like the above call.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:24: typedef CdmPromise<void> CdmChangeSessionPromise;
On 2014/05/13 22:44:02, ddorwin wrote:
> As earlier, "ChangeSession" is odd, and maybe not necessary at all.

Now SimpleCdmPromise (and NewSessionCdmPromise).

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:46: // Must be kept in sync with cdm::MediaKeyException
(enforced in
On 2014/05/13 22:44:02, ddorwin wrote:
> This shouldn't be necessary. This must be a superset of those values, but all
> values supported here don't necessarily need to be supported there.
> 
> Most importantly, if we use a switch statement instead of casting, we'll
ensure
> we do the right thing if this list or CDM.h is changed.

Updated. Using a switch statement in clear_key_cdm.cc.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:60: MEDIA_KEYS_EXCEPTION_UNKNOWN_ERROR = 30,
On 2014/05/13 22:44:02, ddorwin wrote:
> We don't actually need fixed integers for these since they aren't used in a
> binary-compatible interface.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:70: // Must be kept in sync with the values specified in
the spec:
On 2014/05/13 22:44:02, ddorwin wrote:
> What do you mean in sync? The integer values shouldn't matter. Of course we
need
> to support the various enums, but that applies to any spec-related feature.

Changed.

https://codereview.chromium.org/265993002/diff/80001/media/base/media_keys.h#...
media/base/media_keys.h:72: // Must be kept in sync with cdm::SessionType
(enforced in clear_key_cdm.cc).
On 2014/05/13 22:44:02, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:239: // For now, the AesDecryptor does not care about
|init_data_type| or
On 2014/05/13 22:44:02, ddorwin wrote:
> TODO: Fix this.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:267: if (valid_sessions_.find(web_session_id) ==
valid_sessions_.end()) {
On 2014/05/13 22:44:02, ddorwin wrote:
> Why is this no longer a DCHECK? Can't happen in WD, and it appears prefixed
must
> have been preventing it. Or maybe we lost the prefixed code that handled it?

Prefixed was handling it prior to this when converting web_session_id to uint in
ProxyDecryptor. Added a TODO.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:269: MEDIA_KEYS_EXCEPTION_INVALID_STATE_ERROR, 0,
"Session does not exist.");
On 2014/05/13 22:44:02, ddorwin wrote:
> This is really InvalidAccessError - invalid parameter.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:278:
promise->reject(MEDIA_KEYS_EXCEPTION_SYNTAX_ERROR,
On 2014/05/13 22:44:02, ddorwin wrote:
> Probably InvalidAccessError. Maybe DataError, but I don't think we'll
introduce
> that to EME spec text.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:286:
promise->reject(MEDIA_KEYS_EXCEPTION_SYNTAX_ERROR,
On 2014/05/13 22:44:02, ddorwin wrote:
> InvalidAccessError

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:297: MEDIA_KEYS_EXCEPTION_SYNTAX_ERROR, 0, "Invalid
key length.");
On 2014/05/13 22:44:02, ddorwin wrote:
> InvalidAccessError

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:301:
promise->reject(MEDIA_KEYS_EXCEPTION_INVALID_MODIFICATION_ERROR,
On 2014/05/13 22:44:02, ddorwin wrote:
> I think this is also InvalidAccessError, since it would probably only happen
> with a wrong-sized key.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:325: if (it == valid_sessions_.end()) {
On 2014/05/13 22:44:02, ddorwin wrote:
> ditto on DCHECK and exception.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:430: if (!decryption_key) {
On 2014/05/13 22:44:02, ddorwin wrote:
> This can't happen. We should remove it.

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:237: case cdm::kSyntaxError:
On 2014/05/13 22:44:02, ddorwin wrote:
> I missed that this was still in CDM.h. I don't think it should be possible
above
> the Blink level.

Do you want it removed from cdm.h?

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:354: // Initialize() doesn't report an error, so
CreateSession() can be called
On 2014/05/13 22:44:02, ddorwin wrote:
> This should all be prevented now that we have MediaKeys.create().
> Or is this for prefixed? Still, we should try to avoid it if we can.
> TODO?

Added TODO.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:374: // Initialize() doesn't report an error, so
LoadSession() can be called
On 2014/05/13 22:44:02, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:395: if (!response_ptr || response_size <= 0) {
On 2014/05/13 22:44:02, ddorwin wrote:
> This really should not happen. We should try to PP_DCHECK/NOTREACHED it.

Converted to PP_DCHECKs.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:396: RejectPromise(promise_id, cdm::kSyntaxError,
0, "response is empty.");
On 2014/05/13 22:44:02, ddorwin wrote:
> Really this is an InvalidAccessError.

Removed.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:705: // cdm::Host_4 methods
On 2014/05/13 22:44:02, ddorwin wrote:
> It seems odd to have 4 after 5.

Reordered.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.h:106: bool has_new_key);
On 2014/05/13 22:44:02, ddorwin wrote:
> has_additional_usable_key

Done.

[ddorwin, 2014-05-15 23:15:31]

The files I've been reviewing look good overall. Thanks. I defer to xhwang for a
full review.

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/80001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:237: case cdm::kSyntaxError:
On 2014/05/15 22:38:09, jrummell wrote:
> On 2014/05/13 22:44:02, ddorwin wrote:
> > I missed that this was still in CDM.h. I don't think it should be possible
> above
> > the Blink level.
> 
> Do you want it removed from cdm.h?

Yes, please.

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:125: SimplePromiseMap
session_id_to_change_session_promise_map_;
s/change_session/simple/ ?

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:60:
base::Bind(&WebContentDecryptionModuleSessionImpl::SessionError,
Once the Blink changes are made, will we construct the promise with a Blink
promise object so that everything gets resolved correctly?

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:273: case
PP_EXCEPTIONCODE_SYNTAXERROR:
Let's not support this in Pepper (and remove from CDM.h).

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:242: std::map<uint32_t,
media::SimpleCdmPromise*> void_promises_;
OOC, what do we do if this object is destructed and there are still promises? We
should probably reject them all. WDYT?

https://codereview.chromium.org/265993002/diff/100001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/100001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:357: // that unprefixed call to Initialize() that
fail do not resolve the promise.
This sentence seems to be broken somehow. Same at 379.

[jrummell, 2014-05-16 00:49:53]

Removing syntax error. PTAL.

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:125: SimplePromiseMap
session_id_to_change_session_promise_map_;
On 2014/05/15 23:15:32, ddorwin wrote:
> s/change_session/simple/ ?

Done.

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/media/...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:60:
base::Bind(&WebContentDecryptionModuleSessionImpl::SessionError,
On 2014/05/15 23:15:32, ddorwin wrote:
> Once the Blink changes are made, will we construct the promise with a Blink
> promise object so that everything gets resolved correctly?

I was just going to pass the blink promise to each method (as part of the Bind,
not an additional class), which would call the appropriate method on it.

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:273: case
PP_EXCEPTIONCODE_SYNTAXERROR:
On 2014/05/15 23:15:32, ddorwin wrote:
> Let's not support this in Pepper (and remove from CDM.h).

Done.

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/100001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:242: std::map<uint32_t,
media::SimpleCdmPromise*> void_promises_;
On 2014/05/15 23:15:32, ddorwin wrote:
> OOC, what do we do if this object is destructed and there are still promises?
We
> should probably reject them all. WDYT?

There is a TODO in SatisfyAllPendingCallbacksOnError() to do this. Currently
there are 2 tests that purposely crash the CDM, and don't expect to get any
calls after that. Rejecting the promise generates an error event today, so the
tests fail. Once we have promises the tests will have to change.

https://codereview.chromium.org/265993002/diff/100001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/100001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:357: // that unprefixed call to Initialize() that
fail do not resolve the promise.
On 2014/05/15 23:15:32, ddorwin wrote:
> This sentence seems to be broken somehow. Same at 379.

Split into seperate TODOs.

[xhwang, 2014-05-22 16:44:13]

I only looked at cdm_promise.* and have some comments there. These comments are
all CdmPromise internal stuff and won't affect other files. I'll keep reviewing
the rest of the CL.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:20: PromiseRejectedCB;
This seems to be only used in class CdmPromise. How about moving the typedef
into class CdmPromise?

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:46: PromiseRejectedCB rejected_cb_;
we have |resolve_cb_| but |reject"ed"_cb_|. Shall we be consistent?  e.g.
resolve_cb & reject_cb?

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:47: bool is_pending_;
Add comment for |is_pending_|.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:65: base::Callback<void(void)> resolve_cb_;
Actually base::Callback<void(void)> == base::Closure :)

https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=766

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:65: base::Callback<void(void)> resolve_cb_;
How about having a template class 

template <typename T> struct PromiseResolveCB {
  base::Callback<void(const T&)> cb;
};

template <> struct PromiseResolveCB<void> {
  base::Callback<void(void)> cb;
};

Then you can use PromiseResolveCB<T> resulve_cb_ in CdmPromise and you don't
have to specialize the full class, which will save a lot of duplicate code in
this class.

The downside of this is that you'll need to do something like
resolve_cb_.cb.Run()
But this seems fine to me since it's implementation detail of this class. WDYT?

[xhwang, 2014-05-23 06:01:22]

This is a huge CL :) I stopped at aes_decryptor.* 

Let's address the existing comments first and continue in the next round.

Most comments are trivial. I wonder does it make sense to use a base Promise
class so that we could remove some duplicate code. See detailed comment at:
https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...

+ddorwin: let me know what you think.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:58: }
You need to delete it->second as well otherwise the memory will leak.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:100:
EXCEPTION_NOT_SUPPORTED_ERROR, 0, "LoadSession() is not implemented.");
s/implemented/supported/

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:159:
session_closed_cb_.Run(web_session_id);
This will be messy if we call ReleaseSession(), and before the call gets to the
CDM, the CDM fires SessionClosed() by itself. But I think it's fine in practice.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:197: }
The nested if/else is hard to read, how about:

 181   scoped_ptr<media::NewSessionCdmPromise> promise =
 182       TakeNewSessionPromise(session_id);
 183   if (promise) {
 184     promise->reject(exception_code, system_code, std::string());
         return;
 185   }

       // Maybe an error for other set of promises.
 187   scoped_ptr<media::SimpleCdmPromise> promise2 =
 188       TakeSimplePromise(session_id);
 189   if (promise2) 
 190     promise2->reject(exception_code, system_code, std::string());
         return;
 191   }

 192   // Errors generally happen in response to a request, but it is possible
 193   // for something bad to happen in the CDM and it needs to tell the
client.
 194   session_error_cb_.Run(
 195       web_session_id, exception_code, system_code, std::string());

Also, s/promise/new_session_promise/ and s/promise2/simple_promise/.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:231:
std::make_pair(web_session_id, session_id));
DCHECK that web_session_id isn't already in the map? What if the app call
UpdateSession() twice before the CDM resolve the first promise?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:257: // Should only be one
promise outstanding for any |session_id|.
ditto, what if the app calls update() twice in a row?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:286:
scoped_ptr<media::NewSessionCdmPromise> ProxyMediaKeys::TakeNewSessionPromise(
You could have 

template <typename CdmPromiseType> scoped_ptr<CdmPromiseType>
TakeCdmPromise(uint32_t session_id);

So that you don't need to define this function twice.

Hmm, to do that you need a helper function

template <typename CdmPromiseType> ... GetCdmPromiseMap();

I am not sure whether that'll lead to simpler code. Just an idea :)

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:73: typedef std::map<uint32_t,
media::NewSessionCdmPromise*> NewSessionPromiseMap;
Add comments about these maps. For example, what's the integer in these maps
(e.g. promise ID etc)?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:73: typedef std::map<uint32_t,
media::NewSessionCdmPromise*> NewSessionPromiseMap;
Comment that the promise maps owns the promises. Actually you could use
ScopedPtrHashMap so that you don't need to worry about the ownership.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:90: // support string session
ids.
nit: s/string/string web/

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:94: uint32_t
LookupSessionId(const std::string& web_session_id);
const method?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:95: std::string
LookupWebSessionId(uint32_t session_id);
const method?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.cc:60:
DCHECK(sessions_.find(web_session_id) == sessions_.end());
DCHECK(!ContainsKey(web_session_id));

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.cc:65:
DCHECK(sessions_.find(web_session_id) != sessions_.end());
ditto about ContainsKey.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:27: // This class is need so
that resolving an Update() promise triggers playback
s/need/needed

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:32: base::Callback<void()>
resolve_cb)
s/resolve/additional_resolve/ ? It's not super clear what's the relation between
this |resolve_cb| and the resolve() call in caller_promise.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:44:
scoped_ptr<media::SimpleCdmPromise> caller_promise_;
We use both composition and inheritance? This seems unnecessarily complex. Can
we drop this, and simply put caller_promise in the base media::SimpleCdmPromise?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:119: promise.reset();
Here and elsewhere: promise is a scoped_ptr; you don't really need to reset it
here.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:103: DVLOG(1) <<
"GenerateKeyRequest()";
nit: usually we put this log in the first line of the function body.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:135: std::string())));
Can we avoid creating this promise in two places? We can first figure out
1, persistent
2, load or create session.

Then we can create a promise with |persistent| (1), then call either
CreateSession or LoadSession based on (2).

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:148: // If |web_session_id| is
null, then use the single reference id.
Can you elaborate more in which case this could happen?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.h:119: std::set<std::string>
active_sessions_;
comment that this includes both persistent and non-persistent sessions.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:147:
OnSessionError(exception_code, system_code, error_message);
It seems SessionFoo() just calls OnSessionFoo(). Why do we need this extra layer
of indirection?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:30: using
media::Decryptor;
nit: using media::*Promise and media::MediaKeys here?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:646: promise->resolve();
Is the CdmWrapper making sure the |promise_id| returned is valid? Otherwise, a
CDM bug (e.g. sending an invalid promise ID) may cause a crash in the render
process.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:674:
error_description->value());
nit: return early here so that you don't need the "else"

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1208: return
result.Pass();
There are some duplication here:
1, 2 maps instead of 1 and then we have two sets of SavePromise and TakePromise.
2, The reject() calls of the two Promise types are actually identical.

I wonder does it make sense to have a base CdmPromiseBase class, which provides
the reject() method (since all promises share the same reject()). Then we have
CdmPromise<T> inherits CdmPromiseBase. Then in these conversion points (e.g.
here and ProxyMediaKeys), we only keep one map that stores CdmPromiseBase. In
the rejection case, we only need to call reject() on CdmPromiseBase. In the
resolve case, we need to static_cast CdmPromiseBase into a specific CdmPromise.
But since for the resolve case, we always know the Promise type, this isn't an
issue.

With that, we'll only have one map and we can remove a lot of duplicate code.

More about naming:

If the above idea makes sense to you. Maybe the base class could be CdmPromise.
The template class could be CdmPromiseTemplate. Since CdmPromiseTemplate will be
typedef'ed, so we don't really care about it's name.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:107: PP_Var
error_description_var);
OOC, is it a convention to use foo_var for PP_VAR type variables?

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:243: std::map<uint32_t,
media::NewSessionCdmPromise*> new_session_promises_;
Use ScopedPtrHashMap.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:27: class MEDIA_EXPORT CdmPromise {
See above for the idea of having a base class for all CDM Promises.

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:64: EXCEPTION_OUTPUT_ERROR
Hmm, DOM "exceptions" are all "error"s... Generally enums should start with the
enum name (in this case EXCEPTION). But "EXCEPTION" and "ERROR" seems redundant.
I'am fine with dropping the "EXCEPTION" prefix... WDYT? +ddorwin for comments.

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.cc:257: EXCEPTION_NOT_SUPPORTED_ERROR, 0, "LoadSession()
is not implemented.");
I don't think AesDecryptor will ever support LoadSession.
s/implemented/supported.

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor.h
File media/cdm/aes_decryptor.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.h:30: AesDecryptor(const SessionMessageCB&
session_message_cb);
explicit

[ddorwin, 2014-05-23 16:41:08]

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:35: enum KeyError {
This is no longer used in this header, so it probably doesn't belong here. Do we
now only need it in ProxyDecryptor?

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:64: EXCEPTION_OUTPUT_ERROR
On 2014/05/23 06:01:23, xhwang wrote:
> Hmm, DOM "exceptions" are all "error"s... Generally enums should start with
the
> enum name (in this case EXCEPTION). But "EXCEPTION" and "ERROR" seems
redundant.
> I'am fine with dropping the "EXCEPTION" prefix... WDYT? +ddorwin for comments.

I don't think there is a general *requirement* for prefixes. (The Content API
and other areas may impose one.) I think it's fine to drop EXCEPTION, especially
since ...ERROR is consistent with the DOM error names.

We should try to be consistent in the enum below too. WDYT?
Maybe TEMPORARY_SESSION?

[xhwang, 2014-05-23 16:47:35]

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:64: EXCEPTION_OUTPUT_ERROR
On 2014/05/23 16:41:09, ddorwin wrote:
> On 2014/05/23 06:01:23, xhwang wrote:
> > Hmm, DOM "exceptions" are all "error"s... Generally enums should start with
> the
> > enum name (in this case EXCEPTION). But "EXCEPTION" and "ERROR" seems
> redundant.
> > I'am fine with dropping the "EXCEPTION" prefix... WDYT? +ddorwin for
comments.
> 
> I don't think there is a general *requirement* for prefixes. (The Content API
> and other areas may impose one.) I think it's fine to drop EXCEPTION,
especially
> since ...ERROR is consistent with the DOM error names.
> 
> We should try to be consistent in the enum below too. WDYT?
> Maybe TEMPORARY_SESSION?

I have no strong opinion on this one :)

[jrummell, 2014-05-29 00:54:39]

Updated. Still need to get Android to work with all the recent changes to that
code.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:58: }
On 2014/05/23 06:01:23, xhwang wrote:
> You need to delete it->second as well otherwise the memory will leak.

They are now ScopedPtrHashMaps which should clean up properly.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:100:
EXCEPTION_NOT_SUPPORTED_ERROR, 0, "LoadSession() is not implemented.");
On 2014/05/23 06:01:23, xhwang wrote:
> s/implemented/supported/

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:197: }
On 2014/05/23 06:01:23, xhwang wrote:
> The nested if/else is hard to read, how about:
> 
>  181   scoped_ptr<media::NewSessionCdmPromise> promise =
>  182       TakeNewSessionPromise(session_id);
>  183   if (promise) {
>  184     promise->reject(exception_code, system_code, std::string());
>          return;
>  185   }
> 
>        // Maybe an error for other set of promises.
>  187   scoped_ptr<media::SimpleCdmPromise> promise2 =
>  188       TakeSimplePromise(session_id);
>  189   if (promise2) 
>  190     promise2->reject(exception_code, system_code, std::string());
>          return;
>  191   }
> 
>  192   // Errors generally happen in response to a request, but it is possible
>  193   // for something bad to happen in the CDM and it needs to tell the
> client.
>  194   session_error_cb_.Run(
>  195       web_session_id, exception_code, system_code, std::string());
> 
> Also, s/promise/new_session_promise/ and s/promise2/simple_promise/.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:231:
std::make_pair(web_session_id, session_id));
On 2014/05/23 06:01:23, xhwang wrote:
> DCHECK that web_session_id isn't already in the map? What if the app call
> UpdateSession() twice before the CDM resolve the first promise?

Done. AssignWebSession() is called when the CDM resolves the createSession
promise. It would cause problems if the CDM creates multiple sessions with the
same web_session_id.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:257: // Should only be one
promise outstanding for any |session_id|.
On 2014/05/23 06:01:23, xhwang wrote:
> ditto, what if the app calls update() twice in a row?

Bad things. Ideally the promise would get passed farther down the stack,
avoiding this problem. Another CL.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:286:
scoped_ptr<media::NewSessionCdmPromise> ProxyMediaKeys::TakeNewSessionPromise(
On 2014/05/23 06:01:23, xhwang wrote:
> You could have 
> 
> template <typename CdmPromiseType> scoped_ptr<CdmPromiseType>
> TakeCdmPromise(uint32_t session_id);
> 
> So that you don't need to define this function twice.
> 
> Hmm, to do that you need a helper function
> 
> template <typename CdmPromiseType> ... GetCdmPromiseMap();
> 
> I am not sure whether that'll lead to simpler code. Just an idea :)

Noe that there is a CdmPromise base class this gets simpler.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:73: typedef std::map<uint32_t,
media::NewSessionCdmPromise*> NewSessionPromiseMap;
On 2014/05/23 06:01:23, xhwang wrote:
> Add comments about these maps. For example, what's the integer in these maps
> (e.g. promise ID etc)?

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:73: typedef std::map<uint32_t,
media::NewSessionCdmPromise*> NewSessionPromiseMap;
On 2014/05/23 06:01:23, xhwang wrote:
> Comment that the promise maps owns the promises. Actually you could use
> ScopedPtrHashMap so that you don't need to worry about the ownership.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:90: // support string session
ids.
On 2014/05/23 06:01:23, xhwang wrote:
> nit: s/string/string web/

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:94: uint32_t
LookupSessionId(const std::string& web_session_id);
On 2014/05/23 06:01:23, xhwang wrote:
> const method?

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:95: std::string
LookupWebSessionId(uint32_t session_id);
On 2014/05/23 06:01:23, xhwang wrote:
> const method?

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.cc:60:
DCHECK(sessions_.find(web_session_id) == sessions_.end());
On 2014/05/23 06:01:23, xhwang wrote:
> DCHECK(!ContainsKey(web_session_id));

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.cc:65:
DCHECK(sessions_.find(web_session_id) != sessions_.end());
On 2014/05/23 06:01:23, xhwang wrote:
> ditto about ContainsKey.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:27: // This class is need so
that resolving an Update() promise triggers playback
On 2014/05/23 06:01:23, xhwang wrote:
> s/need/needed

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:32: base::Callback<void()>
resolve_cb)
On 2014/05/23 06:01:23, xhwang wrote:
> s/resolve/additional_resolve/ ? It's not super clear what's the relation
between
> this |resolve_cb| and the resolve() call in caller_promise.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:44:
scoped_ptr<media::SimpleCdmPromise> caller_promise_;
On 2014/05/23 06:01:23, xhwang wrote:
> We use both composition and inheritance? This seems unnecessarily complex. Can
> we drop this, and simply put caller_promise in the base
media::SimpleCdmPromise?
>  

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:119: promise.reset();
On 2014/05/23 06:01:23, xhwang wrote:
> Here and elsewhere: promise is a scoped_ptr; you don't really need to reset it
> here.

Done to make it obvious. Removed since the promise keeps track of whether it has
been completed or not.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:103: DVLOG(1) <<
"GenerateKeyRequest()";
On 2014/05/23 06:01:23, xhwang wrote:
> nit: usually we put this log in the first line of the function body.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:135: std::string())));
On 2014/05/23 06:01:23, xhwang wrote:
> Can we avoid creating this promise in two places? We can first figure out
> 1, persistent
> 2, load or create session.
> 
> Then we can create a promise with |persistent| (1), then call either
> CreateSession or LoadSession based on (2).

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:148: // If |web_session_id| is
null, then use the single reference id.
On 2014/05/23 06:01:23, xhwang wrote:
> Can you elaborate more in which case this could happen?

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.h:119: std::set<std::string>
active_sessions_;
On 2014/05/23 06:01:23, xhwang wrote:
> comment that this includes both persistent and non-persistent sessions.

Updated to a single hash_map.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/media/...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:147:
OnSessionError(exception_code, system_code, error_message);
On 2014/05/23 06:01:23, xhwang wrote:
> It seems SessionFoo() just calls OnSessionFoo(). Why do we need this extra
layer
> of indirection?

Once blink starts passing in promises, it will change. However, changed to call
the OnSessionxx methods directly.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:30: using
media::Decryptor;
On 2014/05/23 06:01:23, xhwang wrote:
> nit: using media::*Promise and media::MediaKeys here?

Looks like it's done sometimes and not others. Changed.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:646: promise->resolve();
On 2014/05/23 06:01:23, xhwang wrote:
> Is the CdmWrapper making sure the |promise_id| returned is valid? Otherwise, a
> CDM bug (e.g. sending an invalid promise ID) may cause a crash in the render
> process.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:674:
error_description->value());
On 2014/05/23 06:01:23, xhwang wrote:
> nit: return early here so that you don't need the "else"

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1208: return
result.Pass();
On 2014/05/23 06:01:23, xhwang wrote:
> There are some duplication here:
> 1, 2 maps instead of 1 and then we have two sets of SavePromise and
TakePromise.
> 2, The reject() calls of the two Promise types are actually identical.
> 
> I wonder does it make sense to have a base CdmPromiseBase class, which
provides
> the reject() method (since all promises share the same reject()). Then we have
> CdmPromise<T> inherits CdmPromiseBase. Then in these conversion points (e.g.
> here and ProxyMediaKeys), we only keep one map that stores CdmPromiseBase. In
> the rejection case, we only need to call reject() on CdmPromiseBase. In the
> resolve case, we need to static_cast CdmPromiseBase into a specific
CdmPromise.
> But since for the resolve case, we always know the Promise type, this isn't an
> issue.
> 
> With that, we'll only have one map and we can remove a lot of duplicate code.
> 
> More about naming:
> 
> If the above idea makes sense to you. Maybe the base class could be
CdmPromise.
> The template class could be CdmPromiseTemplate. Since CdmPromiseTemplate will
be
> typedef'ed, so we don't really care about it's name.

Done.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.h (right):

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:107: PP_Var
error_description_var);
On 2014/05/23 06:01:23, xhwang wrote:
> OOC, is it a convention to use foo_var for PP_VAR type variables?

Nope. Just means I have to come up with better names in the methods. Fixed.

https://codereview.chromium.org/265993002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.h:243: std::map<uint32_t,
media::NewSessionCdmPromise*> new_session_promises_;
On 2014/05/23 06:01:23, xhwang wrote:
> Use ScopedPtrHashMap.

Done.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:20: PromiseRejectedCB;
On 2014/05/22 16:44:14, xhwang wrote:
> This seems to be only used in class CdmPromise. How about moving the typedef
> into class CdmPromise?

Done.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:27: class MEDIA_EXPORT CdmPromise {
On 2014/05/23 06:01:23, xhwang wrote:
> See above for the idea of having a base class for all CDM Promises.

Done.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:46: PromiseRejectedCB rejected_cb_;
On 2014/05/22 16:44:14, xhwang wrote:
> we have |resolve_cb_| but |reject"ed"_cb_|. Shall we be consistent?  e.g.
> resolve_cb & reject_cb?

Done.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:47: bool is_pending_;
On 2014/05/22 16:44:14, xhwang wrote:
> Add comment for |is_pending_|.

Done.

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:65: base::Callback<void(void)> resolve_cb_;
On 2014/05/22 16:44:14, xhwang wrote:
> Actually base::Callback<void(void)> == base::Closure :)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=766

I put it this way so that it maps to the basic template (void(const T&) becomes
void(void)).

https://codereview.chromium.org/265993002/diff/160001/media/base/cdm_promise....
media/base/cdm_promise.h:65: base::Callback<void(void)> resolve_cb_;
On 2014/05/22 16:44:14, xhwang wrote:
> How about having a template class 
> 
> template <typename T> struct PromiseResolveCB {
>   base::Callback<void(const T&)> cb;
> };
> 
> template <> struct PromiseResolveCB<void> {
>   base::Callback<void(void)> cb;
> };
> 
> Then you can use PromiseResolveCB<T> resulve_cb_ in CdmPromise and you don't
> have to specialize the full class, which will save a lot of duplicate code in
> this class.
> 
> The downside of this is that you'll need to do something like
> resolve_cb_.cb.Run()
> But this seems fine to me since it's implementation detail of this class.
WDYT?

With the refactoring to CdmPromiseTemplate, not much overriding is required.

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:35: enum KeyError {
On 2014/05/23 16:41:09, ddorwin wrote:
> This is no longer used in this header, so it probably doesn't belong here. Do
we
> now only need it in ProxyDecryptor?

And in android/proxy_media_keys (which might get fixed when I get to the recent
Android changes). Added a TODO as a reminder.

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:64: EXCEPTION_OUTPUT_ERROR
On 2014/05/23 06:01:23, xhwang wrote:
> Hmm, DOM "exceptions" are all "error"s... Generally enums should start with
the
> enum name (in this case EXCEPTION). But "EXCEPTION" and "ERROR" seems
redundant.
> I'am fine with dropping the "EXCEPTION" prefix... WDYT? +ddorwin for comments.

Done.

https://codereview.chromium.org/265993002/diff/160001/media/base/media_keys.h...
media/base/media_keys.h:64: EXCEPTION_OUTPUT_ERROR
On 2014/05/23 16:41:09, ddorwin wrote:
> On 2014/05/23 06:01:23, xhwang wrote:
> > Hmm, DOM "exceptions" are all "error"s... Generally enums should start with
> the
> > enum name (in this case EXCEPTION). But "EXCEPTION" and "ERROR" seems
> redundant.
> > I'am fine with dropping the "EXCEPTION" prefix... WDYT? +ddorwin for
comments.
> 
> I don't think there is a general *requirement* for prefixes. (The Content API
> and other areas may impose one.) I think it's fine to drop EXCEPTION,
especially
> since ...ERROR is consistent with the DOM error names.
> 
> We should try to be consistent in the enum below too. WDYT?
> Maybe TEMPORARY_SESSION?

Done.

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.cc:257: EXCEPTION_NOT_SUPPORTED_ERROR, 0, "LoadSession()
is not implemented.");
On 2014/05/23 06:01:23, xhwang wrote:
> I don't think AesDecryptor will ever support LoadSession.
> s/implemented/supported.

Done.

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor.h
File media/cdm/aes_decryptor.h (right):

https://codereview.chromium.org/265993002/diff/160001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.h:30: AesDecryptor(const SessionMessageCB&
session_message_cb);
On 2014/05/23 06:01:23, xhwang wrote:
> explicit

Done.

[xhwang, 2014-05-29 23:44:37]

This is the first time I read the whole CL. But I confess, for some files (e.g.
ppapi files) I only skimmed the changes.

The CL looks good in general. I think we should fix the Android part and try to
commit it asap. Otherwise you'll have to rebase it again :(

Thanks for the hard work!

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:76: const uint32 session_id =
CreateSessionId();
nit: there's nothing wrong with this but typically we don't use "const" for
cases like this.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:226: return
next_session_id_++;
With "next_session_id_++", this function can't be const.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:274: return
session_id_to_promise_map_.take_and_erase(it).Pass();
nit: .Pass() is not necessary (maybe :) try it!)

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:98: void
AssignWebSessionId(const uint32_t session_id,
no need to use "const" for a integer that's passed by value.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:101: std::string
LookupWebSessionId(const uint32_t session_id) const;
ditto

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:106: void SavePromise(const
uint32_t session_id,
ditto

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:34: static void
DoNothingOnError(media::MediaKeys::Exception exception_code,
s/Error/Reject to be consistent?

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:48:
base::Bind(&DoNothingOnError)),
hmm, this is still hacky. 

How about drop DISALLOW_COPY_AND_ASSIGN in CdmPromiseTemplate and make it
copiable? Then you can simply do media::SimpleCdmPromise(caller_promise) and
drop caller_promise_.

Or you could make SimpleCdmPromise an interface. It's a bit overkill, but seems
the right thing to do.

+ddorwin for comments.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:175:
scoped_ptr<SessionUpdatedPromise> update_promise(
s/update_promise/session_updated_promise?

https://codereview.chromium.org/265993002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1168: uint32_t promise_id
= ++next_promise_id_;
When we use next_promise_id_, we usually do next_promise_id_++ so that the
next_promise_id_ is really the next promise id to be assigned. Also, to do that,
the initialize value is 1 instead of 0 (assuming 0 is invalid).

https://codereview.chromium.org/265993002/diff/200001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1174: uint32_t promise_id)
{
nit: does this fit in one line now?

https://codereview.chromium.org/265993002/diff/200001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/265993002/diff/200001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.h:102: virtual void SetTimer(int64_t delay_ms, void*
context) OVERRIDE;
These are shared by HOST4 and HOST5?

https://codereview.chromium.org/265993002/diff/200001/media/filters/pipeline_...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/265993002/diff/200001/media/filters/pipeline_...
media/filters/pipeline_integration_test.cc:223: &KeyProvidingApp::OnReject,
base::Unretained(this), expected)));
Promise created code are always lengthy, probably we could have some helper
function/macro to simplify them. But we can do that in another CL.

https://codereview.chromium.org/265993002/diff/200001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/265993002/diff/200001/media/media.gyp#newcode262
media/media.gyp:262: 'base/cdm_promise.h',
order

[jrummell, 2014-05-30 18:04:23]

Updated. Due to recent changes in the Android specific code, I had to change
MediaDrmBridge to no longer inherit from MediaKeys, but it still implements the
old MediaKeys interface. A future CL will cleanup the Android side to pass
promises across IPC.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:76: const uint32 session_id =
CreateSessionId();
On 2014/05/29 23:44:38, xhwang wrote:
> nit: there's nothing wrong with this but typically we don't use "const" for
> cases like this.

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:226: return
next_session_id_++;
On 2014/05/29 23:44:38, xhwang wrote:
> With "next_session_id_++", this function can't be const.

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:274: return
session_id_to_promise_map_.take_and_erase(it).Pass();
On 2014/05/29 23:44:38, xhwang wrote:
> nit: .Pass() is not necessary (maybe :) try it!)

I tried it and it works. However, looks like I didn't update this branch. (Same
pattern is in content_decryptor_delegate.cc, so both will get updated in next
PS).

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:98: void
AssignWebSessionId(const uint32_t session_id,
On 2014/05/29 23:44:38, xhwang wrote:
> no need to use "const" for a integer that's passed by value.

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:101: std::string
LookupWebSessionId(const uint32_t session_id) const;
On 2014/05/29 23:44:38, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:106: void SavePromise(const
uint32_t session_id,
On 2014/05/29 23:44:38, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:34: static void
DoNothingOnError(media::MediaKeys::Exception exception_code,
On 2014/05/29 23:44:38, xhwang wrote:
> s/Error/Reject to be consistent?

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:48:
base::Bind(&DoNothingOnError)),
On 2014/05/29 23:44:38, xhwang wrote:
> hmm, this is still hacky. 
> 
> How about drop DISALLOW_COPY_AND_ASSIGN in CdmPromiseTemplate and make it
> copiable? Then you can simply do media::SimpleCdmPromise(caller_promise) and
> drop caller_promise_.
> 
> Or you could make SimpleCdmPromise an interface. It's a bit overkill, but
seems
> the right thing to do.
> 
> +ddorwin for comments.

Added default constructor on SimpleCdmPromise, so it looks a little bit cleaner.
WDYT?

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:175:
scoped_ptr<SessionUpdatedPromise> update_promise(
On 2014/05/29 23:44:38, xhwang wrote:
> s/update_promise/session_updated_promise? 

Done.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1168: uint32_t promise_id
= ++next_promise_id_;
On 2014/05/29 23:44:38, xhwang wrote:
> When we use next_promise_id_, we usually do next_promise_id_++ so that the
> next_promise_id_ is really the next promise id to be assigned. Also, to do
that,
> the initialize value is 1 instead of 0 (assuming 0 is invalid).

Done. 0 is not invalid, but it is probably best to not use it just in case there
is a callback that uses 0 as a default.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/pepper...
content/renderer/pepper/content_decryptor_delegate.cc:1174: uint32_t promise_id)
{
On 2014/05/29 23:44:38, xhwang wrote:
> nit: does this fit in one line now?

Nope. 3 characters too long.

https://codereview.chromium.org/265993002/diff/200001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/265993002/diff/200001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.h:102: virtual void SetTimer(int64_t delay_ms, void*
context) OVERRIDE;
On 2014/05/29 23:44:38, xhwang wrote:
> These are shared by HOST4 and HOST5?

Correct. I did it this way so that removing support for CDM_4 simply removes the
section above. Rearranged the methods.

https://codereview.chromium.org/265993002/diff/200001/media/filters/pipeline_...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/265993002/diff/200001/media/filters/pipeline_...
media/filters/pipeline_integration_test.cc:223: &KeyProvidingApp::OnReject,
base::Unretained(this), expected)));
On 2014/05/29 23:44:38, xhwang wrote:
> Promise created code are always lengthy, probably we could have some helper
> function/macro to simplify them. But we can do that in another CL.

ddorwin didn't like macros.

https://codereview.chromium.org/265993002/diff/200001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/265993002/diff/200001/media/media.gyp#newcode262
media/media.gyp:262: 'base/cdm_promise.h',
On 2014/05/29 23:44:38, xhwang wrote:
> order

Done.

[ddorwin, 2014-05-30 18:43:54]

https://codereview.chromium.org/265993002/diff/220001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/265993002/diff/220001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:31: class
MediaDrmBridge;
This basically reverts a previous CL. :(
We might need to just wait for https://codereview.chromium.org/308073004/.

[xhwang, 2014-06-02 21:59:20]

Just a few more comments.

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/265993002/diff/200001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:48:
base::Bind(&DoNothingOnError)),
On 2014/05/30 18:04:25, jrummell wrote:
> On 2014/05/29 23:44:38, xhwang wrote:
> > hmm, this is still hacky. 
> > 
> > How about drop DISALLOW_COPY_AND_ASSIGN in CdmPromiseTemplate and make it
> > copiable? Then you can simply do media::SimpleCdmPromise(caller_promise) and
> > drop caller_promise_.
> > 
> > Or you could make SimpleCdmPromise an interface. It's a bit overkill, but
> seems
> > the right thing to do.
> > 
> > +ddorwin for comments.
> 
> Added default constructor on SimpleCdmPromise, so it looks a little bit
cleaner.
> WDYT?

I like this much better :)

https://codereview.chromium.org/265993002/diff/220001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/265993002/diff/220001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:31: class
MediaDrmBridge;
On 2014/05/30 18:43:56, ddorwin wrote:
> This basically reverts a previous CL. :(
> We might need to just wait for https://codereview.chromium.org/308073004/.

Yeah, this will be BrowserCdm, which can be not a subclass of MediaKeys.

https://codereview.chromium.org/265993002/diff/220001/media/base/android/cdm_...
File media/base/android/cdm_factory_android.cc (right):

https://codereview.chromium.org/265993002/diff/220001/media/base/android/cdm_...
media/base/android/cdm_factory_android.cc:21: const
MediaDrmBridge::SessionErrorCB& session_error_cb) {
As mentioned in my CL:
https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...

It might worth putting all the exceptions and callbacks in a common file (e.g.
media/base/cdm_common.h) so that they can be shared by MediaKeys and BrowserCdm.

https://codereview.chromium.org/265993002/diff/220001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/220001/media/base/cdm_promise....
media/base/cdm_promise.h:76: CdmPromiseTemplate();
hmm, how does this work? The copy constructor is still private? Shouldn't we
make the copy constructor protected? 

Also, add a comment why we have a protected copy ctor.

[xhwang, 2014-06-02 23:48:00]

lgtm % comments! Thanks!

https://codereview.chromium.org/265993002/diff/220001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/220001/media/base/cdm_promise....
media/base/cdm_promise.h:76: CdmPromiseTemplate();
On 2014/06/02 21:59:21, xhwang wrote:
> hmm, how does this work? The copy constructor is still private? Shouldn't we
> make the copy constructor protected? 
> 
> Also, add a comment why we have a protected copy ctor.

Summary of offline discussion:

We need a default ctor because the SessionUpdatedPromise needs to inherit this
class but doesn't need any of it's implementation. The copy constructor (trying
to reuse the base's implementation) doesn't work well because the Promise that's
copied also needs to be fulfilled. We can set it's is_pending_ to be false when
we copy. But that also seems hacky. The root cause is that we are mixing the
interface and implementation. 

The "right" solution would be to create pure interfaces (e.g. CdmPromise and
CdmPromiseTemplate are pure interfaces), then have concrete implementations. But
that involves a lot of extra code only for this one application :( So I am okay
with the current approach, which is much simpler.

Please add some comments about why we need a default constructor. Thanks!

[jrummell, 2014-06-03 18:55:23]

This is mostly a rebase on the latest Android changes.

https://codereview.chromium.org/265993002/diff/220001/media/base/android/cdm_...
File media/base/android/cdm_factory_android.cc (right):

https://codereview.chromium.org/265993002/diff/220001/media/base/android/cdm_...
media/base/android/cdm_factory_android.cc:21: const
MediaDrmBridge::SessionErrorCB& session_error_cb) {
On 2014/06/02 21:59:21, xhwang wrote:
> As mentioned in my CL:
>
https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
> 
> It might worth putting all the exceptions and callbacks in a common file (e.g.
> media/base/cdm_common.h) so that they can be shared by MediaKeys and
BrowserCdm.

This should get cleaned up in a later CL when Android supports promises. Moved
the CB definitions to BrowserCdm and added a comment there.

https://codereview.chromium.org/265993002/diff/220001/media/base/cdm_promise.h
File media/base/cdm_promise.h (right):

https://codereview.chromium.org/265993002/diff/220001/media/base/cdm_promise....
media/base/cdm_promise.h:76: CdmPromiseTemplate();
On 2014/06/02 23:48:01, xhwang wrote:
> On 2014/06/02 21:59:21, xhwang wrote:
> > hmm, how does this work? The copy constructor is still private? Shouldn't we
> > make the copy constructor protected? 
> > 
> > Also, add a comment why we have a protected copy ctor.
> 
> Summary of offline discussion:
> 
> We need a default ctor because the SessionUpdatedPromise needs to inherit this
> class but doesn't need any of it's implementation. The copy constructor
(trying
> to reuse the base's implementation) doesn't work well because the Promise
that's
> copied also needs to be fulfilled. We can set it's is_pending_ to be false
when
> we copy. But that also seems hacky. The root cause is that we are mixing the
> interface and implementation. 
> 
> The "right" solution would be to create pure interfaces (e.g. CdmPromise and
> CdmPromiseTemplate are pure interfaces), then have concrete implementations.
But
> that involves a lot of extra code only for this one application :( So I am
okay
> with the current approach, which is much simpler.
> 
> Please add some comments about why we need a default constructor. Thanks!

Comment added.

[jrummell, 2014-06-03 20:12:13]

+palmer@ for security review of ppapi/proxy/ppapi_messages.h
+dmichael@ for ppapi stuff in ppapi/* and content/renderer/pepper/*

[jrummell, 2014-06-03 20:14:29]

+jar@ for histograms.xml

[jar (doing other things), 2014-06-03 22:43:16]

For histograms.xml.... leaving data intact allows folks to read old histograms
etc..  At most, for unused histograms, we mark them as obsolete, but for an
enum, there seems no value in removing it (assuming it not a typo, and is/was
applicable).

Adding asvitkine, as he may be a more responsive reviewer for hisotgrams.

https://codereview.chromium.org/265993002/diff/240001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/265993002/diff/240001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40604: -  <int value="2098849894"
label="PPB_ContentDecryptor_Private;0.10"/>
nit: unless this is a typo... it would probably be better to leave it intact.

[dmichael (off chromium), 2014-06-03 23:15:18]

I haven't gotten too far yet in the pepper & ppapi stuff. Can you explain really
briefly why the pepper APIs have to change at all? I would have guessed that the
Promise stuff would simply be API/syntactic sugar for JavaScript, and that a CDM
wouldn't have to worry about doing anything special.

https://codereview.chromium.org/265993002/diff/240001/ppapi/cpp/private/conte...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/265993002/diff/240001/ppapi/cpp/private/conte...
ppapi/cpp/private/content_decryptor_private.cc:92: pp::Var
web_session_id_var(pp::PASS_REF, web_session_id_arg);
I'm pretty sure you should use the normal, non-PASS_REF constructor here (And in
the pre-existing line on 96). It probably has been relatively harmless, because
it just means that the Var is already released when control gets back to
Chromium, and presumably Chromium releases  the var just after calling this
function. That second release will be for a PP_Var id that's now gone, and I
think it just generates a debug log message.

[jrummell, 2014-06-04 00:26:31]

Updated.

As to the changes to Pepper, previously the EME used events to indicate the
result of each action. So if the JS called createSession() 3 times, it would
expect to get back 3 events, in some order. Now that it is using promises (not
yet implemented in Blink, coming soon), we need the response to come back to the
correct promise so it can be resolved properly (or the error matched to the
correct call).

As well, the previous implementation passed down a session reference ID (int)
rather than the session ID (string) that the CDM refers to. This meant the CDM
had to keep a mapping between the two ids, which this avoids.

Finally, since the EME spec is still evolving, there have been some parameter
changes that need to be passed to the CDM.

https://codereview.chromium.org/265993002/diff/240001/ppapi/cpp/private/conte...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/265993002/diff/240001/ppapi/cpp/private/conte...
ppapi/cpp/private/content_decryptor_private.cc:92: pp::Var
web_session_id_var(pp::PASS_REF, web_session_id_arg);
On 2014/06/03 23:15:17, dmichael wrote:
> I'm pretty sure you should use the normal, non-PASS_REF constructor here (And
in
> the pre-existing line on 96). It probably has been relatively harmless,
because
> it just means that the Var is already released when control gets back to
> Chromium, and presumably Chromium releases  the var just after calling this
> function. That second release will be for a PP_Var id that's now gone, and I
> think it just generates a debug log message.

I just copied what was done for response_var. Changed.

https://codereview.chromium.org/265993002/diff/240001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/265993002/diff/240001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40604: -  <int value="2098849894"
label="PPB_ContentDecryptor_Private;0.10"/>
On 2014/06/03 22:43:16, jar wrote:
> nit: unless this is a typo... it would probably be better to leave it intact.

Added back.

[Alexei Svitkine (slow), 2014-06-04 16:57:40]

histograms lgtm

[dmichael (off chromium), 2014-06-05 18:55:30]

pepper stuff lgtm.

In the future, please try to make smaller, incremental CLs. This is really too
big.

https://codereview.chromium.org/265993002/diff/260001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/265993002/diff/260001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:732: }
You have 4 methods that look just like this...  it doesn't seem to buy you
anything over having 1, like the old OnSessionError. Are you planning on taking
different behavior here at some point?

https://codereview.chromium.org/265993002/diff/260001/ppapi/api/private/pp_co...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/265993002/diff/260001/ppapi/api/private/pp_co...
ppapi/api/private/pp_content_decryptor.idl:416: enum PP_ExceptionCode {
Please name these PP_DecryptorFoo or something to make them a little more
specific. The names here are generic enough it's not impossible that we want to
use them someplace in our public APIs. Same for the enum literals (e.g.,
PP_DECRYPTOR...)

[jrummell, 2014-06-05 20:59:54]

Updated.

+dcheng@ for security review of ppapi/proxy/ppapi_messages.h

https://codereview.chromium.org/265993002/diff/260001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/265993002/diff/260001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:732: }
On 2014/06/05 18:55:30, dmichael wrote:
> You have 4 methods that look just like this...  it doesn't seem to buy you
> anything over having 1, like the old OnSessionError. Are you planning on
taking
> different behavior here at some point?

Not needed anymore. Combined into 1 method.

https://codereview.chromium.org/265993002/diff/260001/ppapi/api/private/pp_co...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/265993002/diff/260001/ppapi/api/private/pp_co...
ppapi/api/private/pp_content_decryptor.idl:416: enum PP_ExceptionCode {
On 2014/06/05 18:55:30, dmichael wrote:
> Please name these PP_DecryptorFoo or something to make them a little more
> specific. The names here are generic enough it's not impossible that we want
to
> use them someplace in our public APIs. Same for the enum literals (e.g.,
> PP_DECRYPTOR...)

Rename PP_CdmExceptionCode.

[dcheng, 2014-06-05 21:18:08]

Blink gardening. I will attempt to get to this review on a best effort basis =(

[jrummell, 2014-06-06 22:49:20]

+inferno@ for security review of ppapi/proxy/ppapi_messages.h

[Tom Sepez, 2014-06-06 23:20:01]

Messages LGTM.

[jrummell, 2014-06-07 00:22:24]

Thanks for the reviews.

[jrummell, 2014-06-07 00:22:33]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-06-07 00:24:30]

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

[commit-bot: I haz the power, 2014-06-07 08:54:33]

Change committed as 275656