Refactoring ContentVideoViewContextDelegate.java

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 e14bf04f8f95cf466bb51b9c4a1fea93a6af61fa..6a7970c73acac03feadd144e1799f1edf6a87786 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
@@ -32,6 +32,7 @@ import java.lang.ref.WeakReference;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
+import org.chromium.base.ThreadUtils;
 import org.chromium.content.common.IChildProcessService;
 import org.chromium.content.R;
 
@@ -66,28 +67,28 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
     private static final int STATE_PAUSED             = 2;
     private static final int STATE_PLAYBACK_COMPLETED = 3;
 
-    private SurfaceHolder mSurfaceHolder = null;
-    private int mVideoWidth = 0;
-    private int mVideoHeight = 0;
+    private SurfaceHolder mSurfaceHolder;
+    private int mVideoWidth;
+    private int mVideoHeight;
     private int mCurrentBufferPercentage;
     private int mDuration;
-    private MediaController mMediaController = null;
+    private MediaController mMediaController;
     private boolean mCanPause;
     private boolean mCanSeekBack;
     private boolean mCanSeekForward;
 
     // Native pointer to C++ ContentVideoView object.
-    private int mNativeContentVideoView = 0;
+    private int mNativeContentVideoView;
 
     // webkit should have prepared the media
     private int mCurrentState = STATE_IDLE;
 
     // Strings for displaying media player errors
-    static String mPlaybackErrorText;
-    static String mUnknownErrorText;
-    static String mErrorButton;
-    static String mErrorTitle;
-    static String mVideoLoadingText;
+    private String mPlaybackErrorText;
+    private String mUnknownErrorText;
+    private String mErrorButton;
+    private String mErrorTitle;
+    private String mVideoLoadingText;
 
     // This view will contain the video.
     private VideoSurfaceView mVideoSurfaceView;
@@ -95,16 +96,9 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
     // Progress view when the video is loading.
     private View mProgressView;
 
-    private Surface mSurface = null;
+    private Surface mSurface;
 
-    // There are can be at most 1 fullscreen video
-    // TODO(qinmin): will change this once  we move the creation of this class
-    // to the host application
-    private static ContentVideoView sContentVideoView = null;
-
-    // The delegate will follow sContentVideoView. We would need to
-    // move this to an instance variable if we allow multiple ContentVideoViews.
-    private static ContentVideoViewContextDelegate sDelegate = null;
+    private ContentVideoViewClient mClient;
 
     private class VideoSurfaceView extends SurfaceView {
 
@@ -136,7 +130,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         private ProgressBar mProgressBar;
         private TextView mTextView;
 
-        public ProgressView(Context context) {
+        public ProgressView(Context context, String videoLoadingText) {
             super(context);
             setOrientation(LinearLayout.VERTICAL);
             setLayoutParams(new LinearLayout.LayoutParams(
@@ -144,7 +138,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
                     LinearLayout.LayoutParams.WRAP_CONTENT));
             mProgressBar = new ProgressBar(context, null, android.R.attr.progressBarStyleLarge);
             mTextView = new TextView(context);
-            mTextView.setText(mVideoLoadingText);
+            mTextView.setText(videoLoadingText);
             addView(mProgressBar);
             addView(mTextView);
         }
@@ -183,22 +177,21 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         }
     };
 
