[Android] JNI binding for MediaDrmCredentialManager

[Android] JNI binding for MediaDrmCredentialManager

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

[Kibeom Kim (inactive), 2013-09-20 21:58:19]

Yaron -- could you take a look at these? Thanks.
- content_jni.gypi
- MediaDrmCredentialManager.java

[Kibeom Kim (inactive), 2013-09-20 21:59:48]

+ browser_jni_registrar.cc

[qinmin, 2013-09-20 22:25:49]

lgtm % comments

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
content/browser/media/android/media_drm_credential_manager.cc:20:
ScopedJavaGlobalRef<jobject>* j_media_drm_credential_manager_callback,
i don't think you need a pointer to a javaref, just use scopedJavaGlobalRef
should be fine.

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
content/browser/media/android/media_drm_credential_manager.cc:63:
ScopedJavaGlobalRef<jobject>* j_scoped_media_drm_credential_manager_callback =
you don't need to new the pointer.

[Kibeom Kim (inactive), 2013-09-20 22:39:41]

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
content/browser/media/android/media_drm_credential_manager.cc:63:
ScopedJavaGlobalRef<jobject>* j_scoped_media_drm_credential_manager_callback =
On 2013/09/20 22:25:50, qinmin wrote:
> you don't need to new the pointer.

Question: then what's the correct way to modify base::Owned(...) below?

[Yaron, 2013-09-20 23:04:37]

Can you piont me to client usage?

https://codereview.chromium.org/24152015/diff/6001/content/browser/media/andr...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/6001/content/browser/media/andr...
content/browser/media/android/media_drm_credential_manager.cc:14: using
base::android::AttachCurrentThread;
NIT: I wouldn't bother with this using. It doesn't save linebreaks or improve
readability.

https://codereview.chromium.org/24152015/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
(right):

https://codereview.chromium.org/24152015/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:15:
int mNativeMediaDrmCredentialManager;
private

[Kibeom Kim (inactive), 2013-09-20 23:30:48]

client usage: https://gerrit-int.chromium.org/#/c/44347/

not done but should be working

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/1/content/browser/media/android...
content/browser/media/android/media_drm_credential_manager.cc:20:
ScopedJavaGlobalRef<jobject>* j_media_drm_credential_manager_callback,
On 2013/09/20 22:25:50, qinmin wrote:
> i don't think you need a pointer to a javaref, just use scopedJavaGlobalRef
> should be fine.

Done.

https://codereview.chromium.org/24152015/diff/6001/content/browser/media/andr...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/6001/content/browser/media/andr...
content/browser/media/android/media_drm_credential_manager.cc:14: using
base::android::AttachCurrentThread;
On 2013/09/20 23:04:37, Yaron wrote:
> NIT: I wouldn't bother with this using. It doesn't save linebreaks or improve
> readability.

Done.

[Yaron, 2013-09-20 23:53:57]

It seems a little strange to me that we have this java wrapper for a c++ class
which basically just creates a MediaDrmBridge temporarily in c++, and which
creates a java counterpart just to call through a seemingly static java android
api. Could you instead just take the body of
MediaDrmBridge.resetDeviceCredentials and call that?

https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
(right):

