Refinement for onVisibilityAggregated

Change the args for onVisibilityAggregated to a single boolean for
visibility to the user. Also fix an ordering bug that could trigger a
view background drawable call to setVisible before the view would
respond true to verifyDrawable, which caused issues with some apps.

Bug 27461617
Bug 27689884

Change-Id: I58bdc7c4364264f7fa0863689ba2b76df904ef81

8 files changed, 61 insertions, 51 deletions

diff --git a/api/current.txt b/api/current.txt
index 27d53fd139f4..29fe842a0f9f 100644
--- a/api/current.txt
+++ b/api/current.txt
@@ -42597,7 +42597,7 @@ package android.view {
     method public void onStartTemporaryDetach();
     method public boolean onTouchEvent(android.view.MotionEvent);
     method public boolean onTrackballEvent(android.view.MotionEvent);
-    method public void onVisibilityAggregated(android.view.View, int);
+    method public void onVisibilityAggregated(boolean);
     method protected void onVisibilityChanged(android.view.View, int);
     method public void onWindowFocusChanged(boolean);
     method public void onWindowSystemUiVisibilityChanged(int);
diff --git a/api/system-current.txt b/api/system-current.txt
index 32fb87f3274f..a28724874a34 100644
--- a/api/system-current.txt
+++ b/api/system-current.txt
@@ -45351,7 +45351,7 @@ package android.view {
     method public void onStartTemporaryDetach();
     method public boolean onTouchEvent(android.view.MotionEvent);
     method public boolean onTrackballEvent(android.view.MotionEvent);
-    method public void onVisibilityAggregated(android.view.View, int);
+    method public void onVisibilityAggregated(boolean);
     method protected void onVisibilityChanged(android.view.View, int);
     method public void onWindowFocusChanged(boolean);
     method public void onWindowSystemUiVisibilityChanged(int);
diff --git a/api/test-current.txt b/api/test-current.txt
index 45ef8398d845..261e17b3f403 100644
--- a/api/test-current.txt
+++ b/api/test-current.txt
@@ -42671,7 +42671,7 @@ package android.view {
     method public void onStartTemporaryDetach();
     method public boolean onTouchEvent(android.view.MotionEvent);
     method public boolean onTrackballEvent(android.view.MotionEvent);
-    method public void onVisibilityAggregated(android.view.View, int);
+    method public void onVisibilityAggregated(boolean);
     method protected void onVisibilityChanged(android.view.View, int);
     method public void onWindowFocusChanged(boolean);
     method public void onWindowSystemUiVisibilityChanged(int);
diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java
index 55393b159ee0..a7bf73a6823c 100644
--- a/core/java/android/view/View.java
+++ b/core/java/android/view/View.java
@@ -10210,44 +10210,38 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
      * ViewGroup. Intended to only be called when {@link #isAttachedToWindow()},
      * {@link #getWindowVisibility()} is {@link #VISIBLE} and this view's parent {@link #isShown()}.
      *
-     * @param changedView the view that changed, either <code>this</code> or an ancestor
-     * @param visibility aggregated visibility of this view's parent or changedView, whichever
-     *                   is closer
-     * @return the aggregated visibility for this view
+     * @param isVisible true if this view's visibility to the user is uninterrupted by its
+     *                  ancestors or by window visibility
+     * @return true if this view is visible to the user, not counting clipping or overlapping
      */
-    int dispatchVisibilityAggregated(View changedView, @Visibility int visibility) {
-        visibility = Math.max(visibility, getVisibility());
-        onVisibilityAggregated(changedView, visibility);
-        return visibility;
+    @Visibility boolean dispatchVisibilityAggregated(boolean isVisible) {
+        final boolean thisVisible = getVisibility() == VISIBLE;
+        if (thisVisible) {
+            onVisibilityAggregated(isVisible);
+        }
+        return thisVisible && isVisible;
     }
 
     /**
      * Called when the user-visibility of this View is potentially affected by a change
      * to this view itself, an ancestor view or the window this view is attached to.
      *
-     * <p>The visibility value reported will be one of {@link #VISIBLE}, {@link #INVISIBLE}
-     * or {@link #GONE}, reflecting this view's aggregated visibility within its window.
-     * The visibility parameter takes this view's own visibility into account.
-     * Calls to this method may be repeated with the same visibility values; implementations
-     * should ensure that work done is not duplicated unnecessarily.</p>
-     *
-     * @param changedView the view that changed; may be <code>this</code> or an ancestor
-     * @param visibility the visibility of this view in the context of the full window
+     * @param isVisible true if this view and all of its ancestors are {@link #VISIBLE}
+     *                  and this view's window is also visible
      */
     @CallSuper
-    public void onVisibilityAggregated(View changedView, @Visibility int visibility) {
-        final boolean visible = visibility == VISIBLE && getVisibility() == VISIBLE;
-        if (visible && mAttachInfo != null) {
+    public void onVisibilityAggregated(boolean isVisible) {
+        if (isVisible && mAttachInfo != null) {
             initialAwakenScrollBars();
         }
 
         final Drawable dr = mBackground;
-        if (dr != null && visible != dr.isVisible()) {
-            dr.setVisible(visible, false);
+        if (dr != null && isVisible != dr.isVisible()) {
+            dr.setVisible(isVisible, false);
         }
         final Drawable fg = mForegroundInfo != null ? mForegroundInfo.mDrawable : null;
-        if (fg != null && visible != fg.isVisible()) {
-            fg.setVisible(visible, false);
+        if (fg != null && isVisible != fg.isVisible()) {
+            fg.setVisible(isVisible, false);
         }
     }
 
@@ -11376,7 +11370,7 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
                 // to change animation states.
                 if (mParent != null && getWindowVisibility() == VISIBLE &&
                         ((!(mParent instanceof ViewGroup)) || ((ViewGroup) mParent).isShown())) {
-                    dispatchVisibilityAggregated(this, newVisibility);
+                    dispatchVisibilityAggregated(newVisibility == VISIBLE);
                 }
                 notifySubtreeAccessibilityStateChangedIfNeeded();
             }
@@ -15289,7 +15283,9 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
         if (vis != GONE) {
             onWindowVisibilityChanged(vis);
             if (isShown()) {
-                onVisibilityAggregated(this, vis);
+                // Calling onVisibilityChanged directly here since the subtree will also
+                // receive dispatchAttachedToWindow and this same call
+                onVisibilityAggregated(vis == VISIBLE);
             }
         }
 
@@ -15312,7 +15308,9 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
             if (vis != GONE) {
                 onWindowVisibilityChanged(GONE);
                 if (isShown()) {
-                    onVisibilityAggregated(this, GONE);
+                    // Invoking onVisibilityAggregated directly here since the subtree
+                    // will also receive detached from window
+                    onVisibilityAggregated(false);
                 }
             }
         }
@@ -18001,12 +17999,14 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
 
         /*
          * Regardless of whether we're setting a new background or not, we want
-         * to clear the previous drawable.
+         * to clear the previous drawable. setVisible first while we still have the callback set.
          */
         if (mBackground != null) {
+            if (isAttachedToWindow()) {
+                mBackground.setVisible(false, false);
+            }
             mBackground.setCallback(null);
             unscheduleDrawable(mBackground);
-            mBackground.setVisible(false, false);
         }
 
         if (background != null) {
@@ -18043,12 +18043,19 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
                 requestLayout = true;
             }
 
+            // Set mBackground before we set this as the callback and start making other
+            // background drawable state change calls. In particular, the setVisible call below
+            // can result in drawables attempting to start animations or otherwise invalidate,
+            // which requires the view set as the callback (us) to recognize the drawable as
+            // belonging to it as per verifyDrawable.
+            mBackground = background;
             background.setCallback(this);
             if (background.isStateful()) {
                 background.setState(getDrawableState());
             }
-            background.setVisible(getWindowVisibility() == VISIBLE && isShown(), false);
-            mBackground = background;
+            if (isAttachedToWindow()) {
+                background.setVisible(getWindowVisibility() == VISIBLE && isShown(), false);
+            }
 
             applyBackgroundTint();
 
@@ -18227,9 +18234,11 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
         }
 
         if (mForegroundInfo.mDrawable != null) {
+            if (isAttachedToWindow()) {
+                mForegroundInfo.mDrawable.setVisible(false, false);
+            }
             mForegroundInfo.mDrawable.setCallback(null);
             unscheduleDrawable(mForegroundInfo.mDrawable);
-            mForegroundInfo.mDrawable.setVisible(false, false);
         }
 
         mForegroundInfo.mDrawable = foreground;
@@ -18244,7 +18253,9 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
                 foreground.setState(getDrawableState());
             }
             applyForegroundTint();
-            foreground.setVisible(getWindowVisibility() == VISIBLE && isShown(), false);
+            if (isAttachedToWindow()) {
+                foreground.setVisible(getWindowVisibility() == VISIBLE && isShown(), false);
+            }
         } else if ((mViewFlags & WILL_NOT_DRAW) != 0 && mBackground == null) {
             mPrivateFlags |= PFLAG_SKIP_DRAW;
         }
diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java
index 36a5db179f1f..aa11e3f96ef5 100644
--- a/core/java/android/view/ViewGroup.java
+++ b/core/java/android/view/ViewGroup.java
@@ -1291,20 +1291,19 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager
     }
 
     @Override
-    int dispatchVisibilityAggregated(View changedView, @Visibility int visibility) {
-        visibility = super.dispatchVisibilityAggregated(changedView, visibility);
+    boolean dispatchVisibilityAggregated(boolean isVisible) {
+        isVisible = super.dispatchVisibilityAggregated(isVisible);
         final int count = mChildrenCount;
         final View[] children = mChildren;
         for (int i = 0; i < count; i++) {
-            // Only dispatch to children with at least the same level of visibility
-            // that we're reporting, otherwise it won't affect that child/subtree at all
-            // so it's not worth telling them about it. Note that we use <= here
-            // because VISIBLE < INVISIBLE < GONE as constant values.
-            if (children[i].getVisibility() <= visibility) {
-                children[i].dispatchVisibilityAggregated(changedView, visibility);
+            // Only dispatch to visible children. Not visible children and their subtrees already
+            // know that they aren't visible and that's not going to change as a result of
+            // whatever triggered this dispatch.
+            if (children[i].getVisibility() == VISIBLE) {
+                children[i].dispatchVisibilityAggregated(isVisible);
             }
         }
-        return visibility;
+        return isVisible;
     }
 
     @Override
diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java
index fecf8922862e..a2295ce5fecf 100644
--- a/core/java/android/view/ViewRootImpl.java
+++ b/core/java/android/view/ViewRootImpl.java
@@ -1472,7 +1472,7 @@ public final class ViewRootImpl implements ViewParent,
         if (viewVisibilityChanged) {
             mAttachInfo.mWindowVisibility = viewVisibility;
             host.dispatchWindowVisibilityChanged(viewVisibility);
-            host.dispatchVisibilityAggregated(host, viewVisibility);
+            host.dispatchVisibilityAggregated(viewVisibility == View.VISIBLE);
             if (viewVisibility != View.VISIBLE || mNewSurfaceNeeded) {
                 endDragResizing();
                 destroyHardwareResources();
diff --git a/core/java/android/widget/ImageView.java b/core/java/android/widget/ImageView.java
index 71b5ce9547c5..04d344f0caa6 100644
--- a/core/java/android/widget/ImageView.java
+++ b/core/java/android/widget/ImageView.java
@@ -1501,10 +1501,10 @@ public class ImageView extends View {
     }
 
     @Override
-    public void onVisibilityAggregated(View changedView, @Visibility int visibility) {
-        super.onVisibilityAggregated(changedView, visibility);
+    public void onVisibilityAggregated(boolean isVisible) {
+        super.onVisibilityAggregated(isVisible);
         if (mDrawable != null) {
-            mDrawable.setVisible(visibility == VISIBLE, false);
+            mDrawable.setVisible(isVisible, false);
         }
     }
 
diff --git a/core/java/android/widget/ProgressBar.java b/core/java/android/widget/ProgressBar.java
index dd0f91e44045..e9fa26ce4658 100644
--- a/core/java/android/widget/ProgressBar.java
+++ b/core/java/android/widget/ProgressBar.java
@@ -617,6 +617,7 @@ public class ProgressBar extends View {
     private void swapCurrentDrawable(Drawable newDrawable) {
         final Drawable oldDrawable = mCurrentDrawable;
         mCurrentDrawable = newDrawable;
+
         if (oldDrawable != mCurrentDrawable) {
             if (oldDrawable != null) {
                 oldDrawable.setVisible(false, false);
@@ -1645,10 +1646,9 @@ public class ProgressBar extends View {
     }
 
     @Override
-    public void onVisibilityAggregated(View changedView, @Visibility int visibility) {
-        super.onVisibilityAggregated(changedView, visibility);
+    public void onVisibilityAggregated(boolean isVisible) {
+        super.onVisibilityAggregated(isVisible);
 
-        final boolean isVisible = visibility == VISIBLE;
         if (isVisible != mAggregatedIsVisible) {
             mAggregatedIsVisible = isVisible;