[Clank] Add JNI interface for media persistent license storage

[Clank] Add JNI interface for media persistent license storage

Add MediaDrmStorageBridge on Java and C++. C++ implementation is a dummy
version.

BUG=493521
Review-Url: https://codereview.chromium.org/2790783002
Cr-Commit-Position: refs/heads/master@{#461347}
Committed: https://chromium.googlesource.com/chromium/src/+/b519ac382d16fb4f3c969fbcf56722030cdca062

[yucliu1, 2017-03-30 21:36:46]

Description was changed from

==========
[Clank] Add JNI interface for media persistent license storage

Add MediaDrmStorageBridge on Java and C++. C++ implementation is a dummy
version.

BUG=493521
==========

to

==========
[Clank] Add JNI interface for media persistent license storage

Add MediaDrmStorageBridge on Java and C++. C++ implementation is a dummy
version.

BUG=493521
==========

[yucliu1, 2017-03-30 21:36:46]

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

[yucliu1, 2017-03-30 21:38:50]

Just add the interface.

+timav for the TODO.

[xhwang, 2017-03-30 23:33:42]

looking good, I only have a few nits

https://codereview.chromium.org/2790783002/diff/1/media/base/android/android_...
File media/base/android/android_util.h (right):

https://codereview.chromium.org/2790783002/diff/1/media/base/android/android_...
media/base/android/android_util.h:1: // Copyright (c) 2017 The Chromium Authors.
All rights reserved.
nit: here and everywhere else, drop the (c) part

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:927:
mStorage.init(new Callback<Boolean>() {
We should only call this when we are doing per-origin provisioning. What's the
plan on this? Will you check it here, or will MDSBridge reject it?

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.h:325: MediaDrmStorageBridge storage_;
Just in case |j_media_drm_| ties to talk to the |storage_| during destruction,
declare this before |j_media_drm_|?

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:25:
MediaDrmStorageBridge::MediaDrmStorageBridge()
Thoughts on the next CL: you can pass in the create_storage_cb_ here, and only
call it to create the MediaDrmStorage when we need it.

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.h:33: void OnInitStorage(JNIEnv*
env,
I see why you call it "Init", since this is the first time we add an entry into
the storage for the "origin". But this is a bit confusing since MediaDrm
shouldn't care about the impl detail of the storage.

Can we rename it to OnProvisioned to be consistent with the MediaDrmStorage
interface?

[yucliu1, 2017-03-31 00:57:17]

https://codereview.chromium.org/2790783002/diff/1/media/base/android/android_...
File media/base/android/android_util.h (right):

https://codereview.chromium.org/2790783002/diff/1/media/base/android/android_...
media/base/android/android_util.h:1: // Copyright (c) 2017 The Chromium Authors.
All rights reserved.
On 2017/03/30 23:33:42, xhwang_slow wrote:
> nit: here and everywhere else, drop the (c) part

Done.

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:927:
mStorage.init(new Callback<Boolean>() {
On 2017/03/30 23:33:42, xhwang_slow wrote:
> We should only call this when we are doing per-origin provisioning. What's the
> plan on this? Will you check it here, or will MDSBridge reject it?

Check if origin is empty here.

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_bridge.h:325: MediaDrmStorageBridge storage_;
On 2017/03/30 23:33:42, xhwang_slow wrote:
> Just in case |j_media_drm_| ties to talk to the |storage_| during destruction,
> declare this before |j_media_drm_|?

Done.

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:25:
MediaDrmStorageBridge::MediaDrmStorageBridge()
On 2017/03/30 23:33:42, xhwang_slow wrote:
> Thoughts on the next CL: you can pass in the create_storage_cb_ here, and only
> call it to create the MediaDrmStorage when we need it.

Just to confirm, we need to lazy create the storage only once per instance,
right?

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.h:33: void OnInitStorage(JNIEnv*
env,
On 2017/03/30 23:33:42, xhwang_slow wrote:
> I see why you call it "Init", since this is the first time we add an entry
into
> the storage for the "origin". But this is a bit confusing since MediaDrm
> shouldn't care about the impl detail of the storage.
> 
> Can we rename it to OnProvisioned to be consistent with the MediaDrmStorage
> interface?

Done.

[Tima Vaisburd, 2017-03-31 01:50:07]

Is there any specific part you'd like me to review?

[yucliu1, 2017-03-31 03:36:01]

On 2017/03/31 01:50:07, Tima Vaisburd wrote:
> Is there any specific part you'd like me to review?

I see a TODO for you in the code I touched. Not sure if you have any comments on
the usage. I'm going to use CreateJavaObjectPtr aggressively.

[xhwang, 2017-03-31 03:47:57]

lgtm % one comment

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

https://codereview.chromium.org/2790783002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:25:
MediaDrmStorageBridge::MediaDrmStorageBridge()
On 2017/03/31 00:57:17, yucliu1 wrote:
> On 2017/03/30 23:33:42, xhwang_slow wrote:
> > Thoughts on the next CL: you can pass in the create_storage_cb_ here, and
only
> > call it to create the MediaDrmStorage when we need it.
> 
> Just to confirm, we need to lazy create the storage only once per instance,
> right?

Yes, only once per instance.

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

https://codereview.chromium.org/2790783002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:950: boolean
isOriginSet = !isWidevine() || !mMediaDrm.getPropertyString(ORIGIN).isEmpty();
Is it ever possible that getPropertyString(ORIGIN) returns a non-empty string
for non-widevine or on a pre-M device? Will it be sufficient if we just check
!mMediaDrm.getPropertyString(ORIGIN).isEmpty() ?

[yucliu1, 2017-03-31 05:38:18]

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

https://codereview.chromium.org/2790783002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:950: boolean
isOriginSet = !isWidevine() || !mMediaDrm.getPropertyString(ORIGIN).isEmpty();
On 2017/03/31 03:47:57, xhwang_slow wrote:
> Is it ever possible that getPropertyString(ORIGIN) returns a non-empty string
> for non-widevine or on a pre-M device? Will it be sufficient if we just check
> !mMediaDrm.getPropertyString(ORIGIN).isEmpty() ?

We should check origin only for M or higher device. Since we return true in
setOrigin for other key systems (line 388), we also need to return true here. I
have a question here, do we want to support other key system in this release? If
no, I'll change setOrigin and here to return false. What do you think?

[xhwang, 2017-03-31 06:25:50]

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

https://codereview.chromium.org/2790783002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:950: boolean
isOriginSet = !isWidevine() || !mMediaDrm.getPropertyString(ORIGIN).isEmpty();
On 2017/03/31 05:38:17, yucliu1 wrote:
> On 2017/03/31 03:47:57, xhwang_slow wrote:
> > Is it ever possible that getPropertyString(ORIGIN) returns a non-empty
string
> > for non-widevine or on a pre-M device? Will it be sufficient if we just
check
> > !mMediaDrm.getPropertyString(ORIGIN).isEmpty() ?
> 
> We should check origin only for M or higher device. Since we return true in
> setOrigin for other key systems (line 388), we also need to return true here.
I
> have a question here, do we want to support other key system in this release?
If
> no, I'll change setOrigin and here to return false. What do you think?

At least for now, per-origin provisioning is a requirement for persistent
license. So one option is you can store a boolean on line 396, then check it
here. I am not sure whether mMediaDrm.getPropertyString(ORIGIN).isEmpty() is
equivalent to that. For now, storing a boolean (e.g. mIsOriginSet) seems to be
the easiest.

[yucliu1, 2017-03-31 18:42:30]

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

https://codereview.chromium.org/2790783002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:950: boolean
isOriginSet = !isWidevine() || !mMediaDrm.getPropertyString(ORIGIN).isEmpty();
On 2017/03/31 06:25:50, xhwang_slow wrote:
> On 2017/03/31 05:38:17, yucliu1 wrote:
> > On 2017/03/31 03:47:57, xhwang_slow wrote:
> > > Is it ever possible that getPropertyString(ORIGIN) returns a non-empty
> string
> > > for non-widevine or on a pre-M device? Will it be sufficient if we just
> check
> > > !mMediaDrm.getPropertyString(ORIGIN).isEmpty() ?
> > 
> > We should check origin only for M or higher device. Since we return true in
> > setOrigin for other key systems (line 388), we also need to return true
here.
> I
> > have a question here, do we want to support other key system in this
release?
> If
> > no, I'll change setOrigin and here to return false. What do you think?
> 
> At least for now, per-origin provisioning is a requirement for persistent
> license. So one option is you can store a boolean on line 396, then check it
> here. I am not sure whether mMediaDrm.getPropertyString(ORIGIN).isEmpty() is
> equivalent to that. For now, storing a boolean (e.g. mIsOriginSet) seems to be
> the easiest.

Good point! And I think we want to disable the feature for non-widevine key
systems for now. I can do that in another patch, at cpp level.

[xhwang, 2017-03-31 18:51:19]

lgtm, thanks!

[yucliu1, 2017-03-31 19:31:38]

On 2017/03/31 03:36:01, yucliu1 wrote:
> On 2017/03/31 01:50:07, Tima Vaisburd wrote:
> > Is there any specific part you'd like me to review?
> 
> I see a TODO for you in the code I touched. Not sure if you have any comments
on
> the usage. I'm going to use CreateJavaObjectPtr aggressively.

Tima, any comments on the TODO? Should I keep using this method?

[Tima Vaisburd, 2017-03-31 20:22:44]

On 2017/03/31 19:31:38, yucliu1 wrote:
> On 2017/03/31 03:36:01, yucliu1 wrote:
> > On 2017/03/31 01:50:07, Tima Vaisburd wrote:
> > > Is there any specific part you'd like me to review?
> > 
> > I see a TODO for you in the code I touched. Not sure if you have any
comments
> on
> > the usage. I'm going to use CreateJavaObjectPtr aggressively.
> 
> Tima, any comments on the TODO? Should I keep using this method?

I've finally found the relevant mail thread and just cc'd to you.

The relevant part is this:

torne@:
--------------
The right thing to do is simply construct temporary ScopedJavaGlobalRefs and
bind those. base::Bind will take them without copying since they're rvalues. If
you want to move them on the receiving end you can just take a
ScopedJavaGlobalRef parameter and use BindOnce and you'll get move semantics
automatically; if you only need to refer to them it's easier to take a const
JavaRef& and be consistent with other non-bound functions.

I was going to do this rewrite in media myself but I'm a bit concerned by the
comment at
https://cs.chromium.org/chromium/src/media/base/android/media_drm_bridge.h?rc...
which suggests you're currently using the smartpointer-to-smartpointer to
distinguish three states - do you actually need to make that distinction? If so
I'm not sure what the best thign to do is but I don't think it's continuing to
do what the code currently does.
-------------

So as far as I can get you can try to use ScopedJavaGlobalRefs directly.

[yucliu1, 2017-03-31 22:09:19]

On 2017/03/31 20:22:44, Tima Vaisburd wrote:
> On 2017/03/31 19:31:38, yucliu1 wrote:
> > On 2017/03/31 03:36:01, yucliu1 wrote:
> > > On 2017/03/31 01:50:07, Tima Vaisburd wrote:
> > > > Is there any specific part you'd like me to review?
> > > 
> > > I see a TODO for you in the code I touched. Not sure if you have any
> comments
> > on
> > > the usage. I'm going to use CreateJavaObjectPtr aggressively.
> > 
> > Tima, any comments on the TODO? Should I keep using this method?
> 
> I've finally found the relevant mail thread and just cc'd to you.
> 
> The relevant part is this:
> 
> torne@:
> --------------
> The right thing to do is simply construct temporary ScopedJavaGlobalRefs and
> bind those. base::Bind will take them without copying since they're rvalues.
If
> you want to move them on the receiving end you can just take a
> ScopedJavaGlobalRef parameter and use BindOnce and you'll get move semantics
> automatically; if you only need to refer to them it's easier to take a const
> JavaRef& and be consistent with other non-bound functions.
> 
> I was going to do this rewrite in media myself but I'm a bit concerned by the
> comment at
>
https://cs.chromium.org/chromium/src/media/base/android/media_drm_bridge.h?rc...
> which suggests you're currently using the smartpointer-to-smartpointer to
> distinguish three states - do you actually need to make that distinction? If
so
> I'm not sure what the best thign to do is but I don't think it's continuing to
> do what the code currently does.
> -------------
> 
> So as far as I can get you can try to use ScopedJavaGlobalRefs directly.

Thanks. We'll address this once feature is completed. Let's ship this first.

[yucliu1, 2017-03-31 22:10:23]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-03-31 22:10:24]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2790783002/#ps80001
(title: "TOTO")

[commit-bot: I haz the power, 2017-03-31 22:11:13]

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-03-31 22:38:21]

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

[commit-bot: I haz the power, 2017-03-31 22:38:23]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Tima Vaisburd, 2017-03-31 22:53:32]

lgtm with a nit

https://codereview.chromium.org/2790783002/diff/80001/media/base/android/medi...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2790783002/diff/80001/media/base/android/medi...
media/base/android/media_drm_storage_bridge.h:21: class MediaDrmStorageBridge {
Please write the comment that describes the purpose if this class.

[yucliu1, 2017-04-01 01:27:37]

https://codereview.chromium.org/2790783002/diff/80001/media/base/android/medi...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2790783002/diff/80001/media/base/android/medi...
media/base/android/media_drm_storage_bridge.h:21: class MediaDrmStorageBridge {
On 2017/03/31 22:53:32, Tima Vaisburd wrote:
> Please write the comment that describes the purpose if this class.

Done.

[yucliu1, 2017-04-01 01:27:46]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-04-01 01:27:46]

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

[commit-bot: I haz the power, 2017-04-01 01:28:13]

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-01 01:55:29]

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

[commit-bot: I haz the power, 2017-04-01 01:55:30]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[yucliu1, 2017-04-01 23:59:28]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-04-01 23:59:29]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2790783002/#ps120001
(title: "Fix lint")

[commit-bot: I haz the power, 2017-04-01 23:59:39]

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-02 02:29:45]

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

[commit-bot: I haz the power, 2017-04-02 02:29:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yucliu1, 2017-04-02 18:34:53]

The CQ bit was checked by yucliu@chromium.org

[commit-bot: I haz the power, 2017-04-02 18:34:56]

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-02 19:53:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491158093204550,
"parent_rev": "4b4f7983b399b265d58422749c9acab1dda836b5", "commit_rev":
"b519ac382d16fb4f3c969fbcf56722030cdca062"}

[commit-bot: I haz the power, 2017-04-02 19:55:16]

Description was changed from

==========
[Clank] Add JNI interface for media persistent license storage

Add MediaDrmStorageBridge on Java and C++. C++ implementation is a dummy
version.

BUG=493521
==========

to

==========
[Clank] Add JNI interface for media persistent license storage

Add MediaDrmStorageBridge on Java and C++. C++ implementation is a dummy
version.

BUG=493521

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

[commit-bot: I haz the power, 2017-04-02 19:55:17]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b519ac382d16fb4f3c969fbcf567...