https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:53:
public void ResetCredentials(MediaDrmCredentialManagerCallback callback) {
Err. resetCredentials.

[Kibeom Kim (inactive), 2013-09-21 00:02:55]

On 2013/09/20 23:53:57, Yaron wrote:
> It seems a little strange to me that we have this java wrapper for a c++ class
> which basically just creates a MediaDrmBridge temporarily in c++, and which
> creates a java counterpart just to call through a seemingly static java
android
> api. Could you instead just take the body of
> MediaDrmBridge.resetDeviceCredentials and call that?

Yeah I felt in the same way. @qinmin how about making ResetCredentials(...) a
static function. Probably it can just around media_drm_bridge_ and
reset_credentials_cb_ instead of saving as member variables.

[Kibeom Kim (inactive), 2013-09-21 00:13:45]

talked with qinmin@ offline, he is working on it

[qinmin, 2013-09-21 00:40:16]

On 2013/09/20 23:53:57, Yaron wrote:
> It seems a little strange to me that we have this java wrapper for a c++ class
> which basically just creates a MediaDrmBridge temporarily in c++, and which
> creates a java counterpart just to call through a seemingly static java
android
> api. Could you instead just take the body of
> MediaDrmBridge.resetDeviceCredentials and call that?
> 
>
https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
> (right):
> 
>
https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:53:
> public void ResetCredentials(MediaDrmCredentialManagerCallback callback) {
> Err. resetCredentials.

There are a several things:
1.the MediaDrmBridge.resetDeviceCredentials() is asynchronous, and it is not a
static call. Reset credential can actually fail, that's why we need the
callback.
2. The MediaDrmCredentialManager was introduced to help the caller from knowing
the MediaDrmBridge.  Basically we need to create 2 MediaDrmBridge, call the
asynchronous call, and then callback the caller.
3. Having multiple MediaDrmBridge instances to reset the credentials can mess
things up since getProvisionRequest() and provideProvisionResponse() can arrive
out of order.
I think we should have a global MediaDrmCredentialManager instance, this should
not be a per tab thingy. And UI code can get the instance to reset the
credentials.

[qinmin, 2013-09-23 16:17:01]

https://codereview.chromium.org/24152015/diff/16001/content/browser/media/and...
File content/browser/media/android/media_drm_credential_manager.h (left):

https://codereview.chromium.org/24152015/diff/16001/content/browser/media/and...
content/browser/media/android/media_drm_credential_manager.h:27: void
ResetCredentials(const ResetCredentialsCB& callback);
nit: you can move this to private

[qinmin, 2013-09-23 16:19:02]

https://codereview.chromium.org/24152015/diff/16001/content/browser/media/and...
File content/browser/media/android/media_drm_credential_manager.h (left):

https://codereview.chromium.org/24152015/diff/16001/content/browser/media/and...
content/browser/media/android/media_drm_credential_manager.h:27: void
ResetCredentials(const ResetCredentialsCB& callback);
or just remove the function and merge it with the native jni call
On 2013/09/23 16:17:01, qinmin wrote:
> nit: you can move this to private

[Kibeom Kim (inactive), 2013-09-23 18:28:22]

https://codereview.chromium.org/24152015/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
(right):

https://codereview.chromium.org/24152015/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:15:
int mNativeMediaDrmCredentialManager;
On 2013/09/20 23:04:37, Yaron wrote:
> private

Done.

https://codereview.chromium.org/24152015/diff/16001/content/browser/media/and...
File content/browser/media/android/media_drm_credential_manager.h (left):

https://codereview.chromium.org/24152015/diff/16001/content/browser/media/and...
content/browser/media/android/media_drm_credential_manager.h:27: void
ResetCredentials(const ResetCredentialsCB& callback);
On 2013/09/23 16:17:01, qinmin wrote:
> nit: you can move this to private

Done. (move to private)

https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
(right):

https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:53:
public void ResetCredentials(MediaDrmCredentialManagerCallback callback) {
On 2013/09/20 23:53:57, Yaron wrote:
> Err. resetCredentials.

Done.

[Yaron, 2013-09-23 20:08:31]

On 2013/09/21 00:40:16, qinmin wrote:
> On 2013/09/20 23:53:57, Yaron wrote:
> > It seems a little strange to me that we have this java wrapper for a c++
class
> > which basically just creates a MediaDrmBridge temporarily in c++, and which
> > creates a java counterpart just to call through a seemingly static java
> android
> > api. Could you instead just take the body of
> > MediaDrmBridge.resetDeviceCredentials and call that?
> > 
> >
>
https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/24152015/diff/16001/content/public/android/ja...
> >
>
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:53:
> > public void ResetCredentials(MediaDrmCredentialManagerCallback callback) {
> > Err. resetCredentials.
> 
> There are a several things:
> 1.the MediaDrmBridge.resetDeviceCredentials() is asynchronous, and it is not a
> static call. Reset credential can actually fail, that's why we need the
> callback.
> 2. The MediaDrmCredentialManager was introduced to help the caller from
knowing
> the MediaDrmBridge.  Basically we need to create 2 MediaDrmBridge, call the
> asynchronous call, and then callback the caller.
> 3. Having multiple MediaDrmBridge instances to reset the credentials can mess
> things up since getProvisionRequest() and provideProvisionResponse() can
arrive
> out of order.
> I think we should have a global MediaDrmCredentialManager instance, this
should
> not be a per tab thingy. And UI code can get the instance to reset the
> credentials.

So are you doing ^ in a follow up?

[Kibeom Kim (inactive), 2013-09-23 21:00:16]

> So are you doing ^ in a follow up?

I'll do it.

[Kibeom Kim (inactive), 2013-09-23 21:35:51]

On 2013/09/23 21:00:16, Kibeom Kim wrote:
> > So are you doing ^ in a follow up?
> 
> I'll do it.

Done.

[Kibeom Kim (inactive), 2013-09-23 21:42:18]

Sorry please hold on a moment. I'll make the java class static

[Kibeom Kim (inactive), 2013-09-23 22:19:41]

On 2013/09/23 21:42:18, Kibeom Kim wrote:
> Sorry please hold on a moment. I'll make the java class static

Done.

[qinmin, 2013-09-24 02:35:48]

lgtm % nits

https://codereview.chromium.org/24152015/diff/60001/content/browser/media/and...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/60001/content/browser/media/and...
content/browser/media/android/media_drm_credential_manager.cc:11: #include
"base/memory/singleton.h"
nit: this is already included in the header

https://codereview.chromium.org/24152015/diff/60001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
(right):

https://codereview.chromium.org/24152015/diff/60001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:23:
* @param succeeded Whether or not if it succeeded.
nit: no if

[Kibeom Kim (inactive), 2013-09-24 02:52:48]

https://codereview.chromium.org/24152015/diff/60001/content/browser/media/and...
File content/browser/media/android/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/24152015/diff/60001/content/browser/media/and...
content/browser/media/android/media_drm_credential_manager.cc:11: #include
"base/memory/singleton.h"
On 2013/09/24 02:35:48, qinmin wrote:
> nit: this is already included in the header

Done.

https://codereview.chromium.org/24152015/diff/60001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java
(right):

https://codereview.chromium.org/24152015/diff/60001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/MediaDrmCredentialManager.java:23:
* @param succeeded Whether or not if it succeeded.
On 2013/09/24 02:35:48, qinmin wrote:
> nit: no if

Done.

[Yaron, 2013-09-24 02:59:11]

reitveld is barfing on some of the diffs. "error: old chunk mismatch". Can you
re-upload?

[Yaron, 2013-09-24 03:00:57]

lgtm based on previous patchset (i see it was just nits)

[commit-bot: I haz the power, 2013-09-24 03:08:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/24152015/77001

[commit-bot: I haz the power, 2013-09-24 03:55:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-24 04:19:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/24152015/93001

[commit-bot: I haz the power, 2013-09-24 09:50:11]

Change committed as 224962