Revert "Stop showing bitmap captures during transitions."

This reverts commit 60a95da. Reason for revert: causes multiple StartSurface test failures on Android N (https://crbug.com/1428021) Original change's description: > Stop showing bitmap captures during transitions. > > During transitions between NTP/rendered pages and the GTS, the toolbar > would be hidden, and the bitmap underneath would be shown. With > suppression enabled the bitmap is often not as valid as it used to be. > > To make this look better, we now try to keep the toolbar's android > view's visible and with an opaque background during those transitions. > This was difficult because the started/finished hiding/showing calls > were not really accurate, and so this change starts by making those > events actually observer the layout's android based animations. > > Bug: 1421277 > Change-Id: I389026e234bcbc9aebca75a680fd85d249216284 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4358746 > Reviewed-by: Xi Han <hanxi@chromium.org> > Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com> > Commit-Queue: Sky Malice <skym@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1121912} Bug: 1421277, 1428021 Change-Id: Ieeb3eaf60e23f8b53d519f2ce714b863d957bc05 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4370077 Commit-Queue: Alex Ilin <alexilin@chromium.org> Owners-Override: Alex Ilin <alexilin@google.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#1122349}
chromium · Mar 27, 2023 · ad91220 · ad91220
1 parent 36d00b7
commit ad91220
Show file tree

Hide file tree

Showing 12 changed files with 9 additions and 104 deletions.
diff --git a/...java/src/org/chromium/chrome/features/start_surface/TabSwitcherAndStartSurfaceLayout.java b/...java/src/org/chromium/chrome/features/start_surface/TabSwitcherAndStartSurfaceLayout.java
@@ -897,11 +897,6 @@ public boolean canHostBeFocusable() {
         return super.canHostBeFocusable();
     }
 
-    @Override
-    public boolean isRunningAnimations() {
-        return mDeferredAnimationRunnable != null || mTabToSwitcherAnimation != null;
-    }
-
     /**
      * Shrink/Expand animation is disabled for Tablet TabSwitcher launch polish.
      * @return Whether shrink/expand animation is enabled.

diff --git a/...s/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherLayout.java b/...s/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherLayout.java
@@ -728,11 +728,6 @@ public boolean canHostBeFocusable() {
         return super.canHostBeFocusable();
     }
 
-    @Override
-    public boolean isRunningAnimations() {
-        return mDeferredAnimationRunnable != null || mTabToSwitcherAnimation != null;
-    }
-
     /**
      * Shrink/Expand animation is disabled for Tablet TabSwitcher launch polish.
      * @return Whether shrink/expand animation is enabled.

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java b/chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/Layout.java
@@ -793,9 +793,4 @@ protected void updateSceneLayer(RectF viewport, RectF contentViewport,
      */
     @LayoutType
     public abstract int getLayoutType();
-
-    /** Returns whether the layout is currently running animations. */
-    public boolean isRunningAnimations() {
-        return false;
-    }
 }
diff --git a/...me/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerImpl.java b/...me/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerImpl.java
@@ -59,7 +59,6 @@
 import org.chromium.chrome.browser.theme.ThemeUtils;
 import org.chromium.chrome.browser.theme.TopUiThemeColorProvider;
 import org.chromium.chrome.browser.toolbar.ControlContainer;
-import org.chromium.chrome.browser.toolbar.ToolbarFeatures;
 import org.chromium.chrome.browser.toolbar.bottom.ScrollingBottomViewSceneLayer;
 import org.chromium.chrome.browser.toolbar.top.TopToolbarOverlayCoordinator;
 import org.chromium.chrome.browser.ui.native_page.NativePage;
@@ -474,16 +473,13 @@ boolean onUpdate(long timeMs, long dtMs) {
         }
         mUpdateRequested = false;
 
