media: Support unprefixed EME API on Android.

media: Support unprefixed EME API on Android.

Detailed changes:
1. Supports latest MediaKeys API in ProxyMediaKeys, RendererCdmManager, IPC,
   BrowserCdmManager, BrowserCdm and MediaDrmBridge (C++ and Java), including:
   - Support new API: CreateSessionAndGenerateRequest, CloseSession, etc.
   - Support new events: SessionKeysChange and SessionExpirationUpdate.
   - Remove old events: SessionCreated and SessionReady.
   - Remove old integer session ID.
   - Fully support promises.
2. BrowserCdm inherits MediaKeys. As a result, MediaDrmBridge is also a
   MediaKeys. This will make it easy to use MediaDrmBridge in other places, e.g.
   in MojoCdmService.
3. MediaDrmBridge Java class refactored to accomodate these changes.

Notes:
1. MeidaDrm doesn't support key status. A dummy key ID is used to report key
   status change.
2. MediaDrm doesn't support expiration time.
3. Persistent session is not supported; ProxyMediaKeys rejects LoadSession() and
   RemoveSession() right away.
4. Promises are converted to promise ID in ProxyMediaKeys and MediaDrmBridge so
   that we can pass it over IPC and JNI.

BUG=442584
TEST=Prefixed API test pages work. Need to test unprefixed API.

