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/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::GetApplicationContext;
 using base::android::ScopedJavaGlobalRef;
 
 namespace content {
 
 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)));
-  } else {
-    // Just ask video view to reopen the video.
-    Java_ContentVideoView_openVideo(AttachCurrentThread(),
-                                    j_content_video_view_.obj());
-  }
+void ContentVideoView::CreateContentVideoView(

[joth, 2013/04/05 01:56:40]

what if j_content_video_view_ were not null? I think we should call
DestroyContentVideoView() here first to be safe?
(maybe worth a tests case to cover that too)

- we could make this safer by having a wrapper object that holds the java ref
and also does Java_ContentVideoView_destroyContentVideoView() in its destructor,
and then just hold a scoped_ptr to that wrapper inside this class.

Or even better... why not call Java_ContentVideoView_createContentVideoView from
ContentVideoView c'tor, and destroyContentVideoView from the d'tor, and remove
these two explicit create/destroy methods? that way, the native and java objects
have the same lifetime, and it's much easier to see what's going on.

[michaelbai, 2013/04/05 21:06:56]

if j_content_video_view_ was not null, the same object will be returned (see
java file), the reference count of the object was just decreased/increased
again. it seemed no side effect.
Talked to min, there is a feature here, In the case, there are multiple video
need to play, we don't want to exit full screen mode, then enter again. the
previous code Java_ContentVideoView_openVideo is for this. so it might not good
to put Create/DestoryContentVideoView in c'tor/d'tor.

[joth, 2013/04/08 18:47:34]

Right, the java-side object is a singleton but the native side isn't, so can't
have same lifetimes.

+    ScopedJavaLocalRef<jobject> content_video_view_client) {
+  j_content_video_view_.Reset(
+      Java_ContentVideoView_createContentVideoView(AttachCurrentThread(),
+          GetApplicationContext(), reinterpret_cast<int>(this),
+          content_video_view_client.obj()));
 }
 
 void ContentVideoView::DestroyContentVideoView() {
   if (!j_content_video_view_.is_null()) {
-    Java_ContentVideoView_destroyContentVideoView(AttachCurrentThread());
+    Java_ContentVideoView_destroyContentVideoView(AttachCurrentThread(),
+        j_content_video_view_.obj());
     j_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);
   }
@@ skipping 79 matching lines @@
 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