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

This reverts commit 46c618aa.

Reason for revert: crbug.com/1152817

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}

TBR=dewittj@chromium.org,jianli@chromium.org,skyostil@chromium.org,eseckler@chromium.org

Change-Id: Iac8698c1bd7cf2eb5101e7a85811ab15ff929f5f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1151391
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2560825Reviewed-by: Gang Wu <gangwu@chromium.org>
Commit-Queue: Gang Wu <gangwu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831060}

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

@@ -14,15 +14,11 @@ 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);
@@ -37,7 +33,6 @@ 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);
@@ -45,45 +40,19 @@ 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");
-    }
 
-    @Override
-    protected void onVisibilityChanged(View changedView, int visibility) {
-        super.onVisibilityChanged(changedView, visibility);
-        updateAnimationState(isAttachedToWindow());
+        if (getVisibility() == View.VISIBLE) {
+            mSpinner.start();
         }
-
-    @Override
-    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) {
-        // 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;
-        }
+    public void setVisibility(int visibility) {
+        super.setVisibility(visibility);
 
-        boolean visible = isShown() && isAttached;
-        if (mSpinner.isRunning() && !visible) {
+        if (mSpinner.isRunning() && getVisibility() != View.VISIBLE) {
             mSpinner.stop();
-        } else if (!mSpinner.isRunning() && visible) {
+        } else if (!mSpinner.isRunning() && getVisibility() == View.VISIBLE) {
             mSpinner.start();
         }
     }

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

@@ -8,7 +8,6 @@ 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;
 
@@ -17,7 +16,6 @@ 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;
@@ -25,52 +23,38 @@ 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 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);
+        Activity context = Robolectric.buildActivity(Activity.class).get();
+        context.setTheme(R.style.Light);
+        mMaterialSpinnerView = new MaterialSpinnerView(context);
     }
 
     @Test
     public void testInit_isVisible_spinnerStarted() {
         assertThat(mMaterialSpinnerView.getVisibility()).isEqualTo(View.VISIBLE);
-        assertThat(mMaterialSpinnerView.isShown()).isTrue();
 
-        assertThat(mAnimationDrawable.isRunning()).isTrue();
+        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
+                .isTrue();
     }
 
     @Test
     public void testSetVisibility_gone_stopsSpinner() {
         mMaterialSpinnerView.setVisibility(View.GONE);
 
-        assertThat(mAnimationDrawable.isRunning()).isFalse();
+        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
+                .isFalse();
     }
 
     @Test
     public void testSetVisibility_invisible_stopsSpinner() {
         mMaterialSpinnerView.setVisibility(View.INVISIBLE);
 
-        assertThat(mAnimationDrawable.isRunning()).isFalse();
+        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
+                .isFalse();
     }
 
     @Test
@@ -78,43 +62,7 @@ public class MaterialSpinnerViewTest {
         mMaterialSpinnerView.setVisibility(View.GONE);
         mMaterialSpinnerView.setVisibility(View.VISIBLE);
 
-        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();
+        assertThat(((CircularProgressDrawable) mMaterialSpinnerView.getDrawable()).isRunning())
+                .isTrue();
     }
 }

chrome/browser/flags/android/chrome_feature_list.cc

@@ -106,7 +106,6 @@ 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,8 +322,6 @@ 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,10 +49,6 @@ 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,8 +33,6 @@ extern const base::Feature kInterestFeedV2ClicksAndViewsConditionalUpload;
 
 extern const base::Feature kInterestFeedNoticeCardAutoDismiss;
 
-extern const base::Feature kInterestFeedSpinnerAlwaysAnimate;
-
 std::string GetFeedReferrerUrl();
 
 }  // namespace feed