[M111] Fix both StartSurface and TabSwitcher recorded on a single bac…

…k press Bug was introduced in M111. 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 (cherry picked from commit 44df9c8) Bug: 1347089, 1412152 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-Original-Commit-Position: refs/heads/main@{#1098131} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4220055 Cr-Commit-Position: refs/branch-heads/5563@{#122} Cr-Branched-From: 3ac59a6-refs/heads/main@{#1097615}
chromium · Feb 2, 2023 · 38c042d · 38c042d
1 parent bdcd09d
commit 38c042d
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 4 deletions.
diff --git a/...ce/junit/src/org/chromium/chrome/features/start_surface/StartSurfaceMediatorUnitTest.java b/...ce/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());
     }
 
     /**

diff --git a/...tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherMediator.java b/...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;

diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java b/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());
     }
 
     /**

diff --git a/.../back_press/android/java/src/org/chromium/chrome/browser/back_press/BackPressManager.java b/.../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);
+    }
 }