Reland "feed: Don't spin the spinner when its ancestors are hidden"

This is a reland of 46c618aa with an
added null check in MaterialSpinnerView.updateAnimationState (see diff
from patch set 1).

Original change's description:
> feed: Don't spin the spinner when its ancestors are hidden
>
> Currently, the spinner used in feed will only stop() its animation if
> the spinner view itself is hidden through setVisibility(). However, in
> reality, one of its ancestor views is hidden instead.
>
> This causes a continuous invisible animation to tick at the display's
> refresh rate (usually 60fps) if the spinner was ever shown, until it is
> destroyed (when the NTP views are eventually evicted).
>
> Instead, stop the animation also when any of the spinner's ancestors
> become hidden by listening for visibility changes and window attachment
> state changes.
>
> Bug: 1151391
> Change-Id: Ib7b7a8e56f083619420fd58f4340991253402535
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552781
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Reviewed-by: Jian Li <jianli@chromium.org>
> Commit-Queue: Eric Seckler <eseckler@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#830957}

Bug: 1151391, 1152817
Change-Id: I306b623fc5d7243cf96998367f01380a80833aa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2560108Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Jian Li <jianli@chromium.org>
Auto-Submit: Eric Seckler <eseckler@chromium.org>
Commit-Queue: Jian Li <jianli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831188}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/shared/ui/MaterialSpinnerView.java

@@ -14,11 +14,15 @@ import androidx.annotation.Nullable;
 import androidx.appcompat.widget.AppCompatImageView;
 import androidx.swiperefreshlayout.widget.CircularProgressDrawable;
 
