Update ClearKey to support CDM_6

Update ClearKey to support CDM_6

This change adds support for GetUsableKeyIds() and RemoveSession(),
and renames ReleaseSession() to CloseSession(). There are also
changes due to the Ready event no longer being generated by the CDM
interface.

BUG=358271, 351139
TEST=encrypted-media layout tests and browser_tests for encrypted media pass

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

[jrummell, 2014-08-06 23:14:07]

PTAL. This needs to follow https://codereview.chromium.org/446693004/.

[jrummell, 2014-08-08 17:17:06]

Update now that Host_6 is checked in.

[ddorwin, 2014-08-08 23:36:26]

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:58: // This class is needed so
that resolving an SessionLoaded() promise triggers
nit: s/an/a/

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:60: // additional callback.
Note that this is only needed until KeysChange is piped through Pepper.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:70: is_pending_ = false;
This is one problem with inheriting implementation. Oh well, it's temporary.

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

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:105: bool loadSession =
is_load_session

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:107: bool persistent =
HasHeader(
is_

is_create_persistent?

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:294:
std::make_pair(web_session_id, persistent || load_session));
it might be nice to use a local variable to describe what we're building/passing

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.h:100: void SetSessionId(bool
persistent,
is_new_persistent_session

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.h:101: bool load_session,
is_loaded_session

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

https://codereview.chromium.org/448893002/diff/20001/media/base/cdm_promise.h...
media/base/cdm_promise.h:62: CdmPromiseTemplate();
Where is this being used?

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

https://codereview.chromium.org/448893002/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:33: class MEDIA_EXPORT MediaKeys {
Why isn't GetUsableKeyIds added to this interface?

It seems odd that we're implementing things that are not yet part of the main
interface. Should we implement that (with stubs) first, or as part of this CL
since it probably doesn't add more than a few lines?

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

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.h:46: void GetUsableKeyIds(const std::string&
web_session_id,
Below Release.
Actually, below all of the public items since it is not apparently part of any
base class.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:234: void
OnResolveWithUsableKeyIds(PromiseResult expected,
_result

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:237: EXPECT_EQ(expected, RESOLVED);
This was confusing at first. Maybe add:
<< "Unexpectedly resolved"

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:246: EXPECT_EQ(expected, REJECTED);
ditto

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:270: PromiseResult expected) {
ditto
Also, keep the order consistent. (Plus it makes sense to have the result first.)

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:316: uint32 expected_count,
ditto on order

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:317: PromiseResult result) {
expected_result?

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:323: for (KeyIdsVector::iterator it =
useable_key_ids_.begin();
Maybe DCHECK(!useable_key_ids_.empty()) to catch test bugs.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:400: KeyIdsVector useable_key_ids_;
// Copy of the vector from the last successful call to
OnResolveWithUsableKeyIds().

Or something like that.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:833: 
Though it should never happen in real life, as part of testing this class, you
could try calling an unknown session ID first.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:276:
host_->OnRejectPromise(promise_id,
Separate CL: This should be resolved with undefined. (Not sure how we'd then
handle that in prefixed or if we need to. I guess we could handle resolved with
undefined/0/null as a failure for prefixed.)

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:342: // RemoveSession only
allowed for the simulated loadable session.
Really, the session must be persistent. How about this?

// Similar comment to the one above.
bool is_persistent_session = std::string(kLoadableWebSessionId) ==
std::string(web_session_id, web_session_id_length);
if (is_persistent_session) {
  resolve...

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:347: std::string
message("Incorrect session id specified for RemoveSession().");
"Not supported for non-persistent sessions."

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:720: // UpdateSession() may
be called to finish loading sessions, so handle
I wonder what this means. Is itan artifact of the prefixed implementation?

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:723:
session_id_for_emulated_loadsession_ = std::string();
Separate CL: Do we need session_id_for_emulated_loadsession_ or can we just use
kLoadableWebSessionId? Maybe this is left over from the reference_id days.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:745:
host_->OnResolveKeyIdsPromise(promise_id, result.get(), key_ids.size());
result.size()

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.h (right):

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.h:62: virtual void
GetUsableKeyIds(uint32_t promise_id,
nit: This should before the above function to match CDM_6.

As an aside, I'm surprised SSC is here in CDM.h since it is a non-session (aka
MediaKeys) method.

[jrummell, 2014-08-11 18:59:04]

Updated.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:58: // This class is needed so
that resolving an SessionLoaded() promise triggers
On 2014/08/08 23:36:22, ddorwin wrote:
> nit: s/an/a/

Done.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:60: // additional callback.
On 2014/08/08 23:36:23, ddorwin wrote:
> Note that this is only needed until KeysChange is piped through Pepper.

Done.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:70: is_pending_ = false;
On 2014/08/08 23:36:22, ddorwin wrote:
> This is one problem with inheriting implementation. Oh well, it's temporary.

Acknowledged.

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

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:105: bool loadSession =
On 2014/08/08 23:36:23, ddorwin wrote:
> is_load_session

Done.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:107: bool persistent =
HasHeader(
On 2014/08/08 23:36:23, ddorwin wrote:
> is_
> 
> is_create_persistent?

Done.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:294:
std::make_pair(web_session_id, persistent || load_session));
On 2014/08/08 23:36:23, ddorwin wrote:
> it might be nice to use a local variable to describe what we're
building/passing

Done.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.h:100: void SetSessionId(bool
persistent,
On 2014/08/08 23:36:23, ddorwin wrote:
> is_new_persistent_session

Done.

https://codereview.chromium.org/448893002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.h:101: bool load_session,
On 2014/08/08 23:36:23, ddorwin wrote:
> is_loaded_session

Done.

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

https://codereview.chromium.org/448893002/diff/20001/media/base/cdm_promise.h...
media/base/cdm_promise.h:62: CdmPromiseTemplate();
On 2014/08/08 23:36:23, ddorwin wrote:
> Where is this being used?

class SessionLoadedPromise in ppapi_decryptor.cc. When SessionLoadedPromise (and
SessionUpdatedPromise) go away, these protected methods won't be needed anymore.

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

https://codereview.chromium.org/448893002/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:33: class MEDIA_EXPORT MediaKeys {
On 2014/08/08 23:36:23, ddorwin wrote:
> Why isn't GetUsableKeyIds added to this interface?
> 
> It seems odd that we're implementing things that are not yet part of the main
> interface. Should we implement that (with stubs) first, or as part of this CL
> since it probably doesn't add more than a few lines?

I didn't do it in this CL since MediaKeys is used in a whole bunch of places
(including Android). It will be part of a future CL.

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

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.h:46: void GetUsableKeyIds(const std::string&
web_session_id,
On 2014/08/08 23:36:23, ddorwin wrote:
> Below Release.
> Actually, below all of the public items since it is not apparently part of any
> base class.

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:234: void
OnResolveWithUsableKeyIds(PromiseResult expected,
On 2014/08/08 23:36:24, ddorwin wrote:
> _result

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:237: EXPECT_EQ(expected, RESOLVED);
On 2014/08/08 23:36:24, ddorwin wrote:
> This was confusing at first. Maybe add:
> << "Unexpectedly resolved"

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:246: EXPECT_EQ(expected, REJECTED);
On 2014/08/08 23:36:24, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:270: PromiseResult expected) {
On 2014/08/08 23:36:24, ddorwin wrote:
> ditto
> Also, keep the order consistent. (Plus it makes sense to have the result
first.)

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:316: uint32 expected_count,
On 2014/08/08 23:36:24, ddorwin wrote:
> ditto on order

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:317: PromiseResult result) {
On 2014/08/08 23:36:24, ddorwin wrote:
> expected_result?

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:323: for (KeyIdsVector::iterator it =
useable_key_ids_.begin();
On 2014/08/08 23:36:24, ddorwin wrote:
> Maybe DCHECK(!useable_key_ids_.empty()) to catch test bugs.

Can't as the test calls it before any keys are added, so it should be empty.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:400: KeyIdsVector useable_key_ids_;
On 2014/08/08 23:36:24, ddorwin wrote:
> // Copy of the vector from the last successful call to
> OnResolveWithUsableKeyIds().
> 
> Or something like that.

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:833: 
On 2014/08/08 23:36:24, ddorwin wrote:
> Though it should never happen in real life, as part of testing this class, you
> could try calling an unknown session ID first.
> 

Added DCHECK for this.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:276:
host_->OnRejectPromise(promise_id,
On 2014/08/08 23:36:25, ddorwin wrote:
> Separate CL: This should be resolved with undefined. (Not sure how we'd then
> handle that in prefixed or if we need to. I guess we could handle resolved
with
> undefined/0/null as a failure for prefixed.)

Added TODO.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:342: // RemoveSession only
allowed for the simulated loadable session.
On 2014/08/08 23:36:25, ddorwin wrote:
> Really, the session must be persistent. How about this?
> 
> // Similar comment to the one above.
> bool is_persistent_session = std::string(kLoadableWebSessionId) ==
> std::string(web_session_id, web_session_id_length);
> if (is_persistent_session) {
>   resolve...

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:347: std::string
message("Incorrect session id specified for RemoveSession().");
On 2014/08/08 23:36:25, ddorwin wrote:
> "Not supported for non-persistent sessions."

Done.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:720: // UpdateSession() may
be called to finish loading sessions, so handle
On 2014/08/08 23:36:25, ddorwin wrote:
> I wonder what this means. Is itan artifact of the prefixed implementation?

It's part of simulating LoadSession().

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:723:
session_id_for_emulated_loadsession_ = std::string();
On 2014/08/08 23:36:25, ddorwin wrote:
> Separate CL: Do we need session_id_for_emulated_loadsession_ or can we just
use
> kLoadableWebSessionId? Maybe this is left over from the reference_id days.

I think this is needed by the simulated LoadSession(). aes_decryptor will create
the session using a number (e.g. "1"), yet we need to return
kLoadableWebSessionId as the session id to the callers. This keeps track of the
mapping.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:745:
host_->OnResolveKeyIdsPromise(promise_id, result.get(), key_ids.size());
On 2014/08/08 23:36:25, ddorwin wrote:
> result.size()

Not done. No operators on scoped_ptr<T[]> return the array size. .get() simply
returns T*, so arraysize() doesn't work.

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.h (right):

https://codereview.chromium.org/448893002/diff/20001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.h:62: virtual void
GetUsableKeyIds(uint32_t promise_id,
On 2014/08/08 23:36:25, ddorwin wrote:
> nit: This should before the above function to match CDM_6.
> 
> As an aside, I'm surprised SSC is here in CDM.h since it is a non-session (aka
> MediaKeys) method.

Done.

[ddorwin, 2014-08-11 20:34:16]

Thanks. LG. A few minor comments.

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

https://codereview.chromium.org/448893002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:294: bool persistent =
is_new_persistent_session || is_loaded_session;
is_

https://codereview.chromium.org/448893002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/448893002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.h:100: void SetSessionId(bool
is_new_persistent_session,
Since these can never both be true, we should probably have a single enum.

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

https://codereview.chromium.org/448893002/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:62: CdmPromiseTemplate();
On 2014/08/11 18:59:03, jrummell wrote:
> On 2014/08/08 23:36:23, ddorwin wrote:
> > Where is this being used?
> 
> class SessionLoadedPromise in ppapi_decryptor.cc. When SessionLoadedPromise
(and
> SessionUpdatedPromise) go away, these protected methods won't be needed
anymore.

Add a TODO to remove it so we don't forget.

https://codereview.chromium.org/448893002/diff/40001/media/cdm/ppapi/external...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/448893002/diff/40001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:726:
session_id_for_emulated_loadsession_ = std::string();
On 2014/08/11 18:59:03, jrummell wrote:
> On 2014/08/08 23:36:25, ddorwin wrote:
> > Separate CL: Do we need session_id_for_emulated_loadsession_ or can we just
> use
> > kLoadableWebSessionId? Maybe this is left over from the reference_id days.
> 
> I think this is needed by the simulated LoadSession(). aes_decryptor will
create
> the session using a number (e.g. "1"), yet we need to return
> kLoadableWebSessionId as the session id to the callers. This keeps track of
the
> mapping.

Thanks. Will you please add a comment that explains this (unless I missed it).

[jrummell, 2014-08-13 21:18:06]

Updated.

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

https://codereview.chromium.org/448893002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:294: bool persistent =
is_new_persistent_session || is_loaded_session;
On 2014/08/11 20:34:15, ddorwin wrote:
> is_

Done.

https://codereview.chromium.org/448893002/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/448893002/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.h:100: void SetSessionId(bool
is_new_persistent_session,
On 2014/08/11 20:34:16, ddorwin wrote:
> Since these can never both be true, we should probably have a single enum.

Done.

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

https://codereview.chromium.org/448893002/diff/40001/media/base/cdm_promise.h...
media/base/cdm_promise.h:62: CdmPromiseTemplate();
On 2014/08/11 20:34:16, ddorwin wrote:
> On 2014/08/11 18:59:03, jrummell wrote:
> > On 2014/08/08 23:36:23, ddorwin wrote:
> > > Where is this being used?
> > 
> > class SessionLoadedPromise in ppapi_decryptor.cc. When SessionLoadedPromise
> (and
> > SessionUpdatedPromise) go away, these protected methods won't be needed
> anymore.
> 
> Add a TODO to remove it so we don't forget.

Done.

https://codereview.chromium.org/448893002/diff/40001/media/cdm/ppapi/external...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/448893002/diff/40001/media/cdm/ppapi/external...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:726:
session_id_for_emulated_loadsession_ = std::string();
On 2014/08/11 20:34:16, ddorwin wrote:
> On 2014/08/11 18:59:03, jrummell wrote:
> > On 2014/08/08 23:36:25, ddorwin wrote:
> > > Separate CL: Do we need session_id_for_emulated_loadsession_ or can we
just
> > use
> > > kLoadableWebSessionId? Maybe this is left over from the reference_id days.
> > 
> > I think this is needed by the simulated LoadSession(). aes_decryptor will
> create
> > the session using a number (e.g. "1"), yet we need to return
> > kLoadableWebSessionId as the session id to the callers. This keeps track of
> the
> > mapping.
> 
> Thanks. Will you please add a comment that explains this (unless I missed it).

Added in header file.

[ddorwin, 2014-08-13 21:35:52]

lgtm % comment nit.

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

https://codereview.chromium.org/448893002/diff/60001/media/base/cdm_promise.h...
media/base/cdm_promise.h:62: // TODO(jrummell): Remove when derived classes are
no longer needed.
Which derived classes? Be specific so it's easy to tell when we can remove this
code.

[jrummell, 2014-08-13 21:44:08]

Updated.

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

https://codereview.chromium.org/448893002/diff/60001/media/base/cdm_promise.h...
media/base/cdm_promise.h:62: // TODO(jrummell): Remove when derived classes are
no longer needed.
On 2014/08/13 21:35:52, ddorwin wrote:
> Which derived classes? Be specific so it's easy to tell when we can remove
this
> code.

Done.

[jrummell, 2014-08-13 21:44:31]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-13 21:47:56]

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

[commit-bot: I haz the power, 2014-08-14 03:50:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-14 06:45:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 06:45:41]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2014-08-14 17:41:47]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:46:30]

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

[commit-bot: I haz the power, 2014-08-14 17:51:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-14 17:55:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:55:33]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2014-08-14 19:56:50]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-08-14 19:58:54]

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

[commit-bot: I haz the power, 2014-08-15 03:06:00]

Committed patchset #6 (100001) as 289755