Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12625005:
remove call to get android MediaPlayer class (Closed)
|
Created:
7 years, 9 months ago by qinmin Modified:
7 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionremove call to get android MediaPlayer class
use the global class variable defined in MediaPlayer_jni.h
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188293
Patch Set 1 #
Total comments: 1
Patch Set 2 : removing unneeded local javarefs elsewhere #
Total comments: 4
Patch Set 3 : moving getMetadata() call to java side #
Total comments: 8
Patch Set 4 : addressing comments #Patch Set 5 : remove useless . line #
Total comments: 10
Patch Set 6 : addressing comments #
Total comments: 8
Patch Set 7 : fixing nits #Patch Set 8 : fix findbug warning #
Messages
Total messages: 23 (0 generated)
PTAL
https://codereview.chromium.org/12625005/diff/1/media/base/android/media_play... File media/base/android/media_player_bridge.cc (left): https://codereview.chromium.org/12625005/diff/1/media/base/android/media_play... media/base/android/media_player_bridge.cc:332: GetClass(env, "android/media/MediaPlayer")); Sorry to do this to you, but since you're already updating this, can you remove the rest of the manual bindings? Marcus and I have started doing this and would appreciate your help. E.g. https://codereview.chromium.org/12604003/ and https://codereview.chromium.org/12638003/
sure, will do that in this change On 2013/03/11 19:58:40, Yaron wrote: > https://codereview.chromium.org/12625005/diff/1/media/base/android/media_play... > File media/base/android/media_player_bridge.cc (left): > > https://codereview.chromium.org/12625005/diff/1/media/base/android/media_play... > media/base/android/media_player_bridge.cc:332: GetClass(env, > "android/media/MediaPlayer")); > Sorry to do this to you, but since you're already updating this, can you remove > the rest of the manual bindings? Marcus and I have started doing this and would > appreciate your help. E.g. https://codereview.chromium.org/12604003/ and > https://codereview.chromium.org/12638003/
ok, remove all the other local references that I can find. There is also an internal change 33592 to clean up some other local references
https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... media/base/android/media_player_bridge.cc:332: env, g_MediaPlayer_clazz, "getMetadata", That's not the change I was asking for, but it's a good change regardless. You can leave the other updates in this patch, but I'd also like to see this function call replacted with usage of the JNI generator. Basically, after your patch this should only have things like: Java_MediaPlayer_fooBar(env, ..) and there shouldn't need to be any GetClass calls or CheckException calls. Since you're doing a little bit of work with a system class, you'll probably want to create a small java helper object and call a function on that. Let me know if you want more specifics
https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... media/base/android/media_player_bridge.cc:332: env, g_MediaPlayer_clazz, "getMetadata", There is some problem with the metadata class. It is not exposed as a public SDK API, but it is a public class in android. And the method is also public in MediaPlayer.java. So if I put this code logic into MediaPlayerBridge.java file, I will get a compile error. I will leave this as it is for now, but come up with another change to remove this dependency. On 2013/03/11 23:13:04, Yaron wrote: > That's not the change I was asking for, but it's a good change regardless. You > can leave the other updates in this patch, but I'd also like to see this > function call replacted with usage of the JNI generator. > > Basically, after your patch this should only have things like: > Java_MediaPlayer_fooBar(env, ..) > > and there shouldn't need to be any GetClass calls or CheckException calls. Since > you're doing a little bit of work with a system class, you'll probably want to > create a small java helper object and call a function on that. Let me know if > you want more specifics
https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... media/base/android/media_player_bridge.cc:332: env, g_MediaPlayer_clazz, "getMetadata", On 2013/03/12 16:36:24, qinmin wrote: > There is some problem with the metadata class. It is not exposed as a public SDK > API, but it is a public class in android. And the method is also public in > MediaPlayer.java. > So if I put this code logic into MediaPlayerBridge.java file, I will get a > compile error. > I will leave this as it is for now, but come up with another change to remove > this dependency. > > On 2013/03/11 23:13:04, Yaron wrote: > > That's not the change I was asking for, but it's a good change regardless. You > > can leave the other updates in this patch, but I'd also like to see this > > function call replacted with usage of the JNI generator. > > > > Basically, after your patch this should only have things like: > > Java_MediaPlayer_fooBar(env, ..) > > > > and there shouldn't need to be any GetClass calls or CheckException calls. > Since > > you're doing a little bit of work with a system class, you'll probably want to > > create a small java helper object and call a function on that. Let me know if > > you want more specifics > As discussed offline, this could be a sort of crashes in some versions of Android. I'd rather explicitly expose this error (via reflection) than hide the fact that we're essentially using reflection. As you mentioned offline, if there's a better way of doing it, I'm glad to pursue that, but having it being hidden is not a reason to keep this in C++
https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/5001/media/base/android/media_p... media/base/android/media_player_bridge.cc:332: env, g_MediaPlayer_clazz, "getMetadata", Ok, moved the everything to java and return a boolean array to the native side. And used reflection to check whether the methods are available in Java. On 2013/03/12 16:50:25, Yaron wrote: > On 2013/03/12 16:36:24, qinmin wrote: > > There is some problem with the metadata class. It is not exposed as a public > SDK > > API, but it is a public class in android. And the method is also public in > > MediaPlayer.java. > > So if I put this code logic into MediaPlayerBridge.java file, I will get a > > compile error. > > I will leave this as it is for now, but come up with another change to remove > > this dependency. > > > > On 2013/03/11 23:13:04, Yaron wrote: > > > That's not the change I was asking for, but it's a good change regardless. > You > > > can leave the other updates in this patch, but I'd also like to see this > > > function call replacted with usage of the JNI generator. > > > > > > Basically, after your patch this should only have things like: > > > Java_MediaPlayer_fooBar(env, ..) > > > > > > and there shouldn't need to be any GetClass calls or CheckException calls. > > Since > > > you're doing a little bit of work with a system class, you'll probably want > to > > > create a small java helper object and call a function on that. Let me know > if > > > you want more specifics > > > > As discussed offline, this could be a sort of crashes in some versions of > Android. I'd rather explicitly expose this error (via reflection) than hide the > fact that we're essentially using reflection. > > As you mentioned offline, if there's a better way of doing it, I'm glad to > pursue that, but having it being hidden is not a reason to keep this in C++
https://codereview.chromium.org/12625005/diff/11001/content/browser/android/d... File content/browser/android/download_controller_android_impl.cc (left): https://codereview.chromium.org/12625005/diff/11001/content/browser/android/d... content/browser/android/download_controller_android_impl.cc:38: const char kDownloadControllerClassPathName[] = You can revert this. I've fixed this file in https://codereview.chromium.org/12796004/ https://codereview.chromium.org/12625005/diff/11001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/12625005/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:39: private static boolean[] getMetadata(MediaPlayer player) { Please document the return type. https://codereview.chromium.org/12625005/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:56: getMetadata.setAccessible(true); Nit: remove (already on L44) https://codereview.chromium.org/12625005/diff/11001/media/base/android/media_... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/11001/media/base/android/media_... media/base/android/media_player_bridge.cc:21: using base::android::GetClass; Update these. Please remove unnecessary ones and inline ones that wouldn't introduce new line breaks
https://codereview.chromium.org/12625005/diff/11001/content/browser/android/d... File content/browser/android/download_controller_android_impl.cc (left): https://codereview.chromium.org/12625005/diff/11001/content/browser/android/d... content/browser/android/download_controller_android_impl.cc:38: const char kDownloadControllerClassPathName[] = On 2013/03/12 21:51:54, Yaron wrote: > You can revert this. I've fixed this file in > https://codereview.chromium.org/12796004/ Done. https://codereview.chromium.org/12625005/diff/11001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/12625005/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:39: private static boolean[] getMetadata(MediaPlayer player) { On 2013/03/12 21:51:54, Yaron wrote: > Please document the return type. Done. https://codereview.chromium.org/12625005/diff/11001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:56: getMetadata.setAccessible(true); On 2013/03/12 21:51:54, Yaron wrote: > Nit: remove (already on L44) Done. https://codereview.chromium.org/12625005/diff/11001/media/base/android/media_... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/11001/media/base/android/media_... media/base/android/media_player_bridge.cc:21: using base::android::GetClass; On 2013/03/12 21:51:54, Yaron wrote: > Update these. Please remove unnecessary ones and inline ones that wouldn't > introduce new line breaks Done.
lgtm but please wait for bulach I wish there was a better way to get this information. It seems like those APIs should be public. Otherwise, I don't see how we can implement those functions. One possible improvement would be to return a handle to an inner class on the java side instead of array of booleans. Rather than having an array, you'd call named accessors on that class from the native side. I assume there actually are cases where you can't seek back/forward or pause?
On 2013/03/13 01:47:54, Yaron wrote: > lgtm but please wait for bulach > > I wish there was a better way to get this information. It seems like those APIs > should be public. Otherwise, I don't see how we can implement those functions. Those functions are use by both videoview.java and android browser, so it should be fairly supported on other custom ROMs. > > One possible improvement would be to return a handle to an inner class on the > java side instead of array of booleans. Rather than having an array, you'd call > named accessors on that class from the native side. > > I assume there actually are cases where you can't seek back/forward or pause? Yes, on streaming cases, pause/seek button should not work. Returning an array is mostly a performance concern. It needs only one call, or otherwise i need several calls and some java member variables.
lgtm, thanks Min! some other suggestions and echoing Yaron's suggestions on using an internal class... how often does these methods need to be called? locking / releasing the boolean array is also a potentially expensive operation, so the trade-off in terms of performance may not be too bad considering the improved readability, wdyt? there are some microbenchmarks we may want to expand in base/android/jni_android_unittests.cc, JNIAndroidMicrobenchmark.MethodId... https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:64: env, g_SQLiteCursor_clazz, "<init>", "(I)V"); while at it... :) this is not a system class, but rather our own: chrome/android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java in which case, we should have a static factory method rather than inspect the constructor.. and 58-75 could be simplified into: SQLiteCursor* cursor = new SQLiteCursor(...); ScopedJavaLocalRef<jobjec> ret(env, Java_SQLiteCursor_Create(reinterpret_cast<jint>(cursor)); return ret; https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:95: ScopedJavaLocalRef<jclass> sclass = GetClass(env, "java/lang/String"); 94-103 could use base/android/jni_array.h and just: return ToJavaArrayOfStrings(env, column_names_); https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:138: ScopedJavaLocalRef<jbyteArray> jb(env, env->NewByteArray(blob.size())); ditto, 138-140: return ToJavaByteArray(env, blob.data(), blob.size()); https://codereview.chromium.org/12625005/diff/20001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/12625005/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:42: * The 3rd boolean shows whether the player can seek backward. the knowledge of which boolean is what is now distributed across two layers, and the fact that they are positional makes it harder to follow, plus the documentation can't be enforced. :) the overhead of letting it be compiled would be fairly minimal, and the readability would greatly improve... private static class Metadata { private final boolean mCanPause; private final boolean mCanSeekForward; private final boolean mCanSeekBackward; private Metadata(boolean canPause, boolean canSeekForward, boolean canSeekBackward) { mCanPause = canPause; mCanSeekForward = canSeekForward; mCanSeekBackward = canSeekBackward; } @CalledByNative private boolean canPause() { return mCanPause; } @CalledByNative private boolean canSeekForward() { return mCanSeekForward; } @CalledByNative private boolean canSeekBackward() { return mCanSeekBackward; } } then in 63-69: return new Metadata(..., ..., ...); then in the native side, just use "Java_Metadata_canSeekForward", etc... I agree there are slightly more lines of code, but there will be fewer comments :) and its overhead shouldn't be too significant anyways... wdyt? https://codereview.chromium.org/12625005/diff/20001/media/base/android/media_... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/20001/media/base/android/media_... media/base/android/media_player_bridge.cc:327: can_seek_backward_ = bools[2]; yeah, here it'd be clearer to just: ScopedJavaLocalRef<jobject> metadata = Java_MediaPlayerBridge_getMetadata(env, j_media_player_.obj()); can_pause_ = Java_Metadata_canPause(env, metadata.obj()); can_seek_forward_ = Java_Metadata_canSeekForward(env, metadata.obj()); can_seek_backward_ = Java_Metadata_canSeekBackward(env, metadata.obj()); no need to mess up with the boolean array, etc..
https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:64: env, g_SQLiteCursor_clazz, "<init>", "(I)V"); On 2013/03/13 15:06:10, bulach wrote: > while at it... :) > this is not a system class, but rather our own: > chrome/android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java > > in which case, we should have a static factory method rather than inspect the > constructor.. > > and 58-75 could be simplified into: > > SQLiteCursor* cursor = new SQLiteCursor(...); > ScopedJavaLocalRef<jobjec> ret(env, > Java_SQLiteCursor_Create(reinterpret_cast<jint>(cursor)); > return ret; Good point, Done. https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:95: ScopedJavaLocalRef<jclass> sclass = GetClass(env, "java/lang/String"); Good catch, done. On 2013/03/13 15:06:10, bulach wrote: > 94-103 could use base/android/jni_array.h and just: > > return ToJavaArrayOfStrings(env, column_names_); https://codereview.chromium.org/12625005/diff/20001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:138: ScopedJavaLocalRef<jbyteArray> jb(env, env->NewByteArray(blob.size())); vector::data() is introduced in C++11, use &blob[0] instead. On 2013/03/13 15:06:10, bulach wrote: > ditto, 138-140: > > return ToJavaByteArray(env, blob.data(), blob.size()); Done. https://codereview.chromium.org/12625005/diff/20001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/12625005/diff/20001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:42: * The 3rd boolean shows whether the player can seek backward. On 2013/03/13 15:06:10, bulach wrote: > the knowledge of which boolean is what is now distributed across two layers, and > the fact that they are positional makes it harder to follow, plus the > documentation can't be enforced. :) > the overhead of letting it be compiled would be fairly minimal, and the > readability would greatly improve... > > private static class Metadata { > private final boolean mCanPause; > private final boolean mCanSeekForward; > private final boolean mCanSeekBackward; > private Metadata(boolean canPause, boolean canSeekForward, boolean > canSeekBackward) { > mCanPause = canPause; > mCanSeekForward = canSeekForward; > mCanSeekBackward = canSeekBackward; > } > > @CalledByNative > private boolean canPause() { return mCanPause; } > @CalledByNative > private boolean canSeekForward() { return mCanSeekForward; } > @CalledByNative > private boolean canSeekBackward() { return mCanSeekBackward; } > } > > > > then in 63-69: > return new Metadata(..., ..., ...); > > then in the native side, just use "Java_Metadata_canSeekForward", etc... > I agree there are slightly more lines of code, but there will be fewer comments > :) and its overhead shouldn't be too significant anyways... > > wdyt? Done. https://codereview.chromium.org/12625005/diff/20001/media/base/android/media_... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/20001/media/base/android/media_... media/base/android/media_player_bridge.cc:327: can_seek_backward_ = bools[2]; On 2013/03/13 15:06:10, bulach wrote: > yeah, here it'd be clearer to just: > > ScopedJavaLocalRef<jobject> metadata = > Java_MediaPlayerBridge_getMetadata(env, j_media_player_.obj()); > can_pause_ = Java_Metadata_canPause(env, metadata.obj()); > can_seek_forward_ = Java_Metadata_canSeekForward(env, metadata.obj()); > can_seek_backward_ = Java_Metadata_canSeekBackward(env, metadata.obj()); > > no need to mess up with the boolean array, etc.. Done.
+sky for OWNERS in chrome/browser/history
lgtm
just a nit. Please also update CL description with SQLCursor reference https://codereview.chromium.org/12625005/diff/28001/chrome/browser/history/an... File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/12625005/diff/28001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:18: using base::android::GetClass; Please remove these 3
lgtm, it looks so much nicer!!! thanks a ton Min! just one question and a couple of tiny nits, feel free to go ahead.. https://codereview.chromium.org/12625005/diff/28001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java (right): https://codereview.chromium.org/12625005/diff/28001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java:36: SQLiteCursor(int nativeSQLiteCursor) { nit: could make this private (and probably the class itself, since it's only called by native?) https://codereview.chromium.org/12625005/diff/28001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/12625005/diff/28001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:22: private static class AllowedOperations implements Serializable { is Serializable needed? https://codereview.chromium.org/12625005/diff/28001/media/base/android/media_... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/28001/media/base/android/media_... media/base/android/media_player_bridge.cc:322: base::android::CheckException(env); nit: the generated code already calls CheckException :) (same for 329)
https://codereview.chromium.org/12625005/diff/28001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java (right): https://codereview.chromium.org/12625005/diff/28001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/database/SQLiteCursor.java:36: SQLiteCursor(int nativeSQLiteCursor) { On 2013/03/14 09:59:07, bulach wrote: > nit: could make this private (and probably the class itself, since it's only > called by native?) Done. https://codereview.chromium.org/12625005/diff/28001/chrome/browser/history/an... File chrome/browser/history/android/sqlite_cursor.cc (right): https://codereview.chromium.org/12625005/diff/28001/chrome/browser/history/an... chrome/browser/history/android/sqlite_cursor.cc:18: using base::android::GetClass; On 2013/03/13 22:17:51, Yaron wrote: > Please remove these 3 Done. https://codereview.chromium.org/12625005/diff/28001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/12625005/diff/28001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:22: private static class AllowedOperations implements Serializable { no, removed On 2013/03/14 09:59:07, bulach wrote: > is Serializable needed? https://codereview.chromium.org/12625005/diff/28001/media/base/android/media_... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/12625005/diff/28001/media/base/android/media_... media/base/android/media_player_bridge.cc:322: base::android::CheckException(env); thanks, removed, also remove the checkException below On 2013/03/14 09:59:07, bulach wrote: > nit: the generated code already calls CheckException :) (same for 329)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/12625005/38001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/12625005/54001
Message was sent while issue was closed.
Change committed as 188293 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