Committed: https://crrev.com/d7e021d86cd234f8eb2d48e99f82042b92b1b5c2
Cr-Commit-Position: refs/heads/master@{#312348}

[xhwang, 2015-01-14 21:28:44]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, qinmin@chromium.org

[xhwang, 2015-01-14 21:28:45]

This CL needs a rebase (on jrummell's recent CLs) and I need a better way to
test the new unprefixed API (browsertests don't work on Android and layout tests
don't exercise brwoser side logic).

I did tested existing test pages which use prefixed API and they work fine.

Sorry this CL is bigger than I expected; since the old code is many generations
older than the rest of the EME stack, it's hard to split this CL into smaller
ones.

ddorwin: PTAL at everything.
qimin: PTAL at the MediaDrmBridge.java change.
jrummell: FYI

[xhwang, 2015-01-14 21:30:57]

This CL needs a rebase (on jrummell's recent CLs) and I need a better way to
test the new unprefixed API (browsertests don't work on Android and layout tests
don't exercise brwoser side logic).

I did tested existing test pages which use prefixed API and they work fine.

Sorry this CL is bigger than I expected; since the old code is many generations
older than the rest of the EME stack, it's hard to split this CL into smaller
ones.

ddorwin: PTAL at everything.
qimin: PTAL at the MediaDrmBridge.java change.
jrummell: FYI

[ddorwin, 2015-01-14 23:09:19]

ddorwin@chromium.org changed reviewers:
+ jrummell@chromium.org

[ddorwin, 2015-01-14 23:09:21]

I skimmed it, but didn't look at the details. jrummell@ should do the full
review.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:306: void
BrowserCdmManager::OnCreateSessionAndGenerateRequest(
The session type should be passed. (We can DCHECK for now.)

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:472: void
BrowserCdmManager::RequestSessionPermission(
"SessionPermission" is probably wrong, but I guess Dan can clean that up.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:503: void
BrowserCdmManager::CreateSessionIfPermitted(
GenerateRequestIfPermitted?

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:522: // Only temporary session
is supported in browser CDM path. Add SessionType
Then you won't need this comment. :)

https://codereview.chromium.org/850183002/diff/1/content/common/media/cdm_mes...
File content/common/media/cdm_messages_enums.h (right):

https://codereview.chromium.org/850183002/diff/1/content/common/media/cdm_mes...
content/common/media/cdm_messages_enums.h:8: // Dictates the init data type when
an EME session is created.
nit: Initialization Data Type
nit: It's when a request is generated, not when the session is created.

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

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:54: SessionType session_type,
This param is ignored. Should be checked for support (DCHECK once 384152 is
fixed) and passed down (we shouldn't prevent support at the API level).

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:63:
create_session_init_data_type = CREATE_SESSION_TYPE_MP4;
We need to fix the "...SESSION_TYPE.." naming at some point as it is confusing.

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:87: // blink and add
NOTREACHED() here.
This at least depends on 384152.

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:112:
promise->reject(NOT_SUPPORTED_ERROR, 0, "RemoveSession() not supported.");
Similar TODO here?

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

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.h:73: void OnSessionError(const
std::string& session_id,
_If_ you want, you can start renaming this to legacy/prefixed since this is
entirely local to this class.

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

https://codereview.chromium.org/850183002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:64: OUTPUT_ERROR,
The name "exception" and inclusion of "output" is odd. I guess this is used for
legacy key error? For unprefixed, output is not an exception (not used when
rejecting a promise).

https://codereview.chromium.org/850183002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:65: EXCEPTION_MAX = OUTPUT_ERROR
This is painful to maintain. :(
We had a conversation about "MAX" for UMAs, but I don't know what the outcome
was for other enums.

[xhwang, 2015-01-15 08:13:13]

jrummell: Please review everything.
qinmin: Please review MediaDrmBridge.java
ddorwin: I addressed most of your comments. Please see the changes and my reply.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:306: void
BrowserCdmManager::OnCreateSessionAndGenerateRequest(
On 2015/01/14 23:09:20, ddorwin wrote:
> The session type should be passed. (We can DCHECK for now.)

Chromium IPC only supports at most 5 parameters. I'd hate to have a struct (e.g.
CreateSessionData) to pass SessionType over IPC. If ChromeCast ever needs
persistent sessions, I can add it later (in a separate CL). Otherwise, I'll keep
it as is, and hopefully this will be replaced by mojo soon.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:472: void
BrowserCdmManager::RequestSessionPermission(
On 2015/01/14 23:09:20, ddorwin wrote:
> "SessionPermission" is probably wrong, but I guess Dan can clean that up.

Acknowledged.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:503: void
BrowserCdmManager::CreateSessionIfPermitted(
On 2015/01/14 23:09:20, ddorwin wrote:
> GenerateRequestIfPermitted?

Done.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:522: // Only temporary session
is supported in browser CDM path. Add SessionType
On 2015/01/14 23:09:20, ddorwin wrote:
> Then you won't need this comment. :)

ditto

https://codereview.chromium.org/850183002/diff/1/content/common/media/cdm_mes...
File content/common/media/cdm_messages_enums.h (right):

https://codereview.chromium.org/850183002/diff/1/content/common/media/cdm_mes...
content/common/media/cdm_messages_enums.h:8: // Dictates the init data type when
an EME session is created.
On 2015/01/14 23:09:20, ddorwin wrote:
> nit: Initialization Data Type
> nit: It's when a request is generated, not when the session is created.

Done.

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

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:54: SessionType session_type,
On 2015/01/14 23:09:20, ddorwin wrote:
> This param is ignored. Should be checked for support (DCHECK once 384152 is
> fixed) and passed down (we shouldn't prevent support at the API level).

Checked. Not passing down due to IPC limitations. See my comment in another
file.

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:63:
create_session_init_data_type = CREATE_SESSION_TYPE_MP4;
On 2015/01/14 23:09:20, ddorwin wrote:
> We need to fix the "...SESSION_TYPE.." naming at some point as it is
confusing.

Agreed.

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:87: // blink and add
NOTREACHED() here.
On 2015/01/14 23:09:20, ddorwin wrote:
> This at least depends on 384152.

Done.

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.cc:112:
promise->reject(NOT_SUPPORTED_ERROR, 0, "RemoveSession() not supported.");
On 2015/01/14 23:09:20, ddorwin wrote:
> Similar TODO here?

Done.

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

https://codereview.chromium.org/850183002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_media_keys.h:73: void OnSessionError(const
std::string& session_id,
On 2015/01/14 23:09:20, ddorwin wrote:
> _If_ you want, you can start renaming this to legacy/prefixed since this is
> entirely local to this class.

Done. I'll rename the rest in a different CL.

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

https://codereview.chromium.org/850183002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:64: OUTPUT_ERROR,
On 2015/01/14 23:09:21, ddorwin wrote:
> The name "exception" and inclusion of "output" is odd. I guess this is used
for
> legacy key error? For unprefixed, output is not an exception (not used when
> rejecting a promise).

Acknowledged.

https://codereview.chromium.org/850183002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:65: EXCEPTION_MAX = OUTPUT_ERROR
On 2015/01/14 23:09:21, ddorwin wrote:
> This is painful to maintain. :(
> We had a conversation about "MAX" for UMAs, but I don't know what the outcome
> was for other enums.

Agreed this is ugly. It seems most enums used in IPC use MAX_EXCEPTION style
instead of NUM_EXCEPTIONS style. Also NUM_EXCEPTIONS introduces a new enum
value. In that sense, MAX_EXCEPTION seems cleaner.

[ddorwin, 2015-01-15 17:36:52]

Thanks. The changes LG, and this seems LG overall.

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/1/content/browser/media/cdm/br...
content/browser/media/cdm/browser_cdm_manager.cc:306: void
BrowserCdmManager::OnCreateSessionAndGenerateRequest(
On 2015/01/15 08:13:13, xhwang wrote:
> On 2015/01/14 23:09:20, ddorwin wrote:
> > The session type should be passed. (We can DCHECK for now.)
> 
> Chromium IPC only supports at most 5 parameters. I'd hate to have a struct
(e.g.
> CreateSessionData) to pass SessionType over IPC. If ChromeCast ever needs
> persistent sessions, I can add it later (in a separate CL). Otherwise, I'll
keep
> it as is, and hopefully this will be replaced by mojo soon.

Acknowledged.

https://codereview.chromium.org/850183002/diff/20001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/20001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:527: // Only temporary session
is supported in browser CDM path. Add SessionType
tiny nit: The first sentence doesn't read well. "Only the temporary session
type..."?
(This same text appears in promise messages later, which is why I even bring it
up.)

Is the second sentence a TODO? It seems it should at least be on a separate
line.

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

https://codereview.chromium.org/850183002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_media_keys.cc:61: "Only temporary session is
supported.");
nit: ditto

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:438: "Only temporary session is
supported.");
ditto

[qinmin, 2015-01-15 18:17:26]

MediaDrmBridge lgtm % comments

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:144: * 
Converts byte array to hex string for logging.
nit: s/Converts/Convert/

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:382: * @param
session Session on which we need to get the key request.
nit:update the javadoc

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:405: */
nit: @param?

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:511: * @param
sessionId Crypto session Id.
nit @return?

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:475: promise->reject(NOT_SUPPORTED_ERROR,
0, "LoadSession() is not supported.");
do we need this if it is NOTREACHED?

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:510: promise->reject(NOT_SUPPORTED_ERROR,
0, "RemoveSession() is not supported.");
ditto

[jrummell, 2015-01-15 18:56:23]

lgtm w/nits

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

https://codereview.chromium.org/850183002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/renderer_cdm_manager.cc:63: const
std::vector<uint8_t>& certificate) {
nit: The other methods below all have DCHECK(GetMediaKeys(cdm_id)), so it should
be added here.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:273: : reinterpret_cast<const
uint8_t*>(&*str.begin());
std::string has const char* data() const; Can you not use it here, since it's
been around for a while (before C++11).

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:400: scoped_ptr<MediaDrmBridge>
MediaDrmBridge::CreateSessionless(
less? Is this just the old code needed a different name since CreateSession()
was already used?

[xhwang, 2015-01-15 19:03:43]

xhwang@chromium.org changed reviewers:
+ gunsch@chromium.org

[xhwang, 2015-01-15 19:03:44]

gunsch: I am changing BrowserCdm interface. Please take a look at that change
from ChromeCast's perspective.

https://codereview.chromium.org/850183002/diff/20001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/20001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:527: // Only temporary session
is supported in browser CDM path. Add SessionType
On 2015/01/15 17:36:52, ddorwin wrote:
> tiny nit: The first sentence doesn't read well. "Only the temporary session
> type..."?
> (This same text appears in promise messages later, which is why I even bring
it
> up.)
> 
> Is the second sentence a TODO? It seems it should at least be on a separate
> line.

Done.

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

https://codereview.chromium.org/850183002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/proxy_media_keys.cc:61: "Only temporary session is
supported.");
On 2015/01/15 17:36:52, ddorwin wrote:
> nit: ditto

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:144: * 
Converts byte array to hex string for logging.
On 2015/01/15 18:17:25, qinmin wrote:
> nit: s/Converts/Convert/

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:382: * @param
session Session on which we need to get the key request.
On 2015/01/15 18:17:25, qinmin wrote:
> nit:update the javadoc

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:405: */
On 2015/01/15 18:17:25, qinmin wrote:
> nit: @param?

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:511: * @param
sessionId Crypto session Id.
On 2015/01/15 18:17:25, qinmin wrote:
> nit @return?

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:438: "Only temporary session is
supported.");
On 2015/01/15 17:36:52, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:475: promise->reject(NOT_SUPPORTED_ERROR,
0, "LoadSession() is not supported.");
On 2015/01/15 18:17:25, qinmin wrote:
> do we need this if it is NOTREACHED?

When we use MediaDrmBridge with mojo, we need to reject the promise. I'll drop
the NOTREACHED() check to be consistent.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:510: promise->reject(NOT_SUPPORTED_ERROR,
0, "RemoveSession() is not supported.");
On 2015/01/15 18:17:25, qinmin wrote:
> ditto

ditto

[xhwang, 2015-01-15 19:37:26]

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

https://codereview.chromium.org/850183002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/renderer_cdm_manager.cc:63: const
std::vector<uint8_t>& certificate) {
On 2015/01/15 18:56:23, jrummell wrote:
> nit: The other methods below all have DCHECK(GetMediaKeys(cdm_id)), so it
should
> be added here.

Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:273: : reinterpret_cast<const
uint8_t*>(&*str.begin());
On 2015/01/15 18:56:23, jrummell wrote:
> std::string has const char* data() const; Can you not use it here, since it's
> been around for a while (before C++11).

Good point. I thought this would avoid a possible data copy in data(), but now I
think it's premature optimization. Done.

https://codereview.chromium.org/850183002/diff/20001/media/base/android/media...
media/base/android/media_drm_bridge.cc:400: scoped_ptr<MediaDrmBridge>
MediaDrmBridge::CreateSessionless(
On 2015/01/15 18:56:23, jrummell wrote:
> less? Is this just the old code needed a different name since CreateSession()
> was already used?

lol, I agree this name is confusing. Changed to CreateWithoutSessionSupport.

[xhwang, 2015-01-15 19:39:10]

xhwang@chromium.org changed reviewers:
+ dcheng@chromium.org

[xhwang, 2015-01-15 19:39:12]

dcheng: Please OWNERS review changes in content/common/media/cdm_message*

There are some inconsistency in the existing code on how params are checked at
the sender/receiver. I'll clean that up in a separate CL.

[gunsch, 2015-01-15 19:48:34]

On 2015/01/15 19:39:12, xhwang wrote:
> dcheng: Please OWNERS review changes in content/common/media/cdm_message*
> 
> There are some inconsistency in the existing code on how params are checked at
> the sender/receiver. I'll clean that up in a separate CL.

Thanks for the heads-up. I don't see any potential issues from Chromecast side,
though this might make it tricky to maintain our internal repo builds against
simultaneous downstream and upstream :)

[xhwang, 2015-01-16 17:10:24]

On 2015/01/15 19:48:34, gunsch wrote:
> On 2015/01/15 19:39:12, xhwang wrote:
> > dcheng: Please OWNERS review changes in content/common/media/cdm_message*
> > 
> > There are some inconsistency in the existing code on how params are checked
at
> > the sender/receiver. I'll clean that up in a separate CL.
> 
> Thanks for the heads-up. I don't see any potential issues from Chromecast
side,
> though this might make it tricky to maintain our internal repo builds against
> simultaneous downstream and upstream :)

dcheng: Kindly ping.....

[dcheng, 2015-01-17 00:08:17]

This patch is large enough that it's difficult for me to do an IPC review.
Things /look/ OK, but even now, I'm still not completely sure.

If it's possible to break these CLs into smaller patches in the future, IPC
reviewers will greatly appreciate it =)

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:197: void
BrowserCdmManager::ResolvePromise<std::string>(
I think I find the new version much harder to read (and it makes it hard for a
reader to resolve which 'overload' ResolvePromise will call). Why not just call
this ResolvePromiseWithSession rather than adding explicit template
specializations?

https://codereview.chromium.org/850183002/diff/100001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/850183002/diff/100001/content/common/media/cd...
content/common/media/cdm_messages.h:33: IPC_STRUCT_TRAITS_MEMBER(key_id)
Unfortunately, clang-format misformats this. Do you mind indenting this?

https://codereview.chromium.org/850183002/diff/100001/content/renderer/media/...
File content/renderer/media/crypto/proxy_media_keys.cc (right):

https://codereview.chromium.org/850183002/diff/100001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.cc:49:
std::vector<uint8>(certificate_data,
I wonder if some sort of StringPiece equivalent for vectors would help reduce
the amount of intermediate copies.

std::move() might work nicely for this end as well (once it's allowed)

https://codereview.chromium.org/850183002/diff/100001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/100001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:67: //
CdmKeyInformation::KeyStatus.
As I recall, there's some way to automatically generate java bindings from C++
enums. Maybe a cleanup for the future?

https://codereview.chromium.org/850183002/diff/100001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/100001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:75: const char kDummyKeyId[] = "Dummy Key
Id";
These constants should probably all be in an unnamed namespace.

[xhwang, 2015-01-17 01:16:09]

comments addressed

[xhwang, 2015-01-17 01:20:40]

dcheng: Comments addressed.

Sorry about the big CL. Usually I am a big fan of smaller CLs. But this part of
code is months behind ToT in media/ (see the ducktape code I removed from
ProxyMediaKeys). To split this into smaller CLs, I need to add some new glue
code to make sure things are still working, which is a pain. I actually already
have some small preparation CLs before this, but kinda felt it's hard to split
this any further.

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:197: void
BrowserCdmManager::ResolvePromise<std::string>(
On 2015/01/17 00:08:16, dcheng wrote:
> I think I find the new version much harder to read (and it makes it hard for a
> reader to resolve which 'overload' ResolvePromise will call). Why not just
call
> this ResolvePromiseWithSession rather than adding explicit template
> specializations?

If I don't use template here, I need to specialize the templates on l.74.

Also media::CdmPromiseTemplate tends to be generic (we used to have 3 types of
templates, which was recently reduced to 2). If we ever add another promise
type, the current approach is more scalable, i.e. l.74 doesn't need to change.

https://codereview.chromium.org/850183002/diff/100001/content/common/media/cd...
File content/common/media/cdm_messages.h (right):

https://codereview.chromium.org/850183002/diff/100001/content/common/media/cd...
content/common/media/cdm_messages.h:33: IPC_STRUCT_TRAITS_MEMBER(key_id)
On 2015/01/17 00:08:16, dcheng wrote:
> Unfortunately, clang-format misformats this. Do you mind indenting this?

That's what I did but clang-format kept doing this for me so I gave up :)

Done.

https://codereview.chromium.org/850183002/diff/100001/content/renderer/media/...
File content/renderer/media/crypto/proxy_media_keys.cc (right):

https://codereview.chromium.org/850183002/diff/100001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.cc:49:
std::vector<uint8>(certificate_data,
On 2015/01/17 00:08:16, dcheng wrote:
> I wonder if some sort of StringPiece equivalent for vectors would help reduce
> the amount of intermediate copies.
> 
> std::move() might work nicely for this end as well (once it's allowed)

It's only for legacy reasons that we pass uint8*/length in these functions. I'll
fix all of them soon: https://code.google.com/p/chromium/issues/detail?id=433446

https://codereview.chromium.org/850183002/diff/100001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/100001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:67: //
CdmKeyInformation::KeyStatus.
On 2015/01/17 00:08:16, dcheng wrote:
> As I recall, there's some way to automatically generate java bindings from C++
> enums. Maybe a cleanup for the future?

+qinmin: If there's a way to do this, I'll be happy to clean this file up in a
separate CL.

https://codereview.chromium.org/850183002/diff/100001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/850183002/diff/100001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:75: const char kDummyKeyId[] = "Dummy Key
Id";
On 2015/01/17 00:08:16, dcheng wrote:
> These constants should probably all be in an unnamed namespace.

consts are by default static, which is equivalent to unnamed namespace. 

Still Done because I agree that is cleaner. Sorry for moving code around.

[dcheng, 2015-01-20 23:33:03]

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:197: void
BrowserCdmManager::ResolvePromise<std::string>(
On 2015/01/17 01:20:39, xhwang wrote:
> On 2015/01/17 00:08:16, dcheng wrote:
> > I think I find the new version much harder to read (and it makes it hard for
a
> > reader to resolve which 'overload' ResolvePromise will call). Why not just
> call
> > this ResolvePromiseWithSession rather than adding explicit template
> > specializations?
> 
> If I don't use template here, I need to specialize the templates on l.74.
> 
> Also media::CdmPromiseTemplate tends to be generic (we used to have 3 types of
> templates, which was recently reduced to 2). If we ever add another promise
> type, the current approach is more scalable, i.e. l.74 doesn't need to change.

I still think that this template overload is confusing (and violates the spirit
of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...).

Naming these methods ResolveNewSessionPromise and ResolveSimplePromise will
result in a bit more code, but it will be much clearer to read.

[xhwang, 2015-01-21 00:03:03]

Please take another look. Thanks!

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/100001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:197: void
BrowserCdmManager::ResolvePromise<std::string>(
On 2015/01/20 23:33:03, dcheng wrote:
> On 2015/01/17 01:20:39, xhwang wrote:
> > On 2015/01/17 00:08:16, dcheng wrote:
> > > I think I find the new version much harder to read (and it makes it hard
for
> a
> > > reader to resolve which 'overload' ResolvePromise will call). Why not just
> > call
> > > this ResolvePromiseWithSession rather than adding explicit template
> > > specializations?
> > 
> > If I don't use template here, I need to specialize the templates on l.74.
> > 
> > Also media::CdmPromiseTemplate tends to be generic (we used to have 3 types
of
> > templates, which was recently reduced to 2). If we ever add another promise
> > type, the current approach is more scalable, i.e. l.74 doesn't need to
change.
> 
> I still think that this template overload is confusing (and violates the
spirit
> of
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...).
> 
> Naming these methods ResolveNewSessionPromise and ResolveSimplePromise will
> result in a bit more code, but it will be much clearer to read.

Sure. Done.

[dcheng, 2015-01-21 00:12:05]

IPC changes LGTM

https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:92: void
CdmPromiseInternal<>::resolve() {
I'm not going to push for it in this patchset, but I believe that
CdmPromiseInternal (and CdmPromiseTemplate) should probably also be
de-templatized, and SimplePromise/NewSessionPromise should be proper subclasses
of CdmPromiseInternal (same reason: I believe that the current code, while
clever, is in violation of the style guide principles on function overloading).

[xhwang, 2015-01-21 00:52:37]

On 2015/01/21 00:12:05, dcheng wrote:
> IPC changes LGTM
> 
>
https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
> File content/browser/media/cdm/browser_cdm_manager.cc (right):
> 
>
https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
> content/browser/media/cdm/browser_cdm_manager.cc:92: void
> CdmPromiseInternal<>::resolve() {
> I'm not going to push for it in this patchset, but I believe that
> CdmPromiseInternal (and CdmPromiseTemplate) should probably also be
> de-templatized, and SimplePromise/NewSessionPromise should be proper
subclasses
> of CdmPromiseInternal (same reason: I believe that the current code, while
> clever, is in violation of the style guide principles on function
overloading).

Thanks for the review!

I understand your point. But it didn't work very well for us before. Here are
more details:

Note that CdmPromiseTemplate is the base type we pass around. So it needs to
have the correct resolve method (with nothing, or with std::string) on it's
interface. Based on what you suggested, we should have something like
CdmPromiseBase and CdmPromiseWithSessionBase. Then we'll have CdmPromiseInternal
inheriting CdmPromiseBase, and CdmPromiseWithSessionInternal inheriting
CdmPromiseWithSessionBase. Note that CdmPromiseInternal and
CdmPromiseWithSessionInternal actually are almost the same. You either end up
with a lot of duplicate code, or you need to have a common class (e.g.
CdmPromiseInternalImpl) doing most of the common work. Either way, you end up
with more classes and more code.

If this doesn't look terrible to you, we used to have 3 promise types (which was
recently dropped to 2; but we could add more promise types later). Having to
maintain three interfaces and subclasses make the code really ugly. That the
reason we switched to use templates. 

Note also that we actually have 3 other places where we need to inherit from
CdmPromiseTemplate
(https://code.google.com/p/chromium/codesearch#search/&q=%22public%20CdmPromis...).
We'll need more common implementation classes if we de-templatize
CdmPromiseTemplate.

BTW, CdmPromiseInternal and CdmPromiseTemplate are class templates, so the
function overloading rule doesn't really apply here. We do have a section about
"Template metaprogramming"
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Template_meta...),
but I don't really think we are using any fancy, hard-to-understand template
techniques... (well, my opinion may be biased...)

[xhwang, 2015-01-21 05:45:05]

rebase only

[xhwang, 2015-01-21 06:48:32]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-01-21 06:51:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/850183002/160001

[commit-bot: I haz the power, 2015-01-21 14:14:24]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-21 14:16:44]

Patchset 9 (id:??) landed as
https://crrev.com/d7e021d86cd234f8eb2d48e99f82042b92b1b5c2
Cr-Commit-Position: refs/heads/master@{#312348}

[dcheng, 2015-01-21 22:13:38]

On 2015/01/21 at 00:52:37, xhwang wrote:
> On 2015/01/21 00:12:05, dcheng wrote:
> > IPC changes LGTM
> > 
> >
https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
> > File content/browser/media/cdm/browser_cdm_manager.cc (right):
> > 
> >
https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
> > content/browser/media/cdm/browser_cdm_manager.cc:92: void
> > CdmPromiseInternal<>::resolve() {
> > I'm not going to push for it in this patchset, but I believe that
> > CdmPromiseInternal (and CdmPromiseTemplate) should probably also be
> > de-templatized, and SimplePromise/NewSessionPromise should be proper
subclasses
> > of CdmPromiseInternal (same reason: I believe that the current code, while
> > clever, is in violation of the style guide principles on function
overloading).
> 
> Thanks for the review!
> 
> I understand your point. But it didn't work very well for us before. Here are
more details:
> 
> Note that CdmPromiseTemplate is the base type we pass around. So it needs to
have the correct resolve method (with nothing, or with std::string) on it's
interface. Based on what you suggested, we should have something like
CdmPromiseBase and CdmPromiseWithSessionBase. Then we'll have CdmPromiseInternal
inheriting CdmPromiseBase, and CdmPromiseWithSessionInternal inheriting
CdmPromiseWithSessionBase. Note that CdmPromiseInternal and
CdmPromiseWithSessionInternal actually are almost the same. You either end up
with a lot of duplicate code, or you need to have a common class (e.g.
CdmPromiseInternalImpl) doing most of the common work. Either way, you end up
with more classes and more code.
> 
> If this doesn't look terrible to you, we used to have 3 promise types (which
was recently dropped to 2; but we could add more promise types later). Having to
maintain three interfaces and subclasses make the code really ugly. That the
reason we switched to use templates. 
> 
> Note also that we actually have 3 other places where we need to inherit from
CdmPromiseTemplate
(https://code.google.com/p/chromium/codesearch#search/&q=%22public%20CdmPromis...).
We'll need more common implementation classes if we de-templatize
CdmPromiseTemplate.
> 
> BTW, CdmPromiseInternal and CdmPromiseTemplate are class templates, so the
function overloading rule doesn't really apply here. We do have a section about
"Template metaprogramming"
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Template_meta...),
but I don't really think we are using any fancy, hard-to-understand template
techniques... (well, my opinion may be biased...)

I'm happy to sit down sometime to try to figure out alternatives.

Really, the primary concern I have is the overloading of resolve(). If I wrote
the following code:
class ChromeExtensionAPIFunctionDispatcher {
 public:
  scoped_ptr<base::Value> Run() {
    return GetBackgroundPage();
  }

  scoped_ptr<base::Value> Run(const std::string& arg) {
    return GetExtensionURLPath(arg);
  }
};

I would expect a reviewer to complain. And that is exactly what
CdmPromiseTemplate does =/

[xhwang, 2015-01-21 22:28:59]

On 2015/01/21 22:13:38, dcheng wrote:
> On 2015/01/21 at 00:52:37, xhwang wrote:
> > On 2015/01/21 00:12:05, dcheng wrote:
> > > IPC changes LGTM
> > > 
> > >
>
https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
> > > File content/browser/media/cdm/browser_cdm_manager.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/850183002/diff/140001/content/browser/media/c...
> > > content/browser/media/cdm/browser_cdm_manager.cc:92: void
> > > CdmPromiseInternal<>::resolve() {
> > > I'm not going to push for it in this patchset, but I believe that
> > > CdmPromiseInternal (and CdmPromiseTemplate) should probably also be
> > > de-templatized, and SimplePromise/NewSessionPromise should be proper
> subclasses
> > > of CdmPromiseInternal (same reason: I believe that the current code, while
> > > clever, is in violation of the style guide principles on function
> overloading).
> > 
> > Thanks for the review!
> > 
> > I understand your point. But it didn't work very well for us before. Here
are
> more details:
> > 
> > Note that CdmPromiseTemplate is the base type we pass around. So it needs to
> have the correct resolve method (with nothing, or with std::string) on it's
> interface. Based on what you suggested, we should have something like
> CdmPromiseBase and CdmPromiseWithSessionBase. Then we'll have
CdmPromiseInternal
> inheriting CdmPromiseBase, and CdmPromiseWithSessionInternal inheriting
> CdmPromiseWithSessionBase. Note that CdmPromiseInternal and
> CdmPromiseWithSessionInternal actually are almost the same. You either end up
> with a lot of duplicate code, or you need to have a common class (e.g.
> CdmPromiseInternalImpl) doing most of the common work. Either way, you end up
> with more classes and more code.
> > 
> > If this doesn't look terrible to you, we used to have 3 promise types (which
> was recently dropped to 2; but we could add more promise types later). Having
to
> maintain three interfaces and subclasses make the code really ugly. That the
> reason we switched to use templates. 
> > 
> > Note also that we actually have 3 other places where we need to inherit from
> CdmPromiseTemplate
>
(https://code.google.com/p/chromium/codesearch#search/&q=%22public%20CdmPromis...).
> We'll need more common implementation classes if we de-templatize
> CdmPromiseTemplate.
> > 
> > BTW, CdmPromiseInternal and CdmPromiseTemplate are class templates, so the
> function overloading rule doesn't really apply here. We do have a section
about
> "Template metaprogramming"
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Template_meta...),
> but I don't really think we are using any fancy, hard-to-understand template
> techniques... (well, my opinion may be biased...)
> 
> I'm happy to sit down sometime to try to figure out alternatives.
> 
> Really, the primary concern I have is the overloading of resolve(). If I wrote
> the following code:
> class ChromeExtensionAPIFunctionDispatcher {
>  public:
>   scoped_ptr<base::Value> Run() {
>     return GetBackgroundPage();
>   }
> 
>   scoped_ptr<base::Value> Run(const std::string& arg) {
>     return GetExtensionURLPath(arg);
>   }
> };
> 
> I would expect a reviewer to complain. And that is exactly what
> CdmPromiseTemplate does =/

Yeah, I agree the example you have is a clear violation of the style guide.

But in our case, we don't have function templates in a class. Instead, we have
one normal non-templated function (resolve()) in class templates. I agree this
is not perfect and sometimes can also be confusing. But as I mentioned, we
didn't find any other better solutions. If you have good suggestions I'd be
happy to make the change :)

[gunsch, 2015-01-22 05:41:15]

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:554:
Log.d(TAG, "Session " + bytesToHexString(sessionId) + "closed.");
Should onSessionClosed have been called here? Looks like it's become an orphaned
function, so the media::SessionClosedCB is never called...

[xhwang, 2015-01-22 17:42:14]

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:554:
Log.d(TAG, "Session " + bytesToHexString(sessionId) + "closed.");
On 2015/01/22 05:41:15, gunsch wrote:
> Should onSessionClosed have been called here? Looks like it's become an
orphaned
> function, so the media::SessionClosedCB is never called...

Good question.

For unprefixed EME, a session can be closed for two reasons:

1, As a result of close() call. In this case closeSession() is called on
MediaDrmBridge. When the promised is resolved above, in JS, the promise returned
by closed() attribute will be resolved:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

2, When the CDM closes a session automatically. This was not supported by the
old spec, but is supported in the new spec:
http://w3c.github.io/encrypted-media/#session-close

In MediaDrmBridge, we are not implementing this yet. In the future, when a tab
becomes inactive (swapped to background), we want to release all the
MediaDrmBridge resources so that other apps can use the sessions.
OnSessionClosed() should be called in this case. Note that currently we are
already doing this (releasing resources) in MediaSourcePlayer, but not in
MediaDrmBridge.


For prefixed EME, when the promise is resolved, OnSessionClosed() will be fired
in ProxyDecryptor:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/proxy_de...

[gunsch, 2015-01-22 17:47:02]

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:554:
Log.d(TAG, "Session " + bytesToHexString(sessionId) + "closed.");
Thanks for the details. So to confirm my understanding, it sounds like the
meaning of SessionClosedCB& has changed slightly. It should now only be called
when the session is closed _outside_ of the closeSession(Promise) path, and no
longer as a standard response to a closeSession() operation.

[xhwang, 2015-01-22 17:48:44]

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:554:
Log.d(TAG, "Session " + bytesToHexString(sessionId) + "closed.");
On 2015/01/22 17:47:01, gunsch wrote:
> Thanks for the details. So to confirm my understanding, it sounds like the
> meaning of SessionClosedCB& has changed slightly. It should now only be called
> when the session is closed _outside_ of the closeSession(Promise) path, and no
> longer as a standard response to a closeSession() operation.

This is a perfect summarize :)

[ddorwin, 2015-01-22 17:54:32]

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:554:
Log.d(TAG, "Session " + bytesToHexString(sessionId) + "closed.");
On 2015/01/22 17:48:44, xhwang wrote:
> On 2015/01/22 17:47:01, gunsch wrote:
> > Thanks for the details. So to confirm my understanding, it sounds like the
> > meaning of SessionClosedCB& has changed slightly. It should now only be
called
> > when the session is closed _outside_ of the closeSession(Promise) path, and
no
> > longer as a standard response to a closeSession() operation.
> 
> This is a perfect summarize :)

It is not a response to close session (we have promises for that), but if/when
the CDM closes the session in response to closeSession(), it should call
SessionClosedCB.

SessionClosedCB is how we know to resolve the closed promise attribute on
MediaKeySession.

[xhwang, 2015-01-22 18:01:53]

On 2015/01/22 17:54:32, ddorwin wrote:
>
https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
> File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java
(right):
> 
>
https://codereview.chromium.org/850183002/diff/160001/media/base/android/java...
> media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:554:
> Log.d(TAG, "Session " + bytesToHexString(sessionId) + "closed.");
> On 2015/01/22 17:48:44, xhwang wrote:
> > On 2015/01/22 17:47:01, gunsch wrote:
> > > Thanks for the details. So to confirm my understanding, it sounds like the
> > > meaning of SessionClosedCB& has changed slightly. It should now only be
> called
> > > when the session is closed _outside_ of the closeSession(Promise) path,
and
> no
> > > longer as a standard response to a closeSession() operation.
> > 
> > This is a perfect summarize :)
> 
> It is not a response to close session (we have promises for that), but if/when
> the CDM closes the session in response to closeSession(), it should call
> SessionClosedCB.
> 
> SessionClosedCB is how we know to resolve the closed promise attribute on
> MediaKeySession.

David is right and I misunderstood it. Now I looked at other places and we
should call OnSessionClosed() after resolving the promise:

https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/ap...

and here is what AesDecryptor does:

https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/aes_decr...

I'll fix MediaDrmBridge.

But now I am confused about prefixed EME, are we calling OnSessionClosed()
twice? One in the promise resolving, and one explicitly? I'll double check this.