Refactor ContentViewClient (1/6)

content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java

Index: content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java b/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
index 262584eaae387141ac29ac998d09196767c8d5f5..0b6fb3b58d1443f40d6e4ca9f8c46a409a264653 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
@@ -113,6 +113,21 @@ public class ContentVideoView extends FrameLayout
         }
     }
 
+    private static final ContentVideoViewEmbedder NULL_VIDEO_EMBEDDER =

[boliu, 2016/09/26 21:25:25]

why is needed now, but not before? seems like native side has a null check for
null embedder already that it won't even call into java, so this should never
happen?

[Jinsuk Kim, 2016/09/27 10:08:59]

OverlayPanelContent.java was passing an empty impl of ContentVideoViewEmbedder -
in fact this has a bug but fixed in the new patch - I think it would be better
to have the definition here rather than adding native part, going up all the way
to Java layer only to get the empty impl. Also there can be other instances
which also injects an empty impl, for which having it here will be better.

[boliu, 2016/09/27 17:24:44]

I can't tell if you agree with me or not..

native ContentVideoView::CreateJavaObject already has a null check for the
embedder, which means in the java ContentVideoView constructor, the embedder can
never be null, so this NULL_VIDEO_EMBEDDER will never be needed.

Or did I miss something?

+            new ContentVideoViewEmbedder() {
+                @Override
+                public void enterFullscreenVideo(View view, boolean isVideoLoaded) {}
+
+                @Override
+                public void fullscreenVideoLoaded() {}
+
+                @Override
+                public void exitFullscreenVideo() {}
+
+                @Override
+                public void setSystemUiVisibility(boolean enterFullscreen) {}
+    };
+
     private final Runnable mExitFullscreenRunnable = new Runnable() {
         @Override
         public void run() {
@@ -124,7 +139,7 @@ public class ContentVideoView extends FrameLayout
             ContentVideoViewEmbedder embedder, int videoWidth, int videoHeight) {
         super(context);
         mNativeContentVideoView = nativeContentVideoView;
-        mEmbedder = embedder;
+        mEmbedder = (embedder != null) ? embedder : NULL_VIDEO_EMBEDDER;

[boliu, 2016/09/26 21:25:25]

nit: no need for brackets

[Jinsuk Kim, 2016/09/27 10:08:59]

Done.

         mUmaRecorded = false;
         mPossibleAccidentalChange = false;
         mIsVideoLoaded = videoWidth > 0 && videoHeight > 0;
@@ -282,10 +297,10 @@ public class ContentVideoView extends FrameLayout
      */
     @CalledByNative
     private static ContentVideoView createContentVideoView(ContentViewCore contentViewCore,
-            long nativeContentVideoView, int videoWidth, int videoHeight) {
+            ContentVideoViewEmbedder embedder, long nativeContentVideoView,
+            int videoWidth, int videoHeight) {
         ThreadUtils.assertOnUiThread();
         Context context = contentViewCore.getContext();
-        ContentVideoViewEmbedder embedder = contentViewCore.getContentVideoViewEmbedder();
         ContentVideoView videoView = new ContentVideoView(
                 context, nativeContentVideoView, embedder, videoWidth, videoHeight);
         return videoView;