+import org.chromium.base.FeatureList;
+import org.chromium.base.TraceEvent;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 
 /** View that shows a Material themed spinner. */
 public class MaterialSpinnerView extends AppCompatImageView {
     private final CircularProgressDrawable mSpinner;
+    private final boolean mAlwaysAnimate;
 
     public MaterialSpinnerView(Context context) {
         this(context, null);
@@ -33,6 +37,7 @@ public class MaterialSpinnerView extends AppCompatImageView {
     @SuppressWarnings({"nullness:argument.type.incompatible", "nullness:method.invocation.invalid"})
     public MaterialSpinnerView(Context context, @Nullable AttributeSet attrs, int defStyle) {
         super(context, attrs, defStyle);
+        TraceEvent.begin("MaterialSpinnerView");
         mSpinner = new CircularProgressDrawable(context);
         mSpinner.setStyle(CircularProgressDrawable.DEFAULT);
         setImageDrawable(mSpinner);
@@ -40,19 +45,49 @@ public class MaterialSpinnerView extends AppCompatImageView {
         Theme theme = context.getTheme();
         theme.resolveAttribute(R.attr.feedSpinnerColor, typedValue, true);
         mSpinner.setColorSchemeColors(typedValue.data);
+        mAlwaysAnimate = FeatureList.isInitialized() ? ChromeFeatureList.isEnabled(
+                                 ChromeFeatureList.INTEREST_FEED_SPINNER_ALWAYS_ANIMATE)
+                                                     : false;
+        updateAnimationState(isAttachedToWindow());
+        TraceEvent.end("MaterialSpinnerView");
+    }
 
-        if (getVisibility() == View.VISIBLE) {
-            mSpinner.start();
-        }
+    @Override
+    protected void onVisibilityChanged(View changedView, int visibility) {
+        super.onVisibilityChanged(changedView, visibility);
+        updateAnimationState(isAttachedToWindow());
     }
 
     @Override
-    public void setVisibility(int visibility) {
-        super.setVisibility(visibility);
+    protected void onAttachedToWindow() {
+        super.onAttachedToWindow();
+        updateAnimationState(/*isAttached=*/true);
+    }
+
+    @Override
+    protected void onDetachedFromWindow() {
+        // isAttachedToWindow() doesn't turn false during onDetachedFromWindow(), so we pass the new
+        // attachment state into updateAnimationState() here explicitly.
+        updateAnimationState(/*isAttached=*/false);
+        super.onDetachedFromWindow();
+    }
+
+    private void updateAnimationState(boolean isAttached) {
+        // Some Android versions call onVisibilityChanged() during the View's constructor before the
+        // spinner is created.
+        if (mSpinner == null) return;
+
+        // TODO(crbug.com/1151391): This feature is used for A:B testing to determine the impact of
+        // a bug fix. Remove after experiment is complete.
+        if (mAlwaysAnimate) {
+            if (!mSpinner.isRunning()) mSpinner.start();
+            return;
+        }
 
-        if (mSpinner.isRunning() && getVisibility() != View.VISIBLE) {
+        boolean visible = isShown() && isAttached;
+        if (mSpinner.isRunning() && !visible) {
             mSpinner.stop();
-        } else if (!mSpinner.isRunning() && getVisibility() == View.VISIBLE) {
+        } else if (!mSpinner.isRunning() && visible) {
             mSpinner.start();
         }
     }

chrome/android/feed/core/javatests/src/org/chromium/chrome/browser/feed/shared/ui/MaterialSpinnerViewTest.java

@@ -8,6 +8,7 @@ import static com.google.common.truth.Truth.assertThat;
 
 import android.app.Activity;
 import android.view.View;
+import android.widget.FrameLayout;
 
 import androidx.swiperefreshlayout.widget.CircularProgressDrawable;
 
@@ -16,6 +17,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.Robolectric;
 import org.robolectric.annotation.Config;
+import org.robolectric.annotation.LooperMode;
 
 import org.chromium.chrome.R;
 import org.chromium.testing.local.LocalRobolectricTestRunner;
@@ -23,38 +25,52 @@ import org.chromium.testing.local.LocalRobolectricTestRunner;
 /** Tests for {@link MaterialSpinnerView}. */
 @RunWith(LocalRobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
+@LooperMode(LooperMode.Mode.PAUSED)
 public class MaterialSpinnerViewTest {
+    private FrameLayout mLayout;
     private MaterialSpinnerView mMaterialSpinnerView;
+    private CircularProgressDrawable mAnimationDrawable;
 
     @Before
     public void setUp() {
-        Activity context = Robolectric.buildActivity(Activity.class).get();
-        context.setTheme(R.style.Light);
-        mMaterialSpinnerView = new MaterialSpinnerView(context);
+        Activity activity = Robolectric.setupActivity(Activity.class);
+        activity.setTheme(R.style.Light);
+
+        // Attach the spinner inside a layout, so we can hide either the spinner
+        // or the parent view (ie. the layout) in the tests. Note that we
+        // require the looper to stay paused (LooperMode.Mode.PAUSED) for the
+        // duration of the tests. Otherwise, Robolectric will run through the
+        // animation and stop it before the tests get run. Because
+        // Robolectric.setupActivity() will run the looper until idle, we call
+        // setContentView() only after launching the activity above.
+        mMaterialSpinnerView = new MaterialSpinnerView(activity);
+        mAnimationDrawable = (CircularProgressDrawable) mMaterialSpinnerView.getDrawable();
+
+        mLayout = new FrameLayout(activity);
+        mLayout.addView(mMaterialSpinnerView);
+        activity.setContentView(mLayout);
     }
 
     @Test
     public void testInit_isVisible_spinnerStarted() {
         assertThat(mMaterialSpinnerView.getVisibility()).isEqualTo(View.VISIBLE);
+        assertThat(mMaterialSpinnerView.isShown()).isTrue();
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isTrue();
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
     }
 
     @Test
     public void testSetVisibility_gone_stopsSpinner() {
         mMaterialSpinnerView.setVisibility(View.GONE);
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isFalse();
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
     }
 
     @Test
     public void testSetVisibility_invisible_stopsSpinner() {
         mMaterialSpinnerView.setVisibility(View.INVISIBLE);
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isFalse();
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
     }
 
     @Test
@@ -62,7 +78,43 @@ public class MaterialSpinnerViewTest {
         mMaterialSpinnerView.setVisibility(View.GONE);
         mMaterialSpinnerView.setVisibility(View.VISIBLE);
 
-        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
-                .isTrue();
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
+    }
+
+    @Test
+    public void testContainerSetVisibility_gone_stopsSpinner() {
+        mLayout.setVisibility(View.GONE);
+
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
+    }
+
+    @Test
+    public void testContainerSetVisibility_invisible_stopsSpinner() {
+        mLayout.setVisibility(View.INVISIBLE);
+
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
+    }
+
+    @Test
+    public void testContainerSetVisibility_toTrue_startsSpinner() {
+        mLayout.setVisibility(View.GONE);
+        mLayout.setVisibility(View.VISIBLE);
+
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
+    }
+
+    @Test
+    public void testDetachFromWindow_stopsSpinner() {
+        mLayout.removeView(mMaterialSpinnerView);
+
+        assertThat(mAnimationDrawable.isRunning()).isFalse();
+    }
+
+    @Test
+    public void testAttachToWindow_startsSpinner() {
+        mLayout.removeView(mMaterialSpinnerView);
+        mLayout.addView(mMaterialSpinnerView);
+
+        assertThat(mAnimationDrawable.isRunning()).isTrue();
     }
 }

chrome/browser/flags/android/chrome_feature_list.cc

@@ -106,6 +106,7 @@ const base::Feature* kFeaturesExposedToJava[] = {
     &feature_engagement::kIPHTabSwitcherButtonFeature,
     &feed::kInterestFeedContentSuggestions,
     &feed::kInterestFeedNoticeCardAutoDismiss,
+    &feed::kInterestFeedSpinnerAlwaysAnimate,
     &feed::kInterestFeedV1ClicksAndViewsConditionalUpload,
     &feed::kInterestFeedV2,
     &feed::kReportFeedUserActions,

chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java

@@ -322,6 +322,8 @@ public abstract class ChromeFeatureList {
     public static final String INTEREST_FEED_CONTENT_SUGGESTIONS = "InterestFeedContentSuggestions";
     public static final String INTEREST_FEED_NOTICE_CARD_AUTO_DISMISS =
             "InterestFeedNoticeCardAutoDismiss";
+    public static final String INTEREST_FEED_SPINNER_ALWAYS_ANIMATE =
+            "InterestFeedSpinnerAlwaysAnimate";
     public static final String INTEREST_FEED_V2 = "InterestFeedV2";
     public static final String KITKAT_SUPPORTED = "KitKatSupported";
     public static final String LOOKALIKE_NAVIGATION_URL_SUGGESTIONS_UI =

components/feed/feed_feature_list.cc

@@ -49,6 +49,10 @@ const base::Feature kInterestFeedV2ClicksAndViewsConditionalUpload{
 const base::Feature kInterestFeedNoticeCardAutoDismiss{
     "InterestFeedNoticeCardAutoDismiss", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Used for A:B testing of a bug fix (crbug.com/1151391).
+const base::Feature kInterestFeedSpinnerAlwaysAnimate{
+    "InterestFeedSpinnerAlwaysAnimate", base::FEATURE_DISABLED_BY_DEFAULT};
+
 const char kDefaultReferrerUrl[] =
     "https://www.googleapis.com/auth/chrome-content-suggestions";
 

components/feed/feed_feature_list.h

@@ -33,6 +33,8 @@ extern const base::Feature kInterestFeedV2ClicksAndViewsConditionalUpload;
 
 extern const base::Feature kInterestFeedNoticeCardAutoDismiss;
 
+extern const base::Feature kInterestFeedSpinnerAlwaysAnimate;
+
 std::string GetFeedReferrerUrl();
 
 }  // namespace feed