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..1f06a5eeceffb0d7f04cdf1dff5f759e40d77405 100644
--- a/content/browser/android/content_video_view.cc
+++ b/content/browser/android/content_video_view.cc
@@ -4,7 +4,6 @@
 
 #include "content/browser/android/content_video_view.h"
 
-#include "base/android/jni_android.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "content/browser/android/media_player_manager_android.h"
@@ -18,72 +17,97 @@ using base::android::ScopedJavaGlobalRef;
 
 namespace content {
 
+namespace {
+// There can only be one content video view at a time, this holds onto that
+// singleton instance.
+ContentVideoView* g_content_video_view = NULL;
+
+}  // namespace
+
+static jobject GetSingletonJavaContentVideoView(JNIEnv*env, jclass) {
+  if (g_content_video_view)
+    return g_content_video_view->GetJavaObject(env).Release();
+  else
+    return NULL;
+}
+
 bool ContentVideoView::RegisterContentVideoView(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
-ContentVideoView::ContentVideoView(MediaPlayerManagerAndroid* manager)
+ContentVideoView::ContentVideoView(
+    const ScopedJavaLocalRef<jobject>& context,
+    const ScopedJavaLocalRef<jobject>& client,
+    MediaPlayerManagerAndroid* manager)
     : manager_(manager) {
+  DCHECK(!g_content_video_view);
+  JNIEnv *env = AttachCurrentThread();
+  j_content_video_view_.reset(new JavaObjectWeakGlobalRef(env,

[joth, 2013/05/15 22:38:57]

j_content_video_view_ = JavaObjectWeakGlobalRef(env,
Java_ContentVideoView_createContentVideoView(env, context.obj())

[michaelbai, 2013/05/22 18:08:39]

Done.

+      Java_ContentVideoView_createContentVideoView(env, context.obj(),
+          reinterpret_cast<int>(this), client.obj()).obj()));
+  g_content_video_view = this;

[joth, 2013/05/15 22:38:57]

problem: at this point no one has a strong-ref to the java side object, so it
could get GC'd before anyone actually tries to use it / add it to the view tree,
etc.
To solve, rather than construct it here, maybe do it lazily in OpenVideo?
OR perhaps clearer, retain a 'private static ContentVideoView sInstance' on the
java side too, and add comments to indicate the ownership model. ? -
destroyContentVideoView() would null out the static instance pointer.

[michaelbai, 2013/05/22 18:08:39]

In the ctor of ContentVideoView(java side), mClient.onShowCustomView is called,
and Java object is supposed be add to the Activity's view tree, if not, it
should be gc-ed, that is our goal, isn't it?


On 2013/05/15 22:38:57, joth wrote:

[joth, 2013/06/03 21:22:04]

Yes, you're right - I missed a step there.

 }
 
 ContentVideoView::~ContentVideoView() {

[joth, 2013/05/15 22:38:57]

do we need to check that ExitFullscreen has been done? maybe DCHECK
j_content_video_view_ is now null?

[michaelbai, 2013/05/22 18:08:39]

Done.

-  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)));
-  } else {
-    // Just ask video view to reopen the video.
-    Java_ContentVideoView_openVideo(AttachCurrentThread(),
-                                    j_content_video_view_.obj());
-  }
+  DCHECK(g_content_video_view);
+  g_content_video_view = NULL;
 }
 
-void ContentVideoView::DestroyContentVideoView() {
-  if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_destroyContentVideoView(AttachCurrentThread());
-    j_content_video_view_.Reset();
-  }
+void ContentVideoView::OpenVideo() {
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetJavaObject(env);
+  if (!content_video_view.is_null())
+    Java_ContentVideoView_openVideo(env, content_video_view.obj());
 }
 
 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 = GetJavaObject(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 = GetJavaObject(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 = GetJavaObject(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 = GetJavaObject(env);
+  if (!content_video_view.is_null())
+    Java_ContentVideoView_onPlaybackComplete(env, content_video_view.obj());
+}
+
+void ContentVideoView::OnExitFullscreen() {
+  JNIEnv *env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> content_video_view = GetJavaObject(env);
+  if (!content_video_view.is_null()) {
+    Java_ContentVideoView_destroyContentVideoView(env,
+        content_video_view.obj());
   }

[joth, 2013/05/15 22:38:57]

j_content_video_view_.reset() ?

[michaelbai, 2013/05/22 18:08:39]

Done.

 }
 
 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 = GetJavaObject(env);
+  if (!content_video_view.is_null())
+    UpdateMediaMetadata(env, content_video_view.obj());
 }
 
 int ContentVideoView::GetVideoWidth(JNIEnv*, jobject obj) const {
@@ -126,7 +150,6 @@ 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();
 }
 
 void ContentVideoView::SetSurface(JNIEnv* env, jobject obj,
@@ -137,10 +160,17 @@ void ContentVideoView::SetSurface(JNIEnv* env, jobject obj,
 void ContentVideoView::UpdateMediaMetadata(JNIEnv* env, jobject obj) {
   media::MediaPlayerBridge* player = manager_->GetFullscreenPlayer();
   if (player && player->prepared())
-    Java_ContentVideoView_updateMediaMetadata(
+    Java_ContentVideoView_onUpdateMediaMetadata(
         env, obj, player->GetVideoWidth(), player->GetVideoHeight(),
         player->GetDuration().InMilliseconds(), player->can_pause(),
         player->can_seek_forward(), player->can_seek_backward());
 }
 
+ScopedJavaLocalRef<jobject> ContentVideoView::GetJavaObject(JNIEnv* env) {
+  if (j_content_video_view_.get()) {
+    return j_content_video_view_->get(env);
+  }
+  return ScopedJavaLocalRef<jobject>();
+}
+
 }  // namespace content