Use display order to sort SceneOverlays

This patch switches the custom "add overlay" methods for a single
unified "addSceneOverlay". This new method does insertion sort based
on a predefined list determining the order of each overlay. Display
order is no longer based exclusively on the order in which the
overlays were added. If two overlays of the same type are added, the
most recently added overlay will display above the others.

Bug: 1070281, 1131536
Change-Id: I5764593debb13d30389f78303120ec4b4fcbb28f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392403
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Mei Liang <meiliang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811784}

chrome/android/chrome_junit_test_java_sources.gni

@@ -44,6 +44,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/compositor/animation/CompositorAnimatorTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/layouts/CompositorModelChangeProcessorUnitTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/layouts/MockLayoutUpdateHost.java",
+  "junit/src/org/chromium/chrome/browser/compositor/layouts/SceneOverlayTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/layouts/StaticLayoutUnitTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/overlays/toolbar/TopToolbarOverlayMediatorTest.java",

chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanel.java

@@ -961,9 +961,4 @@ public class OverlayPanel extends OverlayPanelAnimation implements ActivityState
         closePanel(StateChangeReason.BACK_PRESS, true);
         return true;
     }
-
-    @Override
-    public int getPosition() {
-        return Position.FRONT;
-    }
 }

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManager.java

@@ -36,13 +36,17 @@ import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
 import org.chromium.chrome.browser.compositor.layouts.eventfilter.EdgeSwipeHandler;
 import org.chromium.chrome.browser.compositor.layouts.eventfilter.EventFilter;
 import org.chromium.chrome.browser.compositor.overlays.SceneOverlay;
+import org.chromium.chrome.browser.compositor.overlays.strip.StripLayoutHelperManager;
 import org.chromium.chrome.browser.compositor.overlays.toolbar.TopToolbarOverlayCoordinator;
 import org.chromium.chrome.browser.compositor.scene_layer.SceneLayer;
 import org.chromium.chrome.browser.compositor.scene_layer.SceneOverlayLayer;
+import org.chromium.chrome.browser.compositor.scene_layer.ScrollingBottomViewSceneLayer;
 import org.chromium.chrome.browser.contextualsearch.ContextualSearchManagementDelegate;
 import org.chromium.chrome.browser.device.DeviceClassManager;
 import org.chromium.chrome.browser.fullscreen.BrowserControlsManager;
+import org.chromium.chrome.browser.gesturenav.HistoryNavigationCoordinator;
 import org.chromium.chrome.browser.native_page.NativePageFactory;
+import org.chromium.chrome.browser.status_indicator.StatusIndicatorCoordinator;
 import org.chromium.chrome.browser.tab.SadTab;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.TabBrowserControlsConstraintsHelper;
@@ -69,7 +73,9 @@ import org.chromium.ui.resources.dynamics.DynamicResourceLoader;
 import org.chromium.ui.util.TokenHolder;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 /**
  * A class that is responsible for managing an active {@link Layout} to show to the screen.  This
@@ -132,7 +138,6 @@ public class LayoutManager implements LayoutUpdateHost, LayoutProvider,
     private ContextualSearchPanel mContextualSearchPanel;
     private final OverlayPanelManager mOverlayPanelManager;
     private TopToolbarOverlayCoordinator mToolbarOverlay;
-    private SceneOverlay mStatusIndicatorSceneOverlay;
     private SceneOverlay mGestureNavigationOverscrollGlow;
 
     /** A delegate for interacting with the Contextual Search manager. */
@@ -166,6 +171,9 @@ public class LayoutManager implements LayoutUpdateHost, LayoutProvider,
     /** The overlays that can be drawn on top of the active layout. */
     protected final List<SceneOverlay> mSceneOverlays = new ArrayList<>();
 
+    /** A map of {@link SceneOverlay} to its position relative to the others. */
+    private Map<Class, Integer> mOverlayOrderMap = new HashMap<>();
+
     /**
      * Protected class to handle {@link TabModelObserver} related tasks. Extending classes will
      * need to override any related calls to add new functionality */
@@ -254,6 +262,19 @@ public class LayoutManager implements LayoutUpdateHost, LayoutProvider,
         mContext = host.getContext();
         LayoutRenderHost renderHost = host.getLayoutRenderHost();
 
