Refactoring ContentVideoViewContextDelegate.java

content/browser/android/content_video_view.cc

Index: content/browser/android/content_video_view.cc
diff --git a/content/browser/android/content_video_view.cc b/content/browser/android/content_video_view.cc
index 158749b75be6df302be718e410c34d0956351213..efbb3b7f23fb34f09c48d8b78fc234da27cba26c 100644
--- a/content/browser/android/content_video_view.cc
+++ b/content/browser/android/content_video_view.cc
@@ -4,7 +4,7 @@
 
 #include "content/browser/android/content_video_view.h"
 
-#include "base/android/jni_android.h"
+#include "base/android/jni_helper.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "content/browser/android/media_player_manager_android.h"
@@ -18,6 +18,20 @@ using base::android::ScopedJavaGlobalRef;
 
 namespace content {
 
+// WeakRef Reference to the Java object.

[joth, 2013/04/12 17:56:24]

Add "there can only be one content video view at a time, this holds onto that
singleton instance"

[michaelbai, 2013/04/18 18:21:59]

Done.

+static JavaObjectWeakGlobalRef* g_content_video_view_ = NULL;

[joth, 2013/04/12 17:56:24]

no trailing _ on globals

[michaelbai, 2013/04/18 18:21:59]

Done.

+
+static ScopedJavaLocalRef<jobject> GetContentVideoView(JNIEnv* env) {

[joth, 2013/04/12 17:56:24]

instead of static, put these two  into anon namespace

[michaelbai, 2013/04/18 18:21:59]

Done.

+  if (g_content_video_view_)
+    return g_content_video_view_->get(env);
+  else
+    return ScopedJavaLocalRef<jobject>();

[joth, 2013/04/12 17:56:24]

I think this whole file will be easier to read if you leave the WeakRef as a
member of the native ContentVideoView class, and stash a raw (non-owning)
ContentVideoView* pointer here as the way to find the singleton instance. (i.e.
set it in the c'tor, clear in d'tor, with appropriate DCHECKs to ensure the
singleton invariant)

[michaelbai, 2013/04/18 18:21:59]

Done.

+}
+
+static jobject GetSingletonJavaContentVideoView(JNIEnv*env, jclass) {
+  return GetContentVideoView(env).obj();

[joth, 2013/04/12 17:56:24]

I think you need to use Release() here, otherwise the jobject returned is
referring to the one in the temporary ScopedLocalJaveRef that will be destructed
before the caller has got the jobject return value.

[michaelbai, 2013/04/18 18:21:59]

Done.

+}
+
 bool ContentVideoView::RegisterContentVideoView(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
@@ -30,60 +44,70 @@ ContentVideoView::~ContentVideoView() {
   DestroyContentVideoView();
 }
 
-void ContentVideoView::CreateContentVideoView() {
-  if (j_content_video_view_.is_null()) {
-    JNIEnv* env = AttachCurrentThread();
-    j_content_video_view_.Reset(Java_ContentVideoView_createContentVideoView(
-        env, reinterpret_cast<jint>(this)));
+void ContentVideoView::CreateContentVideoView(
+    ScopedJavaLocalRef<jobject> context,
+    ScopedJavaLocalRef<jobject> content_video_view_client) {
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (content_video_view.is_null()) {
+    g_content_video_view_ = new JavaObjectWeakGlobalRef(env,

[joth, 2013/04/12 17:56:24]

g_content_video_view_ might already hold an instance?

+        Java_ContentVideoView_createContentVideoView(env, context.obj(),
+            reinterpret_cast<int>(this),
+            content_video_view_client.obj()).obj());
   } else {
-    // Just ask video view to reopen the video.
-    Java_ContentVideoView_openVideo(AttachCurrentThread(),
-                                    j_content_video_view_.obj());
+    Java_ContentVideoView_openVideo(env, content_video_view.obj());

[joth, 2013/04/12 17:56:24]

in the else branch we ignore the content_video_view_client. Is that right?

Cramming these two flows into one method (called create, but that may not
actually do any create) is increasingly awkward. IMO this would be clearer to
have the native and java objects be same lifetime, and leave it to the user of
ContentVideoView to make the choice about re-use vs making a new one (as it
would know if the client has changed or not, etc)

   }
 }
 
 void ContentVideoView::DestroyContentVideoView() {
-  if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_destroyContentVideoView(AttachCurrentThread());
-    j_content_video_view_.Reset();
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (!content_video_view.is_null()) {
+    Java_ContentVideoView_destroyContentVideoView(env,
+        content_video_view.obj());
+    g_content_video_view_->reset();
   }
 }
 
 void ContentVideoView::OnMediaPlayerError(int error_type) {
-  if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_onMediaPlayerError(AttachCurrentThread(),
-                                             j_content_video_view_.obj(),
-                                             error_type);
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (!content_video_view.is_null()) {
+    Java_ContentVideoView_onMediaPlayerError(env, content_video_view.obj(),
+        error_type);
   }
 }
 
 void ContentVideoView::OnVideoSizeChanged(int width, int height) {
-  if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_onVideoSizeChanged(AttachCurrentThread(),
-                                             j_content_video_view_.obj(),
-                                             width,
-                                             height);
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (!content_video_view.is_null()) {
+    Java_ContentVideoView_onVideoSizeChanged(env, content_video_view.obj(),
+        width, height);
   }
 }
 
 void ContentVideoView::OnBufferingUpdate(int percent) {
-  if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_onBufferingUpdate(AttachCurrentThread(),
-                                            j_content_video_view_.obj(),
-                                            percent);
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (!content_video_view.is_null()) {
+    Java_ContentVideoView_onBufferingUpdate(env, content_video_view.obj(),
+        percent);
   }
 }
 
 void ContentVideoView::OnPlaybackComplete() {
-  if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_onPlaybackComplete(AttachCurrentThread(),
-                                             j_content_video_view_.obj());
-  }
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (!content_video_view.is_null())
+    Java_ContentVideoView_onPlaybackComplete(env, content_video_view.obj());
 }
 
 void ContentVideoView::UpdateMediaMetadata() {
-  if (!j_content_video_view_.is_null())
-    UpdateMediaMetadata(AttachCurrentThread(), j_content_video_view_.obj());
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetContentVideoView(env);
+  if (!content_video_view.is_null())
+    UpdateMediaMetadata(env, content_video_view.obj());
 }
 
 int ContentVideoView::GetVideoWidth(JNIEnv*, jobject obj) const {
@@ -126,7 +150,7 @@ void ContentVideoView::Pause(JNIEnv*, jobject obj) {
 void ContentVideoView::ExitFullscreen(
     JNIEnv*, jobject, jboolean release_media_player) {
   manager_->ExitFullscreen(release_media_player);
-  j_content_video_view_.Reset();
+  g_content_video_view_->reset();
 }
 
 void ContentVideoView::SetSurface(JNIEnv* env, jobject obj,