-        // TODO(crbug.com/1070281): Remove after the FrameRequestSupplier migrates to the animation
-        //  system.
-        final Layout layout = getActiveLayout();
-
         // TODO(mdjones): Remove the time related params from this method. The new animation system
         // has its own timer.
         boolean areAnimatorsComplete = mAnimationHandler.pushUpdate();
-        if (layout != null && ToolbarFeatures.shouldDelayTransitionsForAnimation()) {
-            areAnimatorsComplete &= !layout.isRunningAnimations();
-        }
+
+        // TODO(crbug.com/1070281): Remove after the FrameRequestSupplier migrates to the animation
+        //  system.
+        final Layout layout = getActiveLayout();
 
         // TODO(crbug.com/1070281): Layout itself should decide when it's done hiding and done
         //  showing.

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java b/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
@@ -933,9 +933,6 @@ public void onFinishedShowing(int layoutType) {
                 if (layoutType == LayoutType.TAB_SWITCHER) {
                     mToolbar.onTabSwitcherTransitionFinished();
                 }
-                if (ToolbarFeatures.shouldDelayTransitionsForAnimation()) {
-                    mToolbar.onTransitionEnd();
-                }
             }
 
             @Override
@@ -950,9 +947,6 @@ public void onStartedHiding(
                         mControlContainer.invalidateBitmap();
                     }
                 }
-                if (ToolbarFeatures.shouldDelayTransitionsForAnimation()) {
-                    mToolbar.onTransitionStart();
-                }
             }
 
             @Override

