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..9ce14b15e92615fc953c96ac01011f264606d322 100644
--- a/content/browser/android/content_video_view.cc
+++ b/content/browser/android/content_video_view.cc
@@ -14,6 +14,7 @@
 
 using base::android::AttachCurrentThread;
 using base::android::CheckException;
+using base::android::GetApplicationContext;
 using base::android::ScopedJavaGlobalRef;
 
 namespace content {
@@ -30,21 +31,18 @@ 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();
   }
 }