Skip to content

Commit 2c1cd95

Browse files
Lijin ShenChromium LUCI CQ
authored andcommitted
[M116] Fix duplicated records of MinimizeAppAndCloseTab in CCT.
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@{chromium#321} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
1 parent eb603e0 commit 2c1cd95

File tree

2 files changed

+111
-11
lines changed

2 files changed

+111
-11
lines changed

chrome/android/java/src/org/chromium/chrome/browser/customtabs/content/CustomTabActivityNavigationController.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,11 @@ public boolean navigateOnBack() {
265265
return true;
266266
}
267267
}
268-
269-
BackPressManager.record(BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB);
268+
if (!BackPressManager.isEnabled()) {
269+
// If enabled, BackPressManager will record this internally. Otherwise, this should
270+
// be recorded manually.
271+
BackPressManager.record(BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB);
272+
}
270273
if (mTabController.dispatchBeforeUnloadIfNeeded()) {
271274
MinimizeAppAndCloseTabBackPressHandler.record(MinimizeAppAndCloseTabType.CLOSE_TAB);
272275
return true;

chrome/android/junit/src/org/chromium/chrome/browser/customtabs/content/CustomTabActivityNavigationControllerTest.java

Lines changed: 106 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,21 @@
2929
import org.chromium.base.task.test.ShadowPostTask;
3030
import org.chromium.base.test.BaseRobolectricTestRunner;
3131
import org.chromium.base.test.util.HistogramWatcher;
32+
import org.chromium.chrome.browser.back_press.BackPressManager;
3233
import org.chromium.chrome.browser.back_press.MinimizeAppAndCloseTabBackPressHandler;
3334
import org.chromium.chrome.browser.back_press.MinimizeAppAndCloseTabBackPressHandler.MinimizeAppAndCloseTabType;
3435
import org.chromium.chrome.browser.customtabs.content.CustomTabActivityNavigationController.FinishHandler;
3536
import org.chromium.chrome.browser.customtabs.content.CustomTabActivityNavigationController.FinishReason;
3637
import org.chromium.chrome.browser.customtabs.shadows.ShadowExternalNavigationDelegateImpl;
3738
import org.chromium.chrome.browser.flags.ActivityType;
39+
import org.chromium.chrome.browser.flags.CachedFeatureFlags;
40+
import org.chromium.chrome.browser.flags.ChromeFeatureList;
3841
import org.chromium.chrome.browser.tab.Tab;
42+
import org.chromium.components.browser_ui.widget.gesture.BackPressHandler;
3943
import org.chromium.url.GURL;
4044

45+
import java.util.Map;
46+
4147
/**
4248
* Unit tests for {@link CustomTabActivityNavigationController}.
4349
*
@@ -73,9 +79,40 @@ public void postDelayedTask(@TaskTraits int taskTraits, Runnable task, long dela
7379

7480
@Test
7581
public void finishes_IfBackNavigationClosesTheOnlyTabWithNoUnloadEvents() {
76-
HistogramWatcher histogramWatcher = HistogramWatcher.newSingleRecordWatcher(
77-
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
78-
MinimizeAppAndCloseTabType.MINIMIZE_APP);
82+
CachedFeatureFlags.setFeaturesForTesting(
83+
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, false));
84+
HistogramWatcher histogramWatcher =
85+
HistogramWatcher.newBuilder()
86+
.expectIntRecord(
87+
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
88+
MinimizeAppAndCloseTabType.MINIMIZE_APP)
89+
.expectIntRecord(BackPressManager.getHistogramForTesting(),
90+
BackPressManager.getHistogramValueForTesting(
91+
BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB))
92+
.build();
93+
when(mTabController.onlyOneTabRemaining()).thenReturn(true);
94+
when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(false);
95+
Assert.assertTrue(mNavigationController.getHandleBackPressChangedSupplier().get());
96+
97+
mNavigationController.navigateOnBack();
98+
histogramWatcher.assertExpected();
99+
verify(mFinishHandler).onFinish(eq(FinishReason.USER_NAVIGATION));
100+
env.tabProvider.removeTab();
101+
Assert.assertNull(env.tabProvider.getTab());
102+
Assert.assertFalse(mNavigationController.getHandleBackPressChangedSupplier().get());
103+
}
104+
105+
@Test
106+
public void finishes_IfBackNavigationClosesTheOnlyTabWithNoUnloadEvents_BackPressRefactor() {
107+
CachedFeatureFlags.setFeaturesForTesting(
108+
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, true));
109+
HistogramWatcher histogramWatcher =
110+
HistogramWatcher.newBuilder()
111+
.expectIntRecord(
112+
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
113+
MinimizeAppAndCloseTabType.MINIMIZE_APP)
114+
.expectNoRecords(BackPressManager.getHistogramForTesting())
115+
.build();
79116
when(mTabController.onlyOneTabRemaining()).thenReturn(true);
80117
when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(false);
81118
Assert.assertTrue(mNavigationController.getHandleBackPressChangedSupplier().get());
@@ -90,9 +127,41 @@ public void finishes_IfBackNavigationClosesTheOnlyTabWithNoUnloadEvents() {
90127

91128
@Test
92129
public void doesntFinish_IfBackNavigationReplacesTabWithPreviousOne() {
93-
HistogramWatcher histogramWatcher = HistogramWatcher.newSingleRecordWatcher(
94-
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
95-
MinimizeAppAndCloseTabType.CLOSE_TAB);
130+
CachedFeatureFlags.setFeaturesForTesting(
131+
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, false));
132+
HistogramWatcher histogramWatcher =
133+
HistogramWatcher.newBuilder()
134+
.expectIntRecord(
135+
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
136+
MinimizeAppAndCloseTabType.CLOSE_TAB)
137+
.expectIntRecord(BackPressManager.getHistogramForTesting(),
138+
BackPressManager.getHistogramValueForTesting(
139+
BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB))
140+
.build();
141+
doAnswer((Answer<Void>) invocation -> {
142+
env.tabProvider.swapTab(env.prepareTab());
143+
return null;
144+
})
145+
.when(mTabController)
146+
.closeTab();
147+
Assert.assertTrue(mNavigationController.getHandleBackPressChangedSupplier().get());
148+
149+
mNavigationController.navigateOnBack();
150+
histogramWatcher.assertExpected();
151+
verify(mFinishHandler, never()).onFinish(anyInt());
152+
}
153+
154+
@Test
155+
public void doesntFinish_IfBackNavigationReplacesTabWithPreviousOne_BackPressRefactor() {
156+
CachedFeatureFlags.setFeaturesForTesting(
157+
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, true));
158+
HistogramWatcher histogramWatcher =
159+
HistogramWatcher.newBuilder()
160+
.expectIntRecord(
161+
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
162+
MinimizeAppAndCloseTabType.CLOSE_TAB)
163+
.expectNoRecords(BackPressManager.getHistogramForTesting())
164+
.build();
96165
doAnswer((Answer<Void>) invocation -> {
97166
env.tabProvider.swapTab(env.prepareTab());
98167
return null;
@@ -106,9 +175,37 @@ public void doesntFinish_IfBackNavigationReplacesTabWithPreviousOne() {
106175

107176
@Test
108177
public void doesntFinish_IfBackNavigationHappensWithBeforeUnloadHandler() {
109-
HistogramWatcher histogramWatcher = HistogramWatcher.newSingleRecordWatcher(
110-
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
111-
MinimizeAppAndCloseTabType.CLOSE_TAB);
178+
CachedFeatureFlags.setFeaturesForTesting(
179+
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, false));
180+
HistogramWatcher histogramWatcher =
181+
HistogramWatcher.newBuilder()
182+
.expectIntRecord(
183+
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
184+
MinimizeAppAndCloseTabType.CLOSE_TAB)
185+
.expectIntRecord(BackPressManager.getHistogramForTesting(),
186+
BackPressManager.getHistogramValueForTesting(
187+
BackPressHandler.Type.MINIMIZE_APP_AND_CLOSE_TAB))
188+
.build();
189+
190+
when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(true);
191+
192+
mNavigationController.navigateOnBack();
193+
histogramWatcher.assertExpected();
194+
verify(mFinishHandler, never()).onFinish(anyInt());
195+
}
196+
197+
@Test
198+
public void doesntFinish_IfBackNavigationHappensWithBeforeUnloadHandler_BackPressRefactor() {
199+
CachedFeatureFlags.setFeaturesForTesting(
200+
Map.of(ChromeFeatureList.BACK_GESTURE_REFACTOR, true));
201+
HistogramWatcher histogramWatcher =
202+
HistogramWatcher.newBuilder()
203+
.expectIntRecord(
204+
MinimizeAppAndCloseTabBackPressHandler.getHistogramNameForTesting(),
205+
MinimizeAppAndCloseTabType.CLOSE_TAB)
206+
.expectNoRecords(BackPressManager.getHistogramForTesting())
207+
.build();
208+
112209
when(mTabController.dispatchBeforeUnloadIfNeeded()).thenReturn(true);
113210

114211
mNavigationController.navigateOnBack();

0 commit comments

Comments
 (0)