From 44df9c8d303febe6952229ba858959eecb1475a5 Mon Sep 17 00:00:00 2001 From: Lijin Shen Date: Fri, 27 Jan 2023 21:53:12 +0000 Subject: [PATCH] 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 Commit-Queue: Lijin Shen Reviewed-by: Xi Han Reviewed-by: Yue Zhang Cr-Commit-Position: refs/heads/main@{#1098131} --- .../StartSurfaceMediatorUnitTest.java | 17 ++++++++++++++--- .../tab_management/TabSwitcherMediator.java | 9 ++++++++- .../chromium/chrome/browser/NavigateTest.java | 14 ++++++++++++++ .../browser/back_press/BackPressManager.java | 10 ++++++++++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/chrome/android/features/start_surface/junit/src/org/chromium/chrome/features/start_surface/StartSurfaceMediatorUnitTest.java b/chrome/android/features/start_surface/junit/src/org/chromium/chrome/features/start_surface/StartSurfaceMediatorUnitTest.java index 3ffed6077faa6..c3972a77ba249 100644 --- a/chrome/android/features/start_surface/junit/src/org/chromium/chrome/features/start_surface/StartSurfaceMediatorUnitTest.java +++ b/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()); } /** diff --git a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherMediator.java b/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherMediator.java index e728f760af303..560a130c791d9 100644 --- a/chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherMediator.java +++ b/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 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 index c17781fb099e4..bce4a8ee0a5f0 100644 --- 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/chrome/browser/back_press/android/java/src/org/chromium/chrome/browser/back_press/BackPressManager.java b/chrome/browser/back_press/android/java/src/org/chromium/chrome/browser/back_press/BackPressManager.java index bc9c98896a17c..bacddfa9ac2eb 100644 --- a/chrome/browser/back_press/android/java/src/org/chromium/chrome/browser/back_press/BackPressManager.java +++ b/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); + } }