[Clank] Load/Remove persistent license

[Clank] Load/Remove persistent license

Add support for load/remove persistent license at Android level.

Load:
1. Load eme session ID to key set ID map from persistent storage.
2. MediaDrm.restoreKeys.

Remove:
1. Generate key release request.
2. Update session with key release response.
3. Clear persistent storage.

BUG=493521
TEST=test page

Review-Url: https://codereview.chromium.org/2796843002
Cr-Commit-Position: refs/heads/master@{#462690}
Committed: https://chromium.googlesource.com/chromium/src/+/ce1d00b6d9fc5330576ae210ecbc6a8c2d68a020

[yucliu1, 2017-04-04 07:06:48]

Description was changed from

==========
[Clank] Load/Remove persistent license

Add support for load/remove persistent license at Android level.

Load:
1. Load eme session ID to key set ID map from persistent storage.
2. MediaDrm.restoreKeys.

Remove:
1. Generate key release request.
2. Update session with key release response.
3. Clear persistent storage.

BUG=493521
TEST=test page
==========

to

==========
[Clank] Load/Remove persistent license

Add support for load/remove persistent license at Android level.

Load:
1. Load eme session ID to key set ID map from persistent storage.
2. MediaDrm.restoreKeys.

Remove:
1. Generate key release request.
2. Update session with key release response.
3. Clear persistent storage.

BUG=493521
TEST=test page
==========

[yucliu1, 2017-04-04 07:06:48]

yucliu@chromium.org changed reviewers:
+ jrummell@chromium.org, xhwang@chromium.org

[yucliu1, 2017-04-04 07:07:26]

Persistent license load and remove without eme session ID to key set ID map.

[yucliu1, 2017-04-04 21:42:20]

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java
(right):

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java:274:
remove(sessionId);
Don't remove the session, just set key set id to null. Later application may
need to close the session.

[yucliu1, 2017-04-05 00:57:45]

Small bug fixes when running a real test app.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java
(right):

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java:274:
remove(sessionId);
On 2017/04/04 21:42:20, yucliu1 wrote:
> Don't remove the session, just set key set id to null. Later application may
> need to close the session.

Done.

[xhwang, 2017-04-05 18:31:50]

Looking pretty good. I only have a few nits/comments. Some comments are in the
previous PS. Thanks!

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

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:975: private
void onPersistentLicenseNoExist(long promiseId) {
nit: Please add a comment that this is per Chromium's CDM API:

https://cs.chromium.org/chromium/src/media/base/content_decryption_module.h?r...

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java
(right):

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java:215:
assert info != null;
To avoid confusion:

assert info.sessionId() == sessionId;

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.cc:282: bool
MediaDrmBridge::IsPersistentLicenseTypeSupported(
Please also check the following in this function:

base::android::BuildInfo::GetInstance()->sdk_int() >= 23

similar to the logic around use_origin_isolated_storage.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.cc:463:
DCHECK(base::FeatureList::IsEnabled(kMediaDrmPersistentLicense));
You can probably just DCHECK(IsPersistentLicenseTypeSupported()) here.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.cc:475: session_id.size());
Might worth putting this in a helper function.

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

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:104: private
static class SessionEventDeferrer {
Please add more comments why we need this.

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:952:
onPersistentLicenseNoExist(promiseId);
When can this happen? When user manually unprovisioned the device but didn't
clear the storage? Shall we also close the session and clear the storage in this
case?

For these unexpected cases, fail quickly is probably the best solution :)

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:1016:
onSessionMessage(sessionId, request);
The spec says queue the message, then return the promise. This implementation is
okay since the order of JS events should be the same as expected. But might
worth a comment.

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/medi...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/medi...
media/base/android/media_drm_bridge.h:223: bool is_key_release);
Add a comment why "key_release" is special.

[yucliu1, 2017-04-05 23:04:07]

Address comment in PS1 and PS2, please take a look.

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

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:975: private
void onPersistentLicenseNoExist(long promiseId) {
On 2017/04/05 18:31:50, xhwang_slow wrote:
> nit: Please add a comment that this is per Chromium's CDM API:
> 
>
https://cs.chromium.org/chromium/src/media/base/content_decryption_module.h?r...

Done.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java
(right):

https://codereview.chromium.org/2796843002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmSessionManager.java:215:
assert info != null;
On 2017/04/05 18:31:50, xhwang_slow wrote:
> To avoid confusion:
> 
> assert info.sessionId() == sessionId;

Done.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.cc:282: bool
MediaDrmBridge::IsPersistentLicenseTypeSupported(
On 2017/04/05 18:31:50, xhwang_slow wrote:
> Please also check the following in this function:
> 
> base::android::BuildInfo::GetInstance()->sdk_int() >= 23
> 
> similar to the logic around use_origin_isolated_storage.

Done.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.cc:463:
DCHECK(base::FeatureList::IsEnabled(kMediaDrmPersistentLicense));
On 2017/04/05 18:31:50, xhwang_slow wrote:
> You can probably just DCHECK(IsPersistentLicenseTypeSupported()) here.

Done.

https://codereview.chromium.org/2796843002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.cc:475: session_id.size());
On 2017/04/05 18:31:50, xhwang_slow wrote:
> Might worth putting this in a helper function.

Done.

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

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:104: private
static class SessionEventDeferrer {
On 2017/04/05 18:31:50, xhwang_slow wrote:
> Please add more comments why we need this.

Done.

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:952:
onPersistentLicenseNoExist(promiseId);
On 2017/04/05 18:31:50, xhwang_slow wrote:
> When can this happen? When user manually unprovisioned the device but didn't
> clear the storage? Shall we also close the session and clear the storage in
this
> case?
> 
> For these unexpected cases, fail quickly is probably the best solution :)

Early return.

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:1016:
onSessionMessage(sessionId, request);
On 2017/04/05 18:31:50, xhwang_slow wrote:
> The spec says queue the message, then return the promise. This implementation
is
> okay since the order of JS events should be the same as expected. But might
> worth a comment.

Switched sequence to match spec.

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/medi...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/2796843002/diff/20001/media/base/android/medi...
media/base/android/media_drm_bridge.h:223: bool is_key_release);
On 2017/04/05 18:31:50, xhwang_slow wrote:
> Add a comment why "key_release" is special.

Done.

[xhwang, 2017-04-06 04:41:30]

lgtm with nits, thanks!

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

https://codereview.chromium.org/2796843002/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:1028:
onPromiseResolved(promiseId);
Sorry I didn't make it clear. Since it's "Queue a "message" Event", the
"message" should be posted, so it should happen in JS after the promise is
resolved. In MediaDrmBridge.cc, we post both onSessionMessage and
onPromiseResolved, so I suspect with the current CL we'll get the message first,
unless we have some additional posts somewhere. To be on the safe side, maybe
revert the order?

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

https://codereview.chromium.org/2796843002/diff/60001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:295: NOTIMPLEMENTED() << "In development.
See http://crbug.com/493521";
You can move this to a comment on line 263. Then this function can just be:

return IsPersistentLicenseTypeSupportedByMediaDrm();

[yucliu1, 2017-04-06 16:47:24]

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

https://codereview.chromium.org/2796843002/diff/60001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:1028:
onPromiseResolved(promiseId);
On 2017/04/06 04:41:30, xhwang_slow wrote:
> Sorry I didn't make it clear. Since it's "Queue a "message" Event", the
> "message" should be posted, so it should happen in JS after the promise is
> resolved. In MediaDrmBridge.cc, we post both onSessionMessage and
> onPromiseResolved, so I suspect with the current CL we'll get the message
first,
> unless we have some additional posts somewhere. To be on the safe side, maybe
> revert the order?

Done.

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

https://codereview.chromium.org/2796843002/diff/60001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:295: NOTIMPLEMENTED() << "In development.
See http://crbug.com/493521";
On 2017/04/06 04:41:30, xhwang_slow wrote:
> You can move this to a comment on line 263. Then this function can just be:
> 
> return IsPersistentLicenseTypeSupportedByMediaDrm();

Done.

[xhwang, 2017-04-06 16:57:40]

lgtm

[yucliu1, 2017-04-06 17:25:03]

On 2017/04/06 16:57:40, xhwang_slow wrote:
> lgtm

John, any comments?

[jrummell, 2017-04-06 21:51:27]

lgtm

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

https://codereview.chromium.org/2796843002/diff/80001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:107: //
promise isn't resolved. The class can block and colloect these events and
nit: s/colloect/collect/

https://codereview.chromium.org/2796843002/diff/80001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:902: * Load
persistent license from stroage.
nit: s/stroage/storage/

[yucliu1, 2017-04-06 22:05:31]

Rebase + nit.

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

https://codereview.chromium.org/2796843002/diff/80001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:107: //
promise isn't resolved. The class can block and colloect these events and
On 2017/04/06 21:51:27, jrummell wrote:
> nit: s/colloect/collect/

Done.

https://codereview.chromium.org/2796843002/diff/80001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:902: * Load
persistent license from stroage.
On 2017/04/06 21:51:27, jrummell wrote:
> nit: s/stroage/storage/

Done.

[yucliu1, 2017-04-06 22:05:39]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-04-06 22:05:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2796843002/#ps100001
(title: "nit")

[commit-bot: I haz the power, 2017-04-06 22:07:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 23:53:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491516339498810,
"parent_rev": "9b7ef119f3fccf1aecf18202637ae3d36abda692", "commit_rev":
"ce1d00b6d9fc5330576ae210ecbc6a8c2d68a020"}

[commit-bot: I haz the power, 2017-04-06 23:54:22]

Description was changed from

==========
[Clank] Load/Remove persistent license

Add support for load/remove persistent license at Android level.

Load:
1. Load eme session ID to key set ID map from persistent storage.
2. MediaDrm.restoreKeys.

Remove:
1. Generate key release request.
2. Update session with key release response.
3. Clear persistent storage.

BUG=493521
TEST=test page
==========

to

==========
[Clank] Load/Remove persistent license

Add support for load/remove persistent license at Android level.

Load:
1. Load eme session ID to key set ID map from persistent storage.
2. MediaDrm.restoreKeys.

Remove:
1. Generate key release request.
2. Update session with key release response.
3. Clear persistent storage.

BUG=493521
TEST=test page

Review-Url: https://codereview.chromium.org/2796843002
Cr-Commit-Position: refs/heads/master@{#462690}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce1d00b6d9fc5330576ae210ecbc...
==========

[commit-bot: I haz the power, 2017-04-06 23:54:24]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ce1d00b6d9fc5330576ae210ecbc...