Android: MediaPlayerBridge JNI cleanup.

Android: MediaPlayerBridge JNI cleanup.

This file contains some hand-written JNI, and a few
hops native<->java.
This clarifies things a bit by:
- adding a MediaPlayerBridge.java to avoid some hops.
- moving code to MediaPlayerListener.java.

Further clean up will be done by auto-generating bindings for
android.media.MediaPlayer directly.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158275

[bulach, 2012-09-20 12:56:45]

hey,

this is a quick first pass at cleaning up the remaining JNI..
if you agree with this :) there's another quick win by using generated headers
for the system MediaPlayer.. I just need to tweak a bit the name mangling before
proceeding.

please let me know your thoughts on this. thanks!

[qinmin, 2012-09-20 14:45:21]

http://codereview.chromium.org/10961015/diff/2001/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

http://codereview.chromium.org/10961015/diff/2001/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:97:
return listener;
do we really need to return the listener here? Since we passed in the
MediaPlayer object here,  this object is no longer needed after we set the
listener to MediaPlayer.

http://codereview.chromium.org/10961015/diff/2001/media/base/android/media_pl...
File media/base/android/media_player_bridge.cc (right):

http://codereview.chromium.org/10961015/diff/2001/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:107: ScopedJavaLocalRef<jobject>
j_listener(listener_.CreateMediaPlayerListener(
this scoedJavaLocalRef is not being used

[bulach, 2012-09-20 17:53:01]

thanks! comments addressed, another look please?

http://codereview.chromium.org/10961015/diff/2001/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaPlayerListener.java
(right):

http://codereview.chromium.org/10961015/diff/2001/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaPlayerListener.java:97:
return listener;
On 2012/09/20 14:45:21, qinmin wrote:
> do we really need to return the listener here? Since we passed in the
> MediaPlayer object here,  this object is no longer needed after we set the
> listener to MediaPlayer.

you're right! removed..

http://codereview.chromium.org/10961015/diff/2001/media/base/android/media_pl...
File media/base/android/media_player_bridge.cc (right):

http://codereview.chromium.org/10961015/diff/2001/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:107: ScopedJavaLocalRef<jobject>
j_listener(listener_.CreateMediaPlayerListener(
On 2012/09/20 14:45:21, qinmin wrote:
> this scoedJavaLocalRef is not being used

Done.

[Yaron, 2012-09-20 18:23:13]

lgtm once Min is happy. This is much better!

[qinmin, 2012-09-20 18:38:03]

lgtm

[scherkus (not reviewing), 2012-09-20 19:13:55]

http://codereview.chromium.org/10961015/diff/6002/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java
(right):

http://codereview.chromium.org/10961015/diff/6002/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:29:
player.setDataSource(context, uri, headersMap);
should these be 4 space indent?

http://codereview.chromium.org/10961015/diff/6002/media/base/android/media_pl...
File media/base/android/media_player_bridge.cc (right):

http://codereview.chromium.org/10961015/diff/6002/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:16: #include
"jni/MediaPlayerBridge_jni.h"
does this have to go here or can it be in the above #include block in a->z
ordering? (i.e., between base/ and media/)

http://codereview.chromium.org/10961015/diff/6002/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:153: j_context, j_url_string.obj(),
j_cookies.obj(), hide_url_log_)) {
nit: indent style is incorrect here

you'll want to drop the previous line args (env, j_m_p_.obj()) to this line

[bulach, 2012-09-21 10:10:26]

thanks all! scherkus: addressed all your comments, another quick look please?

http://codereview.chromium.org/10961015/diff/6002/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java
(right):

http://codereview.chromium.org/10961015/diff/6002/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:29:
player.setDataSource(context, uri, headersMap);
On 2012/09/20 19:13:55, scherkus wrote:
> should these be 4 space indent?

Done.

http://codereview.chromium.org/10961015/diff/6002/media/base/android/media_pl...
File media/base/android/media_player_bridge.cc (right):

http://codereview.chromium.org/10961015/diff/6002/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:16: #include
"jni/MediaPlayerBridge_jni.h"
On 2012/09/20 19:13:55, scherkus wrote:
> does this have to go here or can it be in the above #include block in a->z
> ordering? (i.e., between base/ and media/)

Done.

[scherkus (not reviewing), 2012-09-21 16:17:03]

lgtm media/

[commit-bot: I haz the power, 2012-09-24 11:04:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/10961015/1012

[commit-bot: I haz the power, 2012-09-24 11:04:09]

Failed to apply patch for base/android/jni_generator/jni_generator.py:
While running patch -p1 --forward --force;
patching file base/android/jni_generator/jni_generator.py
Hunk #1 FAILED at 137.
1 out of 1 hunk FAILED -- saving rejects to file
base/android/jni_generator/jni_generator.py.rej

[commit-bot: I haz the power, 2012-09-24 11:44:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/10961015/9003

[commit-bot: I haz the power, 2012-09-24 11:51:44]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-09-24 12:48:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/10961015/9003

[commit-bot: I haz the power, 2012-09-24 14:52:09]

Change committed as 158275