Refactoring ContentVideoViewContextDelegate.java

content/browser/android/content_video_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "content/common/android/surface_texture_peer.h"
 #include "content/public/common/content_switches.h"
 #include "jni/ContentVideoView_jni.h"
 
 using base::android::AttachCurrentThread;
 using base::android::CheckException;
 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);
 }
 
 ContentVideoView::ContentVideoView(MediaPlayerManagerAndroid* manager)
     : manager_(manager) {
 }
 
 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 {
   media::MediaPlayerBridge* player = manager_->GetFullscreenPlayer();
   return player ? player->GetVideoWidth() : 0;
 }
 
 int ContentVideoView::GetVideoHeight(JNIEnv*, jobject obj) const {
   media::MediaPlayerBridge* player = manager_->GetFullscreenPlayer();
   return player ? player->GetVideoHeight() : 0;
@@ skipping 22 matching lines @@
   manager_->FullscreenPlayerPlay();
 }
 
 void ContentVideoView::Pause(JNIEnv*, jobject obj) {
   manager_->FullscreenPlayerPause();
 }
 
 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,
                                   jobject surface) {
   manager_->SetVideoSurface(surface);
 }
 
 void ContentVideoView::UpdateMediaMetadata(JNIEnv* env, jobject obj) {
   media::MediaPlayerBridge* player = manager_->GetFullscreenPlayer();
   if (player && player->prepared())
     Java_ContentVideoView_updateMediaMetadata(
         env, obj, player->GetVideoWidth(), player->GetVideoHeight(),
         player->GetDuration().InMilliseconds(), player->can_pause(),
         player->can_seek_forward(), player->can_seek_backward());
 }
 
 }  // namespace content