-    public ContentVideoView(Context context) {
-        this(context, 0);
-    }
-
-    private ContentVideoView(Context context, int nativeContentVideoView) {
+    private ContentVideoView(Context context, int nativeContentVideoView,
+            ContentVideoViewClient client) {
         super(context);
-        initResources(context);
-
-        if (nativeContentVideoView == 0) return;
         mNativeContentVideoView = nativeContentVideoView;
-
+        mClient = client;
+        initResources(context);
         mCurrentBufferPercentage = 0;
         mVideoSurfaceView = new VideoSurfaceView(context);
+        mClient.onShowCustomView(this);
+        setBackgroundColor(Color.BLACK);
+        showContentVideoView();
+        setVisibility(View.VISIBLE);
     }
 
-    private static void initResources(Context context) {
+    private void initResources(Context context) {
         if (mPlaybackErrorText != null) return;
         mPlaybackErrorText = context.getString(
                 org.chromium.content.R.string.media_player_error_text_invalid_progressive_playback);
@@ -218,11 +211,11 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
                 ViewGroup.LayoutParams.WRAP_CONTENT,
                 Gravity.CENTER);
         this.addView(mVideoSurfaceView, layoutParams);
-        View progressView = sDelegate.getVideoLoadingProgressView();
+        View progressView = mClient.getVideoLoadingProgressView();
         if (progressView != null) {
             mProgressView = progressView;
         } else {
-            mProgressView = new ProgressView(getContext());
+            mProgressView = new ProgressView(getContext(), mVideoLoadingText);
         }
         this.addView(mProgressView, layoutParams);
         mVideoSurfaceView.setZOrderOnTop(true);
@@ -375,10 +368,10 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
     }
 
     @CalledByNative
-    public void openVideo() {
+    private void openVideo() {
         if (mSurfaceHolder != null) {
             mCurrentState = STATE_IDLE;
-            setMediaController(new FullScreenMediaController(sDelegate.getContext(), this));
+            setMediaController(new FullScreenMediaController(getContext(), this));
             if (mNativeContentVideoView != 0) {
                 nativeUpdateMediaMetadata(mNativeContentVideoView);
             }
@@ -475,6 +468,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         return (mCurrentState != STATE_ERROR && mCurrentState != STATE_IDLE);
     }
 
+    @Override
     public void start() {
         if (isInPlaybackState()) {
             if (mNativeContentVideoView != 0) {
@@ -484,6 +478,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         }
     }
 
+    @Override
     public void pause() {
         if (isInPlaybackState()) {
             if (isPlaying()) {
@@ -496,6 +491,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
     }
 
     // cache duration as mDuration for faster access
+    @Override
     public int getDuration() {
         if (isInPlaybackState()) {
             if (mDuration > 0) {
@@ -512,6 +508,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         return mDuration;
     }
 
+    @Override
     public int getCurrentPosition() {
         if (isInPlaybackState() && mNativeContentVideoView != 0) {
             return nativeGetCurrentPosition(mNativeContentVideoView);
@@ -519,47 +516,44 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         return 0;
     }
 
+    @Override
     public void seekTo(int msec) {
         if (mNativeContentVideoView != 0) {
             nativeSeekTo(mNativeContentVideoView, msec);
         }
     }
 
+    @Override
     public boolean isPlaying() {
         return mNativeContentVideoView != 0 && nativeIsPlaying(mNativeContentVideoView);
     }
 
+    @Override
     public int getBufferPercentage() {
         return mCurrentBufferPercentage;
     }
+
+    @Override
     public boolean canPause() {
         return mCanPause;
     }
 
+    @Override
     public boolean canSeekBackward() {
         return mCanSeekBack;
     }
 
+    @Override
     public boolean canSeekForward() {
         return mCanSeekForward;
     }
 
     @CalledByNative
-    public static ContentVideoView createContentVideoView(int nativeContentVideoView) {
-        if (sContentVideoView != null)
-            return sContentVideoView;
-
-        if (sDelegate != null && sDelegate.getContext() != null) {
-            sContentVideoView = new ContentVideoView(sDelegate.getContext(),
-                    nativeContentVideoView);
-
-            sDelegate.onShowCustomView(sContentVideoView);
-            sContentVideoView.setBackgroundColor(Color.BLACK);
-            sContentVideoView.showContentVideoView();
-            sContentVideoView.setVisibility(View.VISIBLE);
-            return sContentVideoView;
-        }
-        return null;
+    private static ContentVideoView createContentVideoView(Context context,

[qinmin, 2013/04/12 16:41:09]

move context to the next line so all params stay in the same line

[michaelbai, 2013/04/18 18:21:59]

Done.

+            int nativeContentVideoView, ContentVideoViewClient client) {
+        ThreadUtils.assertOnUiThread();
+        if (client == null) return null;

[joth, 2013/04/12 17:56:24]

can this reasonably happen? Seems like an error, and returning null just likely
to trip up the caller

that said...
given discussion above about context possibly being non-activity, then it's
quite likely in that case we'd get exception here anyway when we attempt to
contruct the base View against inappropriate context type.
So maybe the answer is to catch any excption in this method, log it, and return
null, and then make sure the native side can deal with null return. (I think
it's already robust to that anyway, especially now we using weak ref)

[joth, 2013/04/13 01:18:35]

hmm one option is to just require 'context instanceof Activity' here, like we do
for alert dialogs (see canShowAlertDialog() in webview classic)
It's a bit hacky, but would avoid random exceptions all down the line when
trying to add views to an inappropriate context type

[michaelbai, 2013/04/18 18:21:59]

Actually, this couldn't happen, if the webview is not on screen view, there is
no way to switch to full screen video. 

Do you want the hacky code here?


On 2013/04/13 01:18:35, joth wrote:

[michaelbai, 2013/04/18 18:21:59]

Done.

[michaelbai, 2013/04/18 18:21:59]

Removed the null check for client.

On 2013/04/12 17:56:24, joth wrote:

+        return new ContentVideoView(context, nativeContentVideoView, client);
     }
 
     public void removeMediaController() {
@@ -578,23 +572,15 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
     }
 
     @CalledByNative
-    public static void destroyContentVideoView() {
-        sDelegate.onDestroyContentVideoView();
-        if (sContentVideoView != null) {
-            sContentVideoView.removeMediaController();
-            sContentVideoView.removeSurfaceView();
-            sContentVideoView.setVisibility(View.GONE);
-        }
-        sContentVideoView = null;
+    public void destroyContentVideoView() {
+        mClient.onDestroyContentVideoView();
+        removeMediaController();
+        removeSurfaceView();
+        setVisibility(View.GONE);
     }
 
     public static ContentVideoView getContentVideoView() {
-        return sContentVideoView;
-    }
-
-    public static void registerContentVideoViewContextDelegate(
-       ContentVideoViewContextDelegate delegate) {
-        sDelegate = delegate;
+        return nativeGetSingletonJavaContentVideoView();
     }
 
     @Override
@@ -611,6 +597,7 @@ public class ContentVideoView extends FrameLayout implements MediaPlayerControl,
         return super.onKeyDown(keyCode, event);
     }
 
+    private static native ContentVideoView nativeGetSingletonJavaContentVideoView();
     private native void nativeExitFullscreen(int nativeContentVideoView, boolean relaseMediaPlayer);
     private native int nativeGetCurrentPosition(int nativeContentVideoView);
     private native int nativeGetDurationInMilliSeconds(int nativeContentVideoView);