Skip to content

Commit

Permalink
[M116] Fix duplicated records of MinimizeAppAndCloseTab in CCT.
Browse files Browse the repository at this point in the history 
In CCT, when refactor is enabled, the MinimizeAppAndCloseTab should be
recorded by BackPressManager internally rather than by the client
manually.

(cherry picked from commit 5208210)

Bug: 1458077
Change-Id: Ib8e052a863e655f480fd020e0225a0a3f35b76ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4647876
Code-Coverage: Findit <[email protected]>
Commit-Queue: Lijin Shen <[email protected]>
Reviewed-by: Theresa Sullivan <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1163189}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4667666
Cr-Commit-Position: refs/branch-heads/5845@{#321}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Lijin Shen authored and Chromium LUCI CQ committed Jul 5, 2023
1 parent eb603e0 commit 2c1cd95
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 11 deletions.
7 changes: 5 additions & 2 deletions ...org/chromium/chrome/browser/customtabs/content/CustomTabActivityNavigationController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,11 @@ public boolean navigateOnBack() {
return true;
}
}

BackPressManager.record(BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB);
if (!BackPressManager.isEnabled()) {
// If enabled, BackPressManager will record this internally. Otherwise, this should
// be recorded manually.
BackPressManager.record(BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB);
}
if (mTabController.dispatchBeforeUnloadIfNeeded()) {
MinimizeAppAndCloseTabBackPressHandler.record(MinimizeAppAndCloseTabType.CLOSE_TAB);
return true;
Expand Down
115 changes: 106 additions & 9 deletions ...chromium/chrome/browser/customtabs/content/CustomTabActivityNavigationControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,21 @@
import org.chromium.base.task.test.ShadowPostTask;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.HistogramWatcher;
import org.chromium.chrome.browser.back_press.BackPressManager;
import org.chromium.chrome.browser.back_press.MinimizeAppAndCloseTabBackPressHandler;
import org.chromium.chrome.browser.back_press.MinimizeAppAndCloseTabBackPressHandler.MinimizeAppAndCloseTabType;
import org.chromium.chrome.browser.customtabs.content.CustomTabActivityNavigationController.FinishHandler;
import org.chromium.chrome.browser.customtabs.content.CustomTabActivityNavigationController.FinishReason;
import org.chromium.chrome.browser.customtabs.shadows.ShadowExternalNavigationDelegateImpl;
import org.chromium.chrome.browser.flags.ActivityType;
import org.chromium.chrome.browser.flags.CachedFeatureFlags;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.components.browser_ui.widget.gesture.BackPressHandler;
import org.chromium.url.GURL;

import java.util.Map;

/**
* Unit tests for {@link CustomTabActivityNavigationController}.
*
Expand Down Expand Up @@ -73,9 +79,40 @@ public void postDelayedTask(@TaskTraits int taskTraits, Runnable task, long dela

@Test
public void finishes_IfBackNavigationClosesTheOnlyTabWithNoUnloadEvents() {
HistogramWatcher histogramWatcher = HistogramWatcher.newSingleRecordWatcher(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.MINIMIZE_APP);
CachedFeatureFlags.setFeaturesForTesting(
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, false));
HistogramWatcher histogramWatcher =
HistogramWatcher.newBuilder()
.expectIntRecord(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.MINIMIZE_APP)
.expectIntRecord(BackPressManager.getHistogramForTesting(),
BackPressManager.getHistogramValueForTesting(
BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB))
.build();
when(mTabController.onlyOneTabRemaining()).thenReturn(true);
when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(false);
Assert.assertTrue(mNavigationController.getHandleBackPressChangedSupplier().get());

mNavigationController.navigateOnBack();
histogramWatcher.assertExpected();
verify(mFinishHandler).onFinish(eq(FinishReason.USER_NAVIGATION));
env.tabProvider.removeTab();
Assert.assertNull(env.tabProvider.getTab());
Assert.assertFalse(mNavigationController.getHandleBackPressChangedSupplier().get());
}

@Test
public void finishes_IfBackNavigationClosesTheOnlyTabWithNoUnloadEvents_BackPressRefactor() {
CachedFeatureFlags.setFeaturesForTesting(
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, true));
HistogramWatcher histogramWatcher =
HistogramWatcher.newBuilder()
.expectIntRecord(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.MINIMIZE_APP)
.expectNoRecords(BackPressManager.getHistogramForTesting())
.build();
when(mTabController.onlyOneTabRemaining()).thenReturn(true);
when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(false);
Assert.assertTrue(mNavigationController.getHandleBackPressChangedSupplier().get());
Expand All @@ -90,9 +127,41 @@ public void finishes_IfBackNavigationClosesTheOnlyTabWithNoUnloadEvents() {

@Test
public void doesntFinish_IfBackNavigationReplacesTabWithPreviousOne() {
HistogramWatcher histogramWatcher = HistogramWatcher.newSingleRecordWatcher(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.CLOSE_TAB);
CachedFeatureFlags.setFeaturesForTesting(
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, false));
HistogramWatcher histogramWatcher =
HistogramWatcher.newBuilder()
.expectIntRecord(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.CLOSE_TAB)
.expectIntRecord(BackPressManager.getHistogramForTesting(),
BackPressManager.getHistogramValueForTesting(
BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB))
.build();
doAnswer((Answer<Void>) invocation -> {
env.tabProvider.swapTab(env.prepareTab());
return null;
})
.when(mTabController)
.closeTab();
Assert.assertTrue(mNavigationController.getHandleBackPressChangedSupplier().get());

mNavigationController.navigateOnBack();
histogramWatcher.assertExpected();
verify(mFinishHandler, never()).onFinish(anyInt());
}

@Test
public void doesntFinish_IfBackNavigationReplacesTabWithPreviousOne_BackPressRefactor() {
CachedFeatureFlags.setFeaturesForTesting(
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, true));
HistogramWatcher histogramWatcher =
HistogramWatcher.newBuilder()
.expectIntRecord(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.CLOSE_TAB)
.expectNoRecords(BackPressManager.getHistogramForTesting())
.build();
doAnswer((Answer<Void>) invocation -> {
env.tabProvider.swapTab(env.prepareTab());
return null;
Expand All @@ -106,9 +175,37 @@ public void doesntFinish_IfBackNavigationReplacesTabWithPreviousOne() {

@Test
public void doesntFinish_IfBackNavigationHappensWithBeforeUnloadHandler() {
HistogramWatcher histogramWatcher = HistogramWatcher.newSingleRecordWatcher(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.CLOSE_TAB);
CachedFeatureFlags.setFeaturesForTesting(
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, false));
HistogramWatcher histogramWatcher =
HistogramWatcher.newBuilder()
.expectIntRecord(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.CLOSE_TAB)
.expectIntRecord(BackPressManager.getHistogramForTesting(),
BackPressManager.getHistogramValueForTesting(
BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB))
.build();

when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(true);

mNavigationController.navigateOnBack();
histogramWatcher.assertExpected();
verify(mFinishHandler, never()).onFinish(anyInt());
}

@Test
public void doesntFinish_IfBackNavigationHappensWithBeforeUnloadHandler_BackPressRefactor() {
CachedFeatureFlags.setFeaturesForTesting(
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, true));
HistogramWatcher histogramWatcher =
HistogramWatcher.newBuilder()
.expectIntRecord(
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
MinimizeAppAndCloseTabType.CLOSE_TAB)
.expectNoRecords(BackPressManager.getHistogramForTesting())
.build();

when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(true);

mNavigationController.navigateOnBack();
Expand Down

0 comments on commit 2c1cd95

Please sign in to comment.