Refactoring ContentVideoViewContextDelegate.java

android_webview/java/src/org/chromium/android_webview/AwContentsClient.java

Index: android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
diff --git a/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java b/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
index 9473d8ff9e9f6148c1685c22a29204c84aa791dd..142738003d9f6286386941b70cc48c1f92f5d7b2 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
@@ -4,6 +4,8 @@
 
 package org.chromium.android_webview;
 
+import android.content.Context;
+import android.content.pm.ActivityInfo;
 import android.graphics.Bitmap;
 import android.graphics.Picture;
 import android.graphics.Rect;
@@ -20,7 +22,9 @@ import android.webkit.GeolocationPermissions;
 import android.webkit.SslErrorHandler;
 import android.webkit.ValueCallback;
 import android.webkit.WebChromeClient;
+import android.webkit.WebChromeClient.CustomViewCallback;

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

rather than import, could you use 'WebChromeClient.CustomViewCallback' where
needed?

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

Done.

 
+import org.chromium.content.browser.ContentVideoViewClient;
 import org.chromium.content.browser.ContentViewClient;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.content.browser.WebContentsObserverAndroid;
@@ -47,6 +51,7 @@ public abstract class AwContentsClient extends ContentViewClient {
 
     private AwWebContentsObserver mWebContentsObserver;
 
+    private ContentVideoViewClient mContentVideoViewClient;
     private double mDIPScale;
 
     //--------------------------------------------------------------------------------------------
@@ -196,6 +201,29 @@ public abstract class AwContentsClient extends ContentViewClient {
 
     }
 
+    public class AwContentVideoViewClient implements ContentVideoViewClient {
+        @Override
+        public void onShowCustomView(View view) {
+            CustomViewCallback cb = new CustomViewCallback() {
+                @Override
+                public void onCustomViewHidden() {
+                }
+            };
+            AwContentsClient.this.onShowCustomView(view,
+                    ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED,
+                    cb);
+        }
+
+        @Override
+        public void onDestroyContentVideoView() {

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

this needs to call onHideCustomView. (is there an instrumentation test to catch
this?)

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

Right, AwContentsClientFullScreenVideoTest test this, but I didn't run it


On 2013/04/05 01:56:40, joth wrote:

+        }
+
+        @Override
+        public View getVideoLoadingProgressView() {
+            return AwContentsClient.this.getVideoLoadingProgressView();
+        }
+    }
+
     void installWebContentsObserver(ContentViewCore contentViewCore) {
         if (mWebContentsObserver != null) {
             mWebContentsObserver.detachFromWebContents();
@@ -330,4 +358,11 @@ public abstract class AwContentsClient extends ContentViewClient {
     @Override
     final public void onContextualActionBarHidden() {
     }
+
+    @Override
+    final public ContentVideoViewClient getContentVideoViewClient() {
+        if (mContentVideoViewClient == null)
+            mContentVideoViewClient = new AwContentVideoViewClient();
+        return mContentVideoViewClient;

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

I'm tempted to remove the mContentVideoViewClient member field and just return
new instance everytime, so we can have the real implementation consistent with
the test code.

This does mean that if the client app does not hold onto the CustomViewCallback
instance we pass them, the ContentVideoViewClient instance will also be GCed
meaning they would not in turn receive the onHideCustomView call.... hmm subtle.

ANother option is to enforce this in base ContentViewClient: have a public final
method there that calls a protected getContentVideoViewClient(), and then caches
the result. (template method pattern).
This means the two lifetimes are necessarily the same, so at that point we could
just remove ContentVideoViewClient and push its methods straight onto
ContentViewClient. :-/

alternatives?

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

I may misunderstood you goal here. It seemed that you don't want to call
onHideCustomView if the client app don't hold the CustomViewCallback we gave
them. If so, return new instance every time will not help on it except we have a
weak reference of CustomViewCallback, even though the goal is not guaranteed, it
depends on when the GC happen. 

So what's your goal?

On 2013/04/05 01:56:40, joth wrote:

+    }
 }