+        // clang-format off
+        // Overlays are ordered back (closest to the web content) to front.
+        Class[] overlayOrder = new Class[] {
+                HistoryNavigationCoordinator.getSceneOverlayClass(),
+                TopToolbarOverlayCoordinator.class,
+                ScrollingBottomViewSceneLayer.class,
+                StripLayoutHelperManager.class,
+                StatusIndicatorCoordinator.getSceneOverlayClass(),
+                ContextualSearchPanel.class};
+        // clang-format off
+
+        for (int i = 0; i < overlayOrder.length; i++) mOverlayOrderMap.put(overlayOrder[i], i);
+
         assert contentContainer != null;
         mContentContainer = contentContainer;
 
@@ -449,6 +470,7 @@ public class LayoutManager implements LayoutUpdateHost, LayoutProvider,
             mToolbarOverlay = new TopToolbarOverlayCoordinator(mContext, mFrameRequestSupplier,
                     this, controlContainer, tabProvider, getBrowserControlsManager(),
                     mAndroidViewShownSupplier, () -> renderHost.getResourceManager());
+            addSceneOverlay(mToolbarOverlay);
         }
 
         // Initialize Layouts
@@ -456,9 +478,7 @@ public class LayoutManager implements LayoutUpdateHost, LayoutProvider,
 
         // Contextual Search scene overlay.
         mContextualSearchPanel = new ContextualSearchPanel(mContext, this, mOverlayPanelManager);
-
-        // Add any SceneOverlays to a layout.
-        addAllSceneOverlays();
+        addSceneOverlay(mContextualSearchPanel);
 
         // Save state
         mContextualSearchDelegate = contextualSearchDelegate;
@@ -1008,46 +1028,34 @@ public class LayoutManager implements LayoutUpdateHost, LayoutProvider,
     }
 
     /**
-     * Set the status indicator {@link SceneOverlay} to be added to the layout.
-     * @param overlay The {@link SceneOverlay} to set.
+     * Add a {@link SceneOverlay} to be drawn on the composited layer of the active layout.
+     * @param overlay The overlay to add.
      */
