Fix both StartSurface and TabSwitcher recorded on a single back press

StartSurface and TabSwitcher are both recorded when exiting tab switcher
when SS is enabled.

By design, a back press should be handled by only one handler.
Recording two back press handlers can cause confusion for data
analysis and debugging.

This CL makes TabSwitcher recorded only when SS is disabled.

Ref: https://crrev.com/c/4116635

Bug: 1347089
Change-Id: I46b15d74a8caf603f434dee0922d711145dac14c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4195696
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Lijin Shen <lazzzis@google.com>
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: Yue Zhang <yuezhanggg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098131}

chrome/android/features/start_surface/junit/src/org/chromium/chrome/features/start_surface/StartSurfaceMediatorUnitTest.java

@@ -76,6 +76,7 @@
 import org.chromium.base.supplier.Supplier;
 import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.base.test.util.JniMocker;
+import org.chromium.base.test.util.MetricsUtils;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.back_press.BackPressManager;
 import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
@@ -114,6 +115,7 @@
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
 import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
+import org.chromium.components.browser_ui.widget.gesture.BackPressHandler;
 import org.chromium.components.prefs.PrefService;
 import org.chromium.components.search_engines.TemplateUrlService;
 import org.chromium.ui.modelutil.PropertyKey;
@@ -1701,19 +1703,28 @@ public void testBackPressHandlerOnTabSwitcher() {
         doReturn(false).when(mCarouselOrSingleTabSwitcherModuleController).isDialogVisible();
 
         doReturn(true).when(mSecondaryTasksSurfaceController).isDialogVisible();
-        mediator.onBackPressed();
+        doReturn(true).when(mSecondaryTasksSurfaceController).onBackPressed();
+        int startSurface =
+                BackPressManager.getHistogramValueForTesting(BackPressHandler.Type.START_SURFACE);
+        MetricsUtils.HistogramDelta startSurfaceBackPressRecord = new MetricsUtils.HistogramDelta(
+                BackPressManager.getHistogramForTesting(), startSurface);
+        Assert.assertTrue(mediator.onBackPressed());
         verify(mCarouselOrSingleTabSwitcherModuleController, never()).onBackPressed();
         verify(mSecondaryTasksSurfaceController).onBackPressed();
+        Assert.assertEquals(1, startSurfaceBackPressRecord.getDelta());
 
         doReturn(false).when(mSecondaryTasksSurfaceController).isDialogVisible();
-        verify(mSecondaryTasksSurfaceController).onBackPressed();
+        doReturn(false).when(mSecondaryTasksSurfaceController).onBackPressed();
+        Assert.assertFalse(verify(mSecondaryTasksSurfaceController).onBackPressed());
+        Assert.assertEquals(1, startSurfaceBackPressRecord.getDelta());
 
         mediator.setStartSurfaceState(StartSurfaceState.SHOWN_HOMEPAGE);
         mediator.setStartSurfaceState(StartSurfaceState.SHOWN_TABSWITCHER);
         Assert.assertEquals(StartSurfaceState.SHOWN_TABSWITCHER, mediator.getStartSurfaceState());
-        mediator.onBackPressed();
+        Assert.assertTrue(mediator.onBackPressed());
         Assert.assertEquals("Should return to home page on back press.",
                 StartSurfaceState.SHOWN_HOMEPAGE, mediator.getStartSurfaceState());
+        Assert.assertEquals(2, startSurfaceBackPressRecord.getDelta());
     }
 
     /**

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherMediator.java

@@ -108,6 +108,8 @@ class TabSwitcherMediator implements TabSwitcher.Controller, TabListRecyclerView
     private final BrowserControlsStateProvider.Observer mBrowserControlsObserver;
     private final ViewGroup mContainerView;
     private final TabContentManager mTabContentManager;
+    private final boolean mIsStartSurfaceEnabled;
+    private final boolean mIsStartSurfaceRefactorEnabled;
     private final MultiWindowModeStateDispatcher mMultiWindowModeStateDispatcher;
     private final MultiWindowModeStateDispatcher.MultiWindowModeObserver mMultiWindowModeObserver;
     private final ObservableSupplierImpl<Boolean> mBackPressChangedSupplier =
@@ -292,6 +294,8 @@ interface PriceWelcomeMessageController {
         mMode = mode;
         mContainerViewModel.set(MODE, mode);
         mContext = context;
+        mIsStartSurfaceEnabled = ReturnToChromeUtil.isStartSurfaceEnabled(context);
+        mIsStartSurfaceRefactorEnabled = ReturnToChromeUtil.isStartSurfaceRefactorEnabled(context);
 
         if (incognitoReauthControllerSupplier != null) {
             mCallbackController = new CallbackController();
@@ -495,6 +499,7 @@ public void onChange() {
             mContainerViewModel.set(BOTTOM_PADDING,
                     (int) context.getResources().getDimension(R.dimen.tab_grid_bottom_padding));
             if (backPressManager != null && BackPressManager.isEnabled()) {
+                assert !mIsStartSurfaceEnabled || mIsStartSurfaceRefactorEnabled;
                 backPressManager.addHandler(this, BackPressHandler.Type.TAB_SWITCHER);
                 notifyBackPressStateChangedInternal();
             }
@@ -831,7 +836,9 @@ public void finishedHiding() {
     @Override
     public boolean onBackPressed() {
         boolean ret = onBackPressedInternal();
-        if (ret) {
+        if (ret && (!mIsStartSurfaceEnabled || mIsStartSurfaceRefactorEnabled)) {
+            // When SS is enabled or refactor is disabled, StartSurfaceMediator will consume back
+            // press and call this method if necessary.
             BackPressManager.record(BackPressHandler.Type.TAB_SWITCHER);
         }
         return ret;

chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java

@@ -34,6 +34,7 @@
 import org.chromium.base.test.util.DisableIf;
 import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
+import org.chromium.base.test.util.MetricsUtils.HistogramDelta;
 import org.chromium.base.test.util.Restriction;
 import org.chromium.base.test.util.UrlUtils;
 import org.chromium.chrome.R;
@@ -587,6 +588,15 @@ public void testNavigateBackWithTabSwitcher() throws Exception {
             navigateAndObserve(url);
         }
 
+        String histogram = BackPressManager.getHistogramForTesting();
+
+        HistogramDelta tabHistoryDelta = new HistogramDelta(histogram,
+                BackPressManager.getHistogramValueForTesting(BackPressHandler.Type.TAB_HISTORY));
+        HistogramDelta startSurfaceDelta = new HistogramDelta(histogram,
+                BackPressManager.getHistogramValueForTesting(BackPressHandler.Type.START_SURFACE));
+        HistogramDelta tabSwitcherDelta = new HistogramDelta(histogram,
+                BackPressManager.getHistogramValueForTesting(BackPressHandler.Type.TAB_SWITCHER));
+
         ChromeTabbedActivity cta = mActivityTestRule.getActivity();
         TabUiTestHelper.enterTabSwitcher(cta);
         Assert.assertTrue(cta.getLayoutManager().isLayoutVisible(LayoutType.TAB_SWITCHER));
@@ -597,6 +607,10 @@ public void testNavigateBackWithTabSwitcher() throws Exception {
             int type = mActivityTestRule.getActivity().getLayoutManager().getActiveLayoutType();
             Assert.assertEquals(LayoutType.BROWSING, type);
         });
+        Assert.assertEquals(
+                "No page navigation when exiting tab switcher.", 0, tabHistoryDelta.getDelta());
+        Assert.assertEquals("Either start surface or tab switcher handles back press.", 1,
+                startSurfaceDelta.getDelta() + tabSwitcherDelta.getDelta());
     }
 
     /**

chrome/browser/back_press/android/java/src/org/chromium/chrome/browser/back_press/BackPressManager.java

@@ -202,4 +202,14 @@ public int getLastCalledHandlerForTesting() {
     public void resetLastCalledHandlerForTesting() {
         mLastCalledHandlerForTesting = -1;
     }
+
+    @VisibleForTesting
+    public static String getHistogramForTesting() {
+        return HISTOGRAM;
+    }
+
+    @VisibleForTesting
+    public static int getHistogramValueForTesting(int type) {
+        return sMetricsMap.get(type);
+    }
 }