diff --git a/chrome/browser/flags/android/chrome_feature_list.cc b/chrome/browser/flags/android/chrome_feature_list.cc
@@ -229,7 +229,6 @@ const base::Feature* const kFeaturesExposedToJava[] = {
     &kContextualSearchThinWebViewImplementation,
     &kDeferKeepScreenOnDuringGesture,
     &kDeferNotifyInMotion,
-    &kDelayTransitionsForAnimation,
     &kExperimentsForAgsa,
     &kExploreSites,
     &kFocusOmniboxInIncognitoTabIntents,
@@ -727,10 +726,6 @@ BASE_FEATURE(kDeferNotifyInMotion,
              "DeferNotifyInMotion",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
-BASE_FEATURE(kDelayTransitionsForAnimation,
-             "DelayTransitionsForAnimation",
-             base::FEATURE_ENABLED_BY_DEFAULT);
-
 BASE_FEATURE(kDownloadAutoResumptionThrottling,
              "DownloadAutoResumptionThrottling",
              base::FEATURE_ENABLED_BY_DEFAULT);

diff --git a/chrome/browser/flags/android/chrome_feature_list.h b/chrome/browser/flags/android/chrome_feature_list.h
@@ -87,7 +87,6 @@ BASE_DECLARE_FEATURE(kContextualSearchSuppressShortView);
 BASE_DECLARE_FEATURE(kContextualSearchThinWebViewImplementation);
 BASE_DECLARE_FEATURE(kDeferKeepScreenOnDuringGesture);
 BASE_DECLARE_FEATURE(kDeferNotifyInMotion);
-BASE_DECLARE_FEATURE(kDelayTransitionsForAnimation);
 BASE_DECLARE_FEATURE(kDontPrefetchLibraries);
 BASE_DECLARE_FEATURE(kDownloadAutoResumptionThrottling);
 BASE_DECLARE_FEATURE(kDownloadHomeForExternalApp);

diff --git a/...e/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java b/...e/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java
@@ -300,7 +300,6 @@ public static boolean getFieldTrialParamByFeatureAsBoolean(
     public static final String DEFER_KEEP_SCREEN_ON_DURING_GESTURE =
             "DeferKeepScreenOnDuringGesture";
     public static final String DEFER_NOTIFY_IN_MOTION = "DeferNotifyInMotion";
-    public static final String DELAY_TRANSITIONS_FOR_ANIMATION = "DelayTransitionsForAnimation";
     public static final String DETAILED_LANGUAGE_SETTINGS = "DetailedLanguageSettings";
     public static final String DISCO_FEED_ENDPOINT = "DiscoFeedEndpoint";
     public static final String DNS_OVER_HTTPS = "DnsOverHttps";

diff --git a/...wser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ToolbarFeatures.java b/...wser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ToolbarFeatures.java
@@ -22,8 +22,6 @@ public final class ToolbarFeatures {
             new MutableFlagWithSafeDefault(ChromeFeatureList.SUPPRESS_TOOLBAR_CAPTURES, false);
     private static final MutableFlagWithSafeDefault sRecordSuppressionMetrics =
             new MutableFlagWithSafeDefault(ChromeFeatureList.RECORD_SUPPRESSION_METRICS, true);
-    private static final MutableFlagWithSafeDefault sDelayTransitionsForAnimation =
-            new MutableFlagWithSafeDefault(ChromeFeatureList.DELAY_TRANSITIONS_FOR_ANIMATION, true);
 
     /** Private constructor to avoid instantiation. */
     private ToolbarFeatures() {}
@@ -44,16 +42,6 @@ public static boolean shouldSuppressCaptures() {
         return sSuppressionFlag.isEnabled();
     }
 
-    /**
-     * Returns whether the layout system will delay transitions between start/done hiding/showing
-     * for Android view animations or not. When this is delayed, the toolbar code will try to
-     * always draw itself from Android views during these transitions, to avoid letting the captured
-     * bitmap leak through during transitions. With suppression enabled, the captured bitmap is less
-     * reliable during these transitions.
-     */
-    public static boolean shouldDelayTransitionsForAnimation() {
-        return sDelayTransitionsForAnimation.isEnabled();
-    }
     /**
      * Returns whether to record metrics from suppression experiment. This allows an arm of
      * suppression to run without the overhead from reporting any extra metrics in Java. Using a

diff --git a/...er/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarLayout.java b/...er/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarLayout.java
@@ -32,7 +32,6 @@
 import org.chromium.base.lifetime.DestroyChecker;
 import org.chromium.base.lifetime.Destroyable;
 import org.chromium.chrome.browser.browser_controls.BrowserStateBrowserControlsVisibilityDelegate;
-import org.chromium.chrome.browser.layouts.LayoutStateProvider;
 import org.chromium.chrome.browser.omnibox.LocationBar;
 import org.chromium.chrome.browser.omnibox.LocationBarCoordinator;
 import org.chromium.chrome.browser.omnibox.NewTabPageDelegate;
@@ -878,16 +877,4 @@ public void setHairlineVisibility(boolean isHairlineVisible) {
             shadow.setVisibility(isHairlineVisible ? VISIBLE : GONE);
         }
     }
-
-    /**
-     * To be called indirectly by
-     * {@link LayoutStateProvider.LayoutStateObserver#onStartedHiding(int, boolean, boolean)}.
-     */
-    public void onTransitionStart() {}
-
-    /**
-     * To be called indirectly by
-     * {@link LayoutStateProvider.LayoutStateObserver#onFinishedShowing(int)}.
-     */
-    public void onTransitionEnd() {}
 }
diff --git a/...ser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java b/...ser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/ToolbarPhone.java
@@ -271,9 +271,6 @@ public class ToolbarPhone extends ToolbarLayout implements OnClickListener, TabC
 
     private boolean mDropdownListScrolled;
 
-    // If we're in a layout transition, between startHiding to doneShowing.
-    private boolean mInLayoutTransition;
-
     // The following are some properties used during animation.  We use explicit property classes
     // to avoid the cost of reflection for each animation setup.
 
@@ -801,18 +798,15 @@ private int getToolbarColorForVisualState(final @VisualState int visualState) {
                     return ChromeColors.getDefaultThemeColor(getContext(), false);
                 }
 
-                // During transition we cannot rely on the background to be opaque yet, so keep full
-                // alpha until the transition is over.
-                int alpha = mInLayoutTransition ? 255 : Math.round(mUrlExpansionFraction * 255);
-
                 // When the NTP fake search box is visible, the background color should be
                 // transparent. When the location bar reaches the top of the screen (i.e. location
                 // bar is fully expanded), the background needs to change back to the default
                 // toolbar color so that the NTP content is not visible beneath the toolbar. In
                 // between the transition, we set a translucent default toolbar color based on
                 // the expansion progress of the toolbar.
                 return androidx.core.graphics.ColorUtils.setAlphaComponent(
-                        ChromeColors.getDefaultThemeColor(getContext(), false), alpha);
+                        ChromeColors.getDefaultThemeColor(getContext(), false),
+                        Math.round(mUrlExpansionFraction * 255));
             case VisualState.NORMAL:
                 return ChromeColors.getDefaultThemeColor(getContext(), false);
             case VisualState.INCOGNITO:
@@ -1410,21 +1404,16 @@ private boolean isChildLeft(View child) {
      *         toolbar.
      */
     private boolean shouldDrawLocationBar() {
-        if (ToolbarFeatures.shouldDelayTransitionsForAnimation()) {
-            // The location bar should have alpha or clip+translation when its not supposed to be
-            // shown. Needs to be drawn during transitions, such as entering/exiting tab switcher.
-            return mLocationBarBackground != null;
-        } else {
-            return mLocationBarBackground != null
-                    && (mTabSwitcherState == STATIC_TAB || mTextureCaptureMode);
-        }
+        return mLocationBarBackground != null
+                && (mTabSwitcherState == STATIC_TAB || mTextureCaptureMode);
     }
 
     private boolean drawLocationBar(Canvas canvas, long drawingTime) {
         TraceEvent.begin("ToolbarPhone.drawLocationBar");
         boolean clipped = false;
         if (shouldDrawLocationBar()) {
             canvas.save();
+
             if (shouldDrawLocationBarBackground()) {
                 if (mActiveLocationBarBackground instanceof NtpSearchBoxDrawable) {
                     ((NtpSearchBoxDrawable) mActiveLocationBarBackground)
@@ -1817,12 +1806,6 @@ public void setTabSwitcherMode(boolean inTabSwitcherMode, boolean showToolbar,
             updateViewsForTabSwitcherMode();
         }
 
-        if (ToolbarFeatures.shouldDelayTransitionsForAnimation()) {
-            // Since mTabSwitcherState has changed, we need to also check if mVisualState should
-            // change.
-            updateVisualsForLocationBarState();
-        }
-
         updateButtonsTranslationY();
 
         if (DeviceClassManager.enableAccessibilityLayout(getContext())) {
@@ -2760,17 +2743,4 @@ private void updateToolbarAndLocationBarColor() {
     private int calculateOnFocusHeightIncrease() {
         return (int) (mBackgroundHeightIncreaseWhenFocus * mUrlFocusChangeFraction / 2);
     }
-
-    @Override
-    public void onTransitionStart() {
-        setVisibility(View.VISIBLE);
-        mInLayoutTransition = true;
-        updateToolbarBackgroundFromState(mVisualState);
-    }
-
-    @Override
-    public void onTransitionEnd() {
-        mInLayoutTransition = false;
-        updateToolbarBackgroundFromState(mVisualState);
-    }
 }
diff --git a/...droid/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/TopToolbarCoordinator.java b/...droid/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/TopToolbarCoordinator.java
@@ -806,12 +806,4 @@ public void setBrowsingModeHairlineVisibility(boolean isVisible) {
     public boolean isBrowsingModeToolbarVisible() {
         return mToolbarLayout.getVisibility() == View.VISIBLE;
     }
-
-    public void onTransitionStart() {
-        mToolbarLayout.onTransitionStart();
-    }
-
-    public void onTransitionEnd() {
-        mToolbarLayout.onTransitionEnd();
-    }
 }