-    public void setStatusIndicatorSceneOverlay(SceneOverlay overlay) {
-        mStatusIndicatorSceneOverlay = overlay;
-    }
+    public void addSceneOverlay(SceneOverlay overlay) {
+        if (mSceneOverlays.contains(overlay)) throw new RuntimeException("Overlay already added!");
 
-    /**
-     * Add any {@link SceneOverlay}s to the layout. This can be used to add the overlays in a
-     * particular order.
-     * Classes that override this method should be careful about the order that
-     * overlays are added and when super is called (i.e. cases where one overlay needs to be
-     * on top of another positioned.
-     */
-    protected void addAllSceneOverlays() {
-        mSceneOverlays.add(mToolbarOverlay);
-        if (mStatusIndicatorSceneOverlay != null) {
-            mSceneOverlays.add(mStatusIndicatorSceneOverlay);
+        if (!mOverlayOrderMap.containsKey(overlay.getClass())) {
+            throw new RuntimeException("Please add overlay to order list in constructor.");
         }
-        mSceneOverlays.add(mContextualSearchPanel);
+
+        int overlayPosition = mOverlayOrderMap.get(overlay.getClass());
+
+        int index;
+        for (index = 0; index < mSceneOverlays.size(); index++) {
+            if (overlayPosition < mOverlayOrderMap.get(mSceneOverlays.get(index).getClass())) break;
+        }
+
+        mSceneOverlays.add(index, overlay);
     }
 
-    /**
-     * Add a {@link SceneOverlay} to the front of the list. This means the overlay will be drawn
-     * last and therefore above all other overlays currently in the list.
-     * @param overlay The overlay to be added to the back of the list.
-     */
-    public void addSceneOverlayToFront(SceneOverlay overlay) {
-        assert !mSceneOverlays.contains(overlay);
-        mSceneOverlays.add(overlay);
+    @VisibleForTesting
+    void setSceneOverlayOrderForTesting(Map<Class, Integer> order) {
+        mOverlayOrderMap = order;
     }
 
-    /**
-     * Add a {@link SceneOverlay} to the back of the list. This means the overlay will be drawn
-     * first and therefore behind all other overlays currently in the list.
-     * @param overlay The overlay to be added to the back of the list.
-     */
-    public void addSceneOverlayToBack(SceneOverlay overlay) {
-        assert !mSceneOverlays.contains(overlay);
-        mSceneOverlays.add(0, overlay);
+    @VisibleForTesting
+    List<SceneOverlay> getSceneOverlaysForTesting() {
+        return mSceneOverlays;
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChromeTablet.java

@@ -45,17 +45,11 @@ public class LayoutManagerChromeTablet extends LayoutManagerChrome {
 
         mTabStripLayoutHelperManager = new StripLayoutHelperManager(
                 host.getContext(), this, mHost.getLayoutRenderHost(), () -> mTitleCache);
+        addSceneOverlay(mTabStripLayoutHelperManager);
 
         setNextLayout(null);
     }
 
-    @Override
-    protected void addAllSceneOverlays() {
-        // Add the tab strip overlay before any others.
-        mSceneOverlays.add(mTabStripLayoutHelperManager);
-        super.addAllSceneOverlays();
-    }
-
     @Override
     public void destroy() {
         super.destroy();

chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/SceneOverlay.java

@@ -6,36 +6,18 @@ package org.chromium.chrome.browser.compositor.overlays;
 
 import android.graphics.RectF;
 
-import androidx.annotation.IntDef;
-
 import org.chromium.chrome.browser.compositor.LayerTitleCache;
 import org.chromium.chrome.browser.compositor.layouts.components.VirtualView;
 import org.chromium.chrome.browser.compositor.layouts.eventfilter.EventFilter;
 import org.chromium.chrome.browser.compositor.scene_layer.SceneOverlayLayer;
 import org.chromium.ui.resources.ResourceManager;
 
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
 import java.util.List;
 
 /**
  * An interface which positions the actual tabs and adds additional UI to the them.
  */
 public interface SceneOverlay {
-    /** Positioning information for when overlays are added to the tree. */
-    @IntDef({Position.FRONT, Position.DEFAULT, Position.BACK})
-    @Retention(RetentionPolicy.SOURCE)
-    @interface Position {
-        /** The overlay will be one the of front-most layers. */
-        int FRONT = 0;
-
-        /** The overlay will be between the front and back layers in the order it was added. */
-        int DEFAULT = 1;
-
-        /** The overlay will be one of the back-most layers. */
-        int BACK = 2;
-    }
-
     /**
      * Updates and gets a {@link SceneOverlayLayer} that represents an scene overlay.
      *
@@ -99,10 +81,4 @@ public interface SceneOverlay {
      * @return True if this overlay handles tab creation.
      */
     boolean handlesTabCreating();
-
-    /**
-     * @return Where this overlay should be positioned relative to the other overlays.
-     */
-    @Position
-    int getPosition();
 }

chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperManager.java

@@ -507,11 +507,6 @@ public class StripLayoutHelperManager implements SceneOverlay {
         return false;
     }
 
-    @Override
-    public int getPosition() {
-        return Position.BACK;
-    }
-
     private void tabModelSwitched(boolean incognito) {
         if (incognito == mIsIncognito) return;
         mIsIncognito = incognito;

chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/toolbar/TopToolbarOverlayCoordinator.java

@@ -113,9 +113,4 @@ public class TopToolbarOverlayCoordinator implements SceneOverlay {
     public boolean handlesTabCreating() {
         return false;
     }
-
-    @Override
-    public int getPosition() {
-        return Position.BACK;
-    }
 }

chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/ScrollingBottomViewSceneLayer.java

@@ -158,11 +158,6 @@ public class ScrollingBottomViewSceneLayer extends SceneOverlayLayer implements
     @Override
     public void getVirtualViews(List<VirtualView> views) {}
 
-    @Override
-    public int getPosition() {
-        return Position.DEFAULT;
-    }
-
     @NativeMethods
     interface Natives {
         long init(ScrollingBottomViewSceneLayer caller);

chrome/android/java/src/org/chromium/chrome/browser/gesturenav/HistoryNavigationCoordinator.java

@@ -88,6 +88,11 @@ public class HistoryNavigationCoordinator
         return coordinator;
     }
 
+    /** @return The class of the {@link SceneOverlay} owned by this coordinator. */
+    public static Class getSceneOverlayClass() {
+        return OverscrollGlowOverlay.class;
+    }
+
     /**
      * Initializes the navigation layout and internal objects.
      */
@@ -155,7 +160,7 @@ public class HistoryNavigationCoordinator
             mInsetObserverView = insetObserverView;
             insetObserverView.addObserver(this);
         }
-        layoutManager.addSceneOverlayToFront(mOverscrollGlowOverlay);
+        layoutManager.addSceneOverlay(mOverscrollGlowOverlay);
     }
 
     private boolean isNativePage() {

chrome/android/java/src/org/chromium/chrome/browser/gesturenav/OverscrollGlowOverlay.java

@@ -117,9 +117,4 @@ class OverscrollGlowOverlay extends NavigationGlow implements SceneOverlay {
     public boolean handlesTabCreating() {
         return false;
     }
-
-    @Override
-    public int getPosition() {
-        return Position.BACK;
-    }
 }

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

@@ -155,6 +155,11 @@ public class StatusIndicatorCoordinator {
         return mSceneLayer;
     }
 
+    /** @return The class of the {@link SceneOverlay} owned by this coordinator. */
+    public static Class getSceneOverlayClass() {
+        return StatusIndicatorSceneLayer.class;
+    }
+
     private void initialize() {
         final ViewStub stub = mActivity.findViewById(R.id.status_indicator_stub);
         final ViewResourceFrameLayout root = (ViewResourceFrameLayout) stub.inflate();

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

@@ -41,7 +41,7 @@ class StatusIndicatorSceneLayer extends SceneOverlayLayer implements SceneOverla
      * @param browserControlsStateProvider {@link BrowserControlsStateProvider} to access browser
      *                                     controls offsets.
      */
-    public StatusIndicatorSceneLayer(BrowserControlsStateProvider browserControlsStateProvider) {
+    StatusIndicatorSceneLayer(BrowserControlsStateProvider browserControlsStateProvider) {
         mBrowserControlsStateProvider = browserControlsStateProvider;
     }
 
@@ -121,11 +121,6 @@ class StatusIndicatorSceneLayer extends SceneOverlayLayer implements SceneOverla
     @Override
     public void getVirtualViews(List<VirtualView> views) {}
 
-    @Override
-    public int getPosition() {
-        return Position.DEFAULT;
-    }
-
     @NativeMethods
     interface Natives {
         long init(StatusIndicatorSceneLayer caller);

chrome/android/java/src/org/chromium/chrome/browser/tabbed_mode/TabbedRootUiCoordinator.java

@@ -317,7 +317,7 @@ public class TabbedRootUiCoordinator extends RootUiCoordinator implements Native
                 mActivity.getCompositorViewHolder().getResourceManager(), browserControlsSizer,
                 mActivity.getStatusBarColorController()::getStatusBarColorWithoutStatusIndicator,
                 mCanAnimateBrowserControls, layoutManager::requestUpdate);
-        layoutManager.setStatusIndicatorSceneOverlay(mStatusIndicatorCoordinator.getSceneLayer());
+        layoutManager.addSceneOverlay(mStatusIndicatorCoordinator.getSceneLayer());
         mStatusIndicatorObserver = new StatusIndicatorCoordinator.StatusIndicatorObserver() {
             @Override
             public void onStatusIndicatorHeightChanged(int indicatorHeight) {

chrome/android/java/src/org/chromium/chrome/browser/toolbar/bottom/BottomControlsViewBinder.java

@@ -56,8 +56,7 @@ class BottomControlsViewBinder {
                     new ScrollingBottomViewSceneLayer(view.root, view.root.getTopShadowHeight());
             view.sceneLayer.setIsVisible(
                     model.get(BottomControlsProperties.COMPOSITED_VIEW_VISIBLE));
-            model.get(BottomControlsProperties.LAYOUT_MANAGER)
-                    .addSceneOverlayToBack(view.sceneLayer);
+            model.get(BottomControlsProperties.LAYOUT_MANAGER).addSceneOverlay(view.sceneLayer);
         } else if (BottomControlsProperties.RESOURCE_MANAGER == propertyKey) {
             model.get(BottomControlsProperties.RESOURCE_MANAGER)
                     .getDynamicResourceLoader()

chrome/android/junit/src/org/chromium/chrome/browser/compositor/layouts/SceneOverlayTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.compositor.layouts;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.when;
+
+import android.content.Context;
+import android.content.res.Resources;
+import android.util.DisplayMetrics;
+import android.view.ViewGroup;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.supplier.ObservableSupplier;
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel;
+import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
+import org.chromium.chrome.browser.compositor.overlays.SceneOverlay;
+import org.chromium.chrome.browser.compositor.overlays.toolbar.TopToolbarOverlayCoordinator;
+import org.chromium.chrome.browser.compositor.scene_layer.ScrollingBottomViewSceneLayer;
+
+import java.util.HashMap;
+import java.util.List;
+
+/** Tests for {@link SceneOverlay} interactions. */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class SceneOverlayTest {
+    @Mock
+    private Context mContext;
+
+    @Mock
+    private Resources mResources;
+
+    @Mock
+    private DisplayMetrics mDisplayMetrics;
+
+    @Mock
+    private LayoutManagerHost mLayoutManagerHost;
+
+    @Mock
+    private ViewGroup mContainerView;
+
+    @Mock
+    private ObservableSupplier<TabContentManager> mTabContentManagerSupplier;
+
+    private LayoutManager mLayoutManager;
+
+    @Before
+    public void setup() {
+        MockitoAnnotations.initMocks(this);
+
+        when(mLayoutManagerHost.getContext()).thenReturn(mContext);
+        when(mContext.getResources()).thenReturn(mResources);
+        when(mResources.getDisplayMetrics()).thenReturn(mDisplayMetrics);
+
+        mLayoutManager =
+                new LayoutManager(mLayoutManagerHost, mContainerView, mTabContentManagerSupplier);
+    }
+
+    @Test
+    public void testSceneOverlayPositions() {
+        List<SceneOverlay> overlays = mLayoutManager.getSceneOverlaysForTesting();
+        assertEquals("The overlay list should be empty.", 0, overlays.size());
+
+        // Use different class names so the overlays can be uniquely identified.
+        SceneOverlay overlay1 = Mockito.mock(SceneOverlay.class);
+        SceneOverlay overlay2 = Mockito.mock(TopToolbarOverlayCoordinator.class);
+        SceneOverlay overlay3 = Mockito.mock(ScrollingBottomViewSceneLayer.class);
+        SceneOverlay overlay4 = Mockito.mock(ContextualSearchPanel.class);
+
+        HashMap<Class, Integer> orderMap = new HashMap<>();
+        orderMap.put(overlay1.getClass(), 0);
+        orderMap.put(overlay2.getClass(), 1);
+        orderMap.put(overlay3.getClass(), 2);
+        orderMap.put(overlay4.getClass(), 3);
+        mLayoutManager.setSceneOverlayOrderForTesting(orderMap);
+
+        // Mix up the addition of each overlay.
+        mLayoutManager.addSceneOverlay(overlay3);
+        mLayoutManager.addSceneOverlay(overlay1);
+        mLayoutManager.addSceneOverlay(overlay4);
+        mLayoutManager.addSceneOverlay(overlay2);
+
+        assertEquals("Overlay is out of order!", overlay1, overlays.get(0));
+        assertEquals("Overlay is out of order!", overlay2, overlays.get(1));
+        assertEquals("Overlay is out of order!", overlay3, overlays.get(2));
+        assertEquals("Overlay is out of order!", overlay4, overlays.get(3));
+
+        assertEquals("The overlay list should have 4 items.", 4, overlays.size());
+    }
+
+    /**
+     * Ensure the ordering in LayoutManager is order-of-addition for multiple instances of the same
+     * SceneOverlay.
+     */
+    @Test
+    public void testSceneOverlayPositions_multipleInstances() {
+        List<SceneOverlay> overlays = mLayoutManager.getSceneOverlaysForTesting();
+        assertEquals("The overlay list should be empty.", 0, overlays.size());
+
+        SceneOverlay overlay1 = Mockito.mock(SceneOverlay.class);
+        SceneOverlay overlay2 = Mockito.mock(SceneOverlay.class);
+        SceneOverlay overlay3 = Mockito.mock(SceneOverlay.class);
+
+        HashMap<Class, Integer> orderMap = new HashMap<>();
+        orderMap.put(overlay1.getClass(), 0);
+        mLayoutManager.setSceneOverlayOrderForTesting(orderMap);
+
+        // Mix up the addition of each overlay.
+        mLayoutManager.addSceneOverlay(overlay3);
+        mLayoutManager.addSceneOverlay(overlay1);
+        mLayoutManager.addSceneOverlay(overlay2);
+
+        assertEquals("Overlay is out of order!", overlay3, overlays.get(0));
+        assertEquals("Overlay is out of order!", overlay1, overlays.get(1));
+        assertEquals("Overlay is out of order!", overlay2, overlays.get(2));
+
+        assertEquals("The overlay list should have 3 items.", 3, overlays.size());
+    }
+}