[aw] Software mode tests for fullscreen API.

android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java

 // Copyright 2013 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.
 
 package org.chromium.android_webview.test;
 
 import android.test.suitebuilder.annotation.MediumTest;
 import android.view.View;
 import android.view.ViewGroup;
 
 import org.chromium.android_webview.test.util.JavascriptEventObserver;
 import org.chromium.base.test.util.Feature;
+import org.chromium.content.browser.ContentVideoView;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.content.browser.test.util.Criteria;
 import org.chromium.content.browser.test.util.CriteriaHelper;
 import org.chromium.content.browser.test.util.DOMUtils;
 import org.chromium.content.browser.test.util.TouchCommon;
 
 import java.util.concurrent.TimeoutException;
 
 /**
  * Test the fullscreen API (WebChromeClient::onShow/HideCustomView).
@@ skipping 27 matching lines @@
         mContentsClient = new FullScreenVideoTestAwContentsClient(getActivity());
         mTestContainerView =
                 createAwTestContainerViewOnMainSync(mContentsClient);
         mContentViewCore = mTestContainerView.getContentViewCore();
         enableJavaScriptOnUiThread(mTestContainerView.getAwContents());
         mTestContainerView.getAwContents().getSettings().setFullscreenSupported(true);
     }
 
     @MediumTest
     @Feature({"AndroidWebView"})
+    @DisableHardwareAccelerationForTest
+    public void testFullscreenVideoInSoftwareModeDoesNotDeadlock() throws Throwable {
+        // Although fullscreen video is not supported without hardware acceleration
+        // we should not deadlock if apps try to use it.
+        loadTestPageAndClickFullscreen(VIDEO_TEST_URL);
+        mContentsClient.waitForCustomViewShown();
+        getInstrumentation().runOnMainSync(new Runnable() {
+            @Override
+            public void run() {
+                mContentsClient.getExitCallback().onCustomViewHidden();
+            }
+        });
+        mContentsClient.waitForCustomViewHidden();
+    }
+
+    @MediumTest
+    @Feature({"AndroidWebView"})
+    @DisableHardwareAccelerationForTest
+    public void testFullscreenForNonVideoElementIsSupportedInSoftwareMode() throws Throwable {
+        // Fullscreen for non-video elements is supported and works as expected. Note that
+        // this test is the same as testOnShowAndHideCustomViewWithCallback_videoInsideDiv below.
+        doTestOnShowAndHideCustomViewWithCallback(VIDEO_INSIDE_DIV_TEST_URL);
+    }
+
+    @MediumTest
+    @Feature({"AndroidWebView"})
     public void testOnShowAndHideCustomViewWithCallback_video() throws Throwable {
         doTestOnShowAndHideCustomViewWithCallback(VIDEO_TEST_URL);
     }
 
     @MediumTest
     @Feature({"AndroidWebView"})
     public void testOnShowAndHideCustomViewWithCallback_videoInsideDiv() throws Throwable {
         doTestOnShowAndHideCustomViewWithCallback(VIDEO_INSIDE_DIV_TEST_URL);
     }
 
@@ skipping 218 matching lines @@
             ViewGroup viewGroup = (ViewGroup) view;
             for (int i = 0; i < viewGroup.getChildCount(); i++) {
                 if (getKeepScreenOn(viewGroup.getChildAt(i))) {
                     return true;
                 }
             }
         }
         return view.getKeepScreenOn();
     }
 
+    private void assertContainsContentVideoView()
+            throws InterruptedException {
+        // We need to poll because the ContentVideoView is added to the customView asynchronously
+        assertTrue(CriteriaHelper.pollForCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return containsVideoView(mContentsClient.getCustomView());

[boliu, 2014/10/22 02:01:02]

Need to do this on the UI thread.

[Ignacio Solla, 2014/10/22 09:32:16]

Why? I'm happy to change it but I'd like to understand the reason first.

[boliu, 2014/10/22 15:53:45]

Because AwContents/ContentViewCore, and the entire view tree is single threaded
and should only be accessed on the UI thread.

[Ignacio Solla, 2014/10/27 11:42:06]

OK done.  (I was thinking that read-only operations with polling in the
instrumentation thread in tests were ok, but I guess it is safer to read it on
the UI thread because thread-local caches in Java)

+            }
+        }));
+    }
+
+    private void assertIsFullscreen() throws InterruptedException {
+        // We need to poll because the Javascript state is updated asynchronously
+        assertTrue(CriteriaHelper.pollForCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                try {
+                    return DOMUtils.isFullscreen(mContentViewCore.getWebContents());

[boliu, 2014/10/22 02:01:02]

I didn't find this DOMUtils method?! I assume it needs to happen on UI thread as
well.

[Ignacio Solla, 2014/10/22 09:32:16]

To avoid conflicts, I have set the diffbase of this change against:
https://codereview.chromium.org/639413002/

So crcr/639413002 needs to be landed first. That method is defined there.

[boliu, 2014/10/22 15:53:45]

Ok. isFullscreen itself looks thread safe. mContentViewCore.getWebContents() is
not!!

[Ignacio Solla, 2014/10/27 11:42:06]

Ok, I'm calling mContentViewCore.getWebContents() on the UI thread now.

Note that we cannot call DOMUtils.blah on the UI thread because
JavaScriptUtils.executeJavaScriptAndWaitForResult must be called on the
instrumentation test.

[boliu, 2014/10/27 16:49:44]

Why? I don't see it just glancing at the code. ThreadUtils.runOnUiThread calls
the Runnable immediately if you are on the UI thread already, and I think
everything else should just work?

And maybe we should fix that in a follow up if it doesn't work.

[Ignacio Solla, 2014/10/27 16:59:52]

See:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

The comment reads:
"Calling this from the UI thread causes it to time-out: the UI thread being
blocked won't have a chance to process the JavaScript eval response)."

+                } catch (InterruptedException | TimeoutException e) {
+                    fail(e.getMessage());
+                    return false;
+                }
+            }
+        }));
+    }
+
+    private boolean containsVideoView(View view) {

[boliu, 2014/10/22 02:01:02]

nit: move this next to assertContainsContentVideoView

[Ignacio Solla, 2014/10/22 09:32:16]

Done.

+        if (view instanceof ContentVideoView) {
+            return true;
+        }
+        if (view instanceof ViewGroup) {
+            ViewGroup viewGroup = (ViewGroup) view;
+            for (int i = 0; i < viewGroup.getChildCount(); i++) {
+                if (containsVideoView(viewGroup.getChildAt(i))) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
     private JavascriptEventObserver registerObserver(final String observerName) {
         final JavascriptEventObserver observer = new JavascriptEventObserver();
         getInstrumentation().runOnMainSync(new Runnable() {
             @Override
             public void run() {
                 observer.register(mContentViewCore, observerName);
             }
         });
         return observer;
     }
 
     private void doOnShowAndHideCustomViewTest(String videoTestUrl, final Runnable existFullscreen)
             throws Throwable {
         doOnShowCustomViewTest(videoTestUrl);
         getInstrumentation().runOnMainSync(existFullscreen);
         mContentsClient.waitForCustomViewHidden();
     }
 
     private void doOnShowCustomViewTest(String videoTestUrl) throws Exception {
         loadTestPageAndClickFullscreen(videoTestUrl);
         mContentsClient.waitForCustomViewShown();
-        assertTrue(DOMUtils.isFullscreen(mContentViewCore.getWebContents()));
+        assertIsFullscreen();
+        if (videoTestUrl.equals(VIDEO_TEST_URL)) {
+            // We only create a ContentVideoView (ie. a hardware accelerated surface) when going
+            // fullscreen on a video element.
+            assertContainsContentVideoView();
+        }
     }
 
     private void loadTestPageAndClickFullscreen(String videoTestUrl) throws Exception {
         loadTestPage(videoTestUrl);
         DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID);

[boliu, 2014/10/22 02:01:02]

UI thread.

[Ignacio Solla, 2014/10/22 09:32:16]

We're already dispatching the click event on the UI thread, see
TouchCommon.dispatchTouchEvent that clickNode ends up calling. 

Do we also need to retrieve the position of the node in the UI thread? I'm happy
to change it but I'd like to understand the reason first.

[boliu, 2014/10/22 15:53:45]

Ahh crap I didn't read that far. I'm pretty sure that findViewById call in
TouchCommon ain't thread safe either...

But even before that, you can't use ContentViewCore in DOMUtils on the
instrumentation thread.

[Ignacio Solla, 2014/10/27 11:42:06]

clickNode cannot be called on the instrumentation thread because
executeJavaScriptAndWaitForResult will block otherwise. You can also see that
all other tests in the codebase call DOMUtils.clickNode on the instrumentation
thread. Fixing the problems that you pointed out with DOMUtils.clickNode are
outside the scope of this patch (but don't be afraid to fix it if you want, but
understand that I don't feel that dumping this on me is reasonable. FYI, I have
noticed and fixed many issues with fullscreen / video playback so in general I'm
not afraid of doing that, but do understand that sometimes I also like to do
more interesting work).

[boliu, 2014/10/27 16:49:44]

This change has to wait for that to be fixed then.

[Ignacio Solla, 2014/10/27 16:59:52]

eh, this change is not changing this line of code

     }
 
     private void loadTestPage(String videoTestUrl) throws Exception {
         loadUrlSync(mTestContainerView.getAwContents(),
                 mContentsClient.getOnPageFinishedHelper(), videoTestUrl);
     }
 }