[Offline indicator v2] Fix hiding animation for Android L

An integration test was failing on L bots because the hiding animation
in the compositor wouldn't complete and the indicator would get stuck
shown. Upon investigating, I discovered that this issue was not limited
to tests and was reproducible on an emulator.

The issue was that #onLayoutChanged would be called during the hide
animation causing a wrong height value to be sent to the compositor as
if it's being shown.

A simple fix is to add a boolean to ignore #onLayoutChange calls when
we no longer need them, and we're basically done once we get a height to
run the show animation.

Bug: 1059297
Change-Id: I943300ebc1e93172eec9311ec28c44172464ba4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130781
Commit-Queue: Sinan Sahin <sinansahin@google.com>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756352}

chrome/android/java/src/org/chromium/chrome/browser/status_indicator/StatusIndicatorMediator.java

@@ -15,6 +15,7 @@ import android.view.View;
 
 import androidx.annotation.ColorInt;
 import androidx.annotation.NonNull;
+import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.supplier.Supplier;
 import org.chromium.chrome.browser.fullscreen.ChromeFullscreenManager;
@@ -38,6 +39,7 @@ class StatusIndicatorMediator
     private Supplier<Boolean> mCanAnimateNativeBrowserControls;
 
     private int mIndicatorHeight;
+    private int mJavaLayoutHeight;
     private boolean mIsHiding;
 
     /**
@@ -75,9 +77,11 @@ class StatusIndicatorMediator
     @Override
     public void onLayoutChange(View v, int left, int top, int right, int bottom, int oldLeft,
             int oldTop, int oldRight, int oldBottom) {
-        if (mIndicatorHeight == v.getHeight()) return;
+        // Wait for first valid height while showing indicator.
+        if (mIsHiding || mJavaLayoutHeight != 0 || v.getHeight() <= 0) return;
 
-        heightChanged(v.getHeight());
+        mJavaLayoutHeight = v.getHeight();
+        updateVisibility(false);
     }
 
     void addObserver(StatusIndicatorCoordinator.StatusIndicatorObserver observer) {
@@ -293,7 +297,7 @@ class StatusIndicatorMediator
         animatorSet.addListener(new AnimatorListenerAdapter() {
             @Override
             public void onAnimationEnd(Animator animation) {
-                heightChanged(0);
+                updateVisibility(true);
             }
         });
         animatorSet.start();
@@ -315,14 +319,16 @@ class StatusIndicatorMediator
 
     // Other internal methods
 
-    private void heightChanged(int newHeight) {
-        mIndicatorHeight = newHeight;
+    /**
+     * Call to kick off height change when status indicator is shown/hidden.
+     * @param hiding Whether the status indicator is hiding.
+     */
+    private void updateVisibility(boolean hiding) {
+        mIsHiding = hiding;
+        mIndicatorHeight = hiding ? 0 : mJavaLayoutHeight;
 
-        if (mIndicatorHeight > 0) {
+        if (!mIsHiding) {
             mFullscreenManager.addListener(this);
-            mIsHiding = false;
-        } else {
-            mIsHiding = true;
         }
 
         // If the browser controls won't be animating, we can pretend that the animation ended.
@@ -330,7 +336,7 @@ class StatusIndicatorMediator
             onOffsetChanged(mIndicatorHeight);
         }
 
-        notifyHeightChange(newHeight);
+        notifyHeightChange(mIndicatorHeight);
     }
 
     private void onOffsetChanged(int topControlsMinHeightOffset) {
@@ -353,6 +359,12 @@ class StatusIndicatorMediator
         if (doneHiding) {
             mFullscreenManager.removeListener(this);
             mIsHiding = false;
+            mJavaLayoutHeight = 0;
         }
     }
+
+    @VisibleForTesting
+    void updateVisibilityForTesting(boolean hiding) {
+        updateVisibility(hiding);
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/status_indicator/StatusIndicatorTest.java

@@ -14,7 +14,6 @@ import static android.support.test.espresso.matcher.ViewMatchers.withId;
 
 import android.graphics.Color;
 import android.graphics.drawable.ColorDrawable;
-import android.os.Build;
 import android.support.test.InstrumentationRegistry;
 import android.support.test.filters.MediumTest;
 import android.view.View;
@@ -32,7 +31,6 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import org.chromium.base.test.util.CommandLineFlags;
-import org.chromium.base.test.util.DisableIf;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
@@ -99,8 +97,6 @@ public class StatusIndicatorTest {
 
     @Test
     @MediumTest
-    @DisableIf.Build(sdk_is_greater_than = Build.VERSION_CODES.KITKAT,
-            sdk_is_less_than = Build.VERSION_CODES.M, message = "crbug.com/1059421")
     public void testShowAndHide() {
         final ChromeFullscreenManager fullscreenManager =
                 mActivityTestRule.getActivity().getFullscreenManager();

chrome/android/junit/src/org/chromium/chrome/browser/status_indicator/StatusIndicatorMediatorTest.java

@@ -90,8 +90,7 @@ public class StatusIndicatorMediatorTest {
         mMediator.onControlsOffsetChanged(0, 70, 0, 0, false);
 
         // Now, hide it. Listener shouldn't be removed.
-        setViewHeight(0);
-        mMediator.onLayoutChange(mStatusIndicatorView, 0, 0, 0, 0, 0, 0, 0, 0);
+        mMediator.updateVisibilityForTesting(true);
         verify(mFullscreenManager, never()).removeListener(mMediator);
 
         // Once the hiding animation is done...
@@ -109,8 +108,7 @@ public class StatusIndicatorMediatorTest {
         // The Android view should be visible at this point.
         assertEquals(View.VISIBLE, mModel.get(StatusIndicatorProperties.ANDROID_VIEW_VISIBILITY));
         // Now hide it.
-        setViewHeight(0);
-        mMediator.onLayoutChange(mStatusIndicatorView, 0, 0, 0, 0, 0, 0, 0, 0);
+        mMediator.updateVisibilityForTesting(true);
         // The hiding animation...
         mMediator.onControlsOffsetChanged(0, 30, 0, 0, false);
         // Android view will be gone once the animation starts.
@@ -160,8 +158,7 @@ public class StatusIndicatorMediatorTest {
         assertTrue(mModel.get(StatusIndicatorProperties.COMPOSITED_VIEW_VISIBLE));
 
         // Hide the indicator.
-        setViewHeight(0);
-        mMediator.onLayoutChange(mStatusIndicatorView, 0, 0, 0, 0, 0, 0, 0, 0);
+        mMediator.updateVisibilityForTesting(true);
         // The indicator should hide immediately.
         assertEquals(View.GONE, mModel.get(StatusIndicatorProperties.ANDROID_VIEW_VISIBILITY));
         assertFalse(mModel.get(StatusIndicatorProperties.COMPOSITED_VIEW_VISIBLE));