Add PopupZoomer view to hierarchy only when necessary.

BUG=164983


Review URL: https://chromiumcodereview.appspot.com/11506009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@172173 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 192 insertions, 17 deletions

diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
index 02e81a7..88c6f3b 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
@@ -545,7 +545,25 @@ public class ContentViewCore implements MotionEventDelegate {
 
     private void initPopupZoomer(Context context){
         mPopupZoomer = new PopupZoomer(context);
-        mContainerView.addView(mPopupZoomer);
+        mPopupZoomer.setOnVisibilityChangedListener(new PopupZoomer.OnVisibilityChangedListener() {
+            @Override
+            public void onPopupZoomerShown(PopupZoomer zoomer) {
+                if (mContainerView.indexOfChild(zoomer) == -1) {
+                    mContainerView.addView(zoomer);
+                } else {
+                    assert false : "PopupZoomer should never be shown without being hidden";
+                }
+            }
+
+            @Override
+            public void onPopupZoomerHidden(PopupZoomer zoomer) {
+                if (mContainerView.indexOfChild(zoomer) != -1) {
+                    mContainerView.removeView(zoomer);
+                } else {
+                    assert false : "PopupZoomer should never be hidden without being shown";
+                }
+            }
+        });
         PopupZoomer.OnTapListener listener = new PopupZoomer.OnTapListener() {
             @Override
             public boolean onSingleTap(View v, MotionEvent e) {
diff --git a/content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java b/content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
index d62ff77..707115d 100644
--- a/content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
+++ b/content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
@@ -55,6 +55,16 @@ class PopupZoomer extends View {
 
     private OnTapListener mOnTapListener = null;
 
+    /**
+     * Interface to be implemented to add and remove PopupZoomer to/from the view hierarchy.
+     */
+    public static interface OnVisibilityChangedListener {
+        public void onPopupZoomerShown(PopupZoomer zoomer);
+        public void onPopupZoomerHidden(PopupZoomer zoomer);
+    }
+
+    private OnVisibilityChangedListener mOnVisibilityChangedListener = null;
+
     // Cached drawable used to frame the zooming popup.
     // TODO(tonyg): This should be marked purgeable so that if the system wants to recover this
     // memory, we can just reload it from the resource ID next time it is needed.
@@ -77,6 +87,9 @@ class PopupZoomer extends View {
     // to avoid having it jump to full size then animate closed.
     private long mTimeLeft = 0;
 
+    // initDimensions() needs to be called in onDraw().
+    private boolean mNeedsToInitDimensions;
+
     // Available view area after accounting for ZOOM_BOUNDS_MARGIN.
     private RectF mViewClipRect;
 
@@ -219,6 +232,13 @@ class PopupZoomer extends View {
     }
 
     /**
+     * Sets the OnVisibilityChangedListener.
+     */
+    public void setOnVisibilityChangedListener(OnVisibilityChangedListener listener) {
+        mOnVisibilityChangedListener = listener;
+    }
+
+    /**
      * Sets the bitmap to be used for the zoomed view.
      */
     public void setBitmap(Bitmap bitmap) {
@@ -227,6 +247,7 @@ class PopupZoomer extends View {
             mZoomedBitmap = null;
         }
         mZoomedBitmap = bitmap;
+
         // Round the corners of the bitmap so it doesn't stick out around the overlay.
         Canvas canvas = new Canvas(mZoomedBitmap);
         Path path = new Path();
@@ -252,7 +273,10 @@ class PopupZoomer extends View {
         mTimeLeft = 0;
         if (show) {
             setVisibility(VISIBLE);
-            initDimensions();
+            mNeedsToInitDimensions = true;
+            if (mOnVisibilityChangedListener != null) {
+                mOnVisibilityChangedListener.onPopupZoomerShown(this);
+            }
         } else {
             long endTime = mAnimationStartTime + ANIMATION_DURATION;
             mTimeLeft = endTime - SystemClock.uptimeMillis();
@@ -266,6 +290,9 @@ class PopupZoomer extends View {
         mAnimating = false;
         mShowing = false;
         mTimeLeft = 0;
+        if (mOnVisibilityChangedListener != null) {
+            mOnVisibilityChangedListener.onPopupZoomerHidden(this);
+        }
         setVisibility(INVISIBLE);
         mZoomedBitmap.recycle();
         mZoomedBitmap = null;
@@ -287,10 +314,6 @@ class PopupZoomer extends View {
     }
 
     private void setTargetBounds(Rect rect) {
-        mViewClipRect = new RectF(ZOOM_BOUNDS_MARGIN,
-                ZOOM_BOUNDS_MARGIN,
-                getWidth() - ZOOM_BOUNDS_MARGIN,
-                getHeight() - ZOOM_BOUNDS_MARGIN);
         mTargetBounds = rect;
     }
 
@@ -308,6 +331,11 @@ class PopupZoomer extends View {
         int width = getWidth();
         int height = getHeight();
 
+        mViewClipRect = new RectF(ZOOM_BOUNDS_MARGIN,
+                ZOOM_BOUNDS_MARGIN,
+                width - ZOOM_BOUNDS_MARGIN,
+                height - ZOOM_BOUNDS_MARGIN);
+
         // Ensure it stays inside the bounds of the view.  First shift it around to see if it
         // can fully fit in the view, then clip it to the padding section of the view to
         // ensure no overflow.
@@ -375,9 +403,23 @@ class PopupZoomer extends View {
         mPopupScrollY = constrain(mPopupScrollY, mMinScrollY, mMaxScrollY);
     }
 
+    /*
+     * Tests override it as the PopupZoomer is never attached to the view hierarchy.
+     */
+    protected boolean acceptZeroSizeView() {
+        return false;
+    }
+
     @Override
     protected void onDraw(Canvas canvas) {
         if (!isShowing() || mZoomedBitmap == null) return;
+        if (!acceptZeroSizeView() && (getWidth() == 0 || getHeight() == 0)) return;
+
+        if (mNeedsToInitDimensions) {
+            mNeedsToInitDimensions = false;
+            initDimensions();
+        }
+
         canvas.save();
         // Calculate the elapsed fraction of animation.
         float time = (SystemClock.uptimeMillis() - mAnimationStartTime + mTimeLeft) /
diff --git a/content/public/android/javatests/src/org/chromium/content/browser/ContentViewPopupZoomerTest.java b/content/public/android/javatests/src/org/chromium/content/browser/ContentViewPopupZoomerTest.java
new file mode 100644
index 0000000..b3fcea3
--- /dev/null
+++ b/content/public/android/javatests/src/org/chromium/content/browser/ContentViewPopupZoomerTest.java
@@ -0,0 +1,106 @@
+// Copyright (c) 2012 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.content.browser;
+
+import android.test.suitebuilder.annotation.MediumTest;
+import android.view.View;
+
+import org.chromium.base.test.util.Feature;
+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.TestCallbackHelperContainer;
+import org.chromium.content.browser.test.util.TestCallbackHelperContainer.OnPageFinishedHelper;
+import org.chromium.content_shell.ContentShellTestBase;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class ContentViewPopupZoomerTest extends ContentShellTestBase {
+    private static final int WAIT_TIMEOUT_SECONDS = 2;
+
+    private static PopupZoomer findPopupZoomer(ContentView view) {
+        assert view != null;
+        for (int i = 0; i < view.getChildCount(); i++) {
+            View child = view.getChildAt(i);
+            if (child instanceof PopupZoomer) return (PopupZoomer) child;
+        }
+        return null;
+    }
+
+    private static class PopupShowingCriteria implements Criteria {
+        private final ContentView mView;
+        private final boolean mShouldBeShown;
+        public PopupShowingCriteria(ContentView view, boolean shouldBeShown) {
+            mView = view;
+            mShouldBeShown = shouldBeShown;
+        }
+        @Override
+        public boolean isSatisfied() {
+            PopupZoomer popup = findPopupZoomer(mView);
+            boolean isVisibilitySet = popup == null ? false : popup.getVisibility() == View.VISIBLE;
+            return isVisibilitySet ? mShouldBeShown : !mShouldBeShown;
+        }
+    }
+
+    private static class PopupHasNonZeroDimensionsCriteria implements Criteria {
+        private final ContentView mView;
+        public PopupHasNonZeroDimensionsCriteria(ContentView view) {
+            mView = view;
+        }
+        @Override
+        public boolean isSatisfied() {
+            PopupZoomer popup = findPopupZoomer(mView);
+            if (popup == null) return false;
+            return popup.getWidth() != 0 && popup.getHeight() != 0;
+        }
+    }
+
+    private String generateTestUrl(int totalUrls, int targetIdAt, String targetId) {
+        StringBuilder testUrl = new StringBuilder();
+        testUrl.append("data:text/html;utf-8,<html><body>");
+        for (int i = 0; i < totalUrls; i++) {
+            boolean isTargeted = i == targetIdAt;
+            testUrl.append("<a href=\"data:text/html;utf-8,<html><head><script>" +
+                    "function doesItWork() { return 'yes'; }</script></head></html>\"" +
+                    (isTargeted ? (" id=\"" + targetId + "\"") : "") + ">" +
+                    "<small><sup>" +
+                    (isTargeted ? "<b>" : "") + i + (isTargeted ? "</b>" : "") +
+                    "</sup></small></a>");
+        }
+        testUrl.append("</small></div></body></head>");
+        return testUrl.toString();
+    }
+
+    public ContentViewPopupZoomerTest() {
+    }
+
+    /**
+     * Tests that shows a zoomer popup and makes sure it has valid dimensions.
+     */
+    @MediumTest
+    @Feature({"Browser"})
+    public void testPopupZoomerShowsUp() throws InterruptedException, TimeoutException {
+        launchContentShellWithUrl(generateTestUrl(100, 15, "clickme"));
+        assertTrue("Page failed to load", waitForActiveShellToBeDoneLoading());
+
+        final ContentView view = getActivity().getActiveContentView();
+        final TestCallbackHelperContainer viewClient =
+                new TestCallbackHelperContainer(view);
+
+        // The popup should be hidden before the click.
+        assertTrue("The zoomer popup is shown after load.",
+                CriteriaHelper.pollForCriteria(new PopupShowingCriteria(view, false)));
+
+        // Once clicked, the popup should show up.
+        DOMUtils.clickNode(this, view, viewClient, "clickme");
+        assertTrue("The zoomer popup did not show up on click.",
+                CriteriaHelper.pollForCriteria(new PopupShowingCriteria(view, true)));
+
+        // The shown popup should have valid dimensions eventually.
+        assertTrue("The zoomer popup has zero dimensions.",
+                CriteriaHelper.pollForCriteria(new PopupHasNonZeroDimensionsCriteria(view)));
+    }
+}
diff --git a/content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java b/content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
index 49950f0..c5d834d 100644
--- a/content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
+++ b/content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java
@@ -42,6 +42,12 @@ public class PopupZoomerTest extends InstrumentationTestCase {
             super.onDraw(c);
         }
 
+        // Test doesn't attach PopupZoomer to the view hierarchy,
+        // but onDraw() should still go on.
+        protected boolean acceptZeroSizeView() {
+            return true;
+        }
+
         public void finishPendingDraws() {
             // Finish all pending draw calls. A draw call may change mPendingDraws.
             while (mPendingDraws > 0) {
diff --git a/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java b/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
index eec2285..7272058 100644
--- a/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
+++ b/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
@@ -6,11 +6,11 @@ package org.chromium.content.browser.test.util;
 
 import android.graphics.Rect;
 import android.test.ActivityInstrumentationTestCase2;
-import android.test.InstrumentationTestCase;
 import android.util.JsonReader;
 
 import java.io.IOException;
 import java.io.StringReader;
+import java.util.concurrent.TimeoutException;
 
 import junit.framework.Assert;
 
@@ -26,7 +26,7 @@ public class DOMUtils {
      */
     public static Rect getNodeBounds(
             final ContentView view, TestCallbackHelperContainer viewClient, String nodeId)
-            throws Throwable {
+            throws InterruptedException, TimeoutException {
         StringBuilder sb = new StringBuilder();
         sb.append("(function() {");
         sb.append("  var node = document.getElementById('" + nodeId + "');");
@@ -58,11 +58,12 @@ public class DOMUtils {
             }
             jsonReader.endArray();
             Assert.assertEquals("Invalid bounds returned.", 4, i);
+
+            jsonReader.close();
         } catch (IOException exception) {
             Assert.fail("Failed to evaluate JavaScript: " + jsonText + "\n" + exception);
         }
 
-        jsonReader.close();
         return new Rect(bounds[0], bounds[1], bounds[0] + bounds[2], bounds[1] + bounds[3]);
     }
 
@@ -71,7 +72,7 @@ public class DOMUtils {
      */
     public static void clickNode(ActivityInstrumentationTestCase2 activityTestCase,
             final ContentView view, TestCallbackHelperContainer viewClient, String nodeId)
-            throws Throwable {
+            throws InterruptedException, TimeoutException {
         Rect bounds = getNodeBounds(view, viewClient, nodeId);
         Assert.assertNotNull("Failed to get DOM element bounds of '" + nodeId + "'.'", bounds);
 
@@ -88,7 +89,7 @@ public class DOMUtils {
      */
     public static void longPressNode(ActivityInstrumentationTestCase2 activityTestCase,
             final ContentView view, TestCallbackHelperContainer viewClient, String nodeId)
-            throws Throwable {
+            throws InterruptedException, TimeoutException {
         Rect bounds = getNodeBounds(view, viewClient, nodeId);
         Assert.assertNotNull("Failed to get DOM element bounds of '" + nodeId + "'.'", bounds);
 
@@ -104,7 +105,8 @@ public class DOMUtils {
      * Scrolls the view to ensure that the required DOM node is visible.
      */
     public static void scrollNodeIntoView(final ContentView view,
-            TestCallbackHelperContainer viewClient, String nodeId) throws Throwable {
+            TestCallbackHelperContainer viewClient, String nodeId)
+            throws InterruptedException, TimeoutException {
         JavaScriptUtils.executeJavaScriptAndWaitForResult(view, viewClient,
                 "document.getElementById('" + nodeId + "').scrollIntoView()");
     }
@@ -113,7 +115,8 @@ public class DOMUtils {
      * Returns the contents of the node by its id.
      */
     public static String getNodeContents(final ContentView view,
-            TestCallbackHelperContainer viewClient, String nodeId) throws Throwable {
+            TestCallbackHelperContainer viewClient, String nodeId)
+            throws InterruptedException, TimeoutException {
         StringBuilder sb = new StringBuilder();
         sb.append("(function() {");
         sb.append("  var node = document.getElementById('" + nodeId + "');");
@@ -133,11 +136,11 @@ public class DOMUtils {
             if (jsonReader.hasNext()) contents = jsonReader.nextString();
             jsonReader.endArray();
             Assert.assertNotNull("Invalid contents returned.", contents);
+
+            jsonReader.close();
         } catch (IOException exception) {
             Assert.fail("Failed to evaluate JavaScript: " + jsonText + "\n" + exception);
         }
-
-        jsonReader.close();
         return contents;
     }
 }
diff --git a/content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java b/content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java
index 68d7b8f3..0b5c971 100644
--- a/content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java
+++ b/content/shell/android/javatests/src/org/chromium/content_shell/ContentShellTestBase.java
@@ -63,9 +63,9 @@ public class ContentShellTestBase extends ActivityInstrumentationTestCase2<Conte
      * loading pages. Instead it should be used more for test initialization. The proper way
      * to wait is to use a TestCallbackHelperContainer after the initial load is completed.
      * @return Whether or not the Shell was actually finished loading.
-     * @throws Exception
+     * @throws InterruptedException
      */
-    protected boolean waitForActiveShellToBeDoneLoading() throws Exception {
+    protected boolean waitForActiveShellToBeDoneLoading() throws InterruptedException {
         final ContentShellActivity activity = getActivity();
 
         // Wait for the Content Shell to be initialized.