Skip to content

Commit

Permalink
[M116]Fix incorrect tab supplier is used by ToolbarTabController
Browse files Browse the repository at this point in the history
When back press refactor is disabled, activity tab provider should be
used as well if the back press with activity tab provider is enabled.

(cherry picked from commit ba67d8f)

Bug: 1410137
Change-Id: If0652a97f2258357cfc337267ce9476e5cfcd617
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4656747
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Lijin Shen <lazzzis@google.com>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/heads/main@{#1164401}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4672343
Cr-Commit-Position: refs/branch-heads/5845@{#395}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Lijin Shen authored and Chromium LUCI CQ committed Jul 10, 2023
1 parent d342a13 commit de37665
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2331,6 +2331,8 @@ private void recordLauncherShortcutAction(boolean isIncognito) {

@Override
public boolean handleBackPressed() {
// Back press event should be handled through the back press handler.
assert !BackPressManager.isEnabled() : "Incorrect way of handling back press.";
if (!mUIWithNativeInitialized) {
RecordUserAction.record("SystemBackBeforeUINativeInitialized");
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
import org.chromium.chrome.browser.flags.CachedFeatureFlags;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.homepage.HomepageManager;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.profiles.ProfileJni;
Expand All @@ -44,6 +46,8 @@
import org.chromium.content_public.browser.LoadUrlParams;
import org.chromium.ui.base.PageTransition;

import java.util.HashMap;

/** Unit tests for ToolbarTabControllerImpl. */
@RunWith(BaseRobolectricTestRunner.class)
@Config(manifest = Config.NONE)
Expand All @@ -67,6 +71,8 @@ public boolean matches(LoadUrlParams argument) {
@Mock
private Tab mTab;
@Mock
private Tab mTab2;
@Mock
private Supplier<Boolean> mOverrideHomePageSupplier;
@Mock
private ObservableSupplier<BottomControlsCoordinator> mBottomControlsCoordinatorSupplier;
Expand All @@ -84,6 +90,8 @@ public boolean matches(LoadUrlParams argument) {
public Profile.Natives mMockProfileNatives;
@Mock
private NativePage mNativePage;
@Mock
private Supplier<Tab> mActivityTabProvider;

@Rule
public TestRule mProcessor = new Features.JUnitProcessor();
Expand All @@ -94,6 +102,7 @@ public boolean matches(LoadUrlParams argument) {
public void setUp() {
MockitoAnnotations.initMocks(this);
doReturn(mTab).when(mTabSupplier).get();
doReturn(mTab).when(mActivityTabProvider).get();
doReturn(false).when(mOverrideHomePageSupplier).get();
mocker.mock(ProfileJni.TEST_HOOKS, mMockProfileNatives);
doReturn(mProfile).when(mMockProfileNatives).fromWebContents(any());
Expand Down Expand Up @@ -191,9 +200,47 @@ public void openHomepage_loadsHomePage() {
new LoadUrlParams(homePageUrl, PageTransition.HOME_PAGE))));
}

@Test
public void testUsingCorrectTabSupplier() {
// Should only use regular tab supplier when back press refactor is disabled and
// control with activity tab provider is also disabled.
var featureList = new HashMap<String, Boolean>();
featureList.put(ChromeFeatureList.BACK_GESTURE_REFACTOR, false);
featureList.put(ChromeFeatureList.BACK_GESTURE_ACTIVITY_TAB_PROVIDER, false);
CachedFeatureFlags.setFeaturesForTesting(featureList);

doReturn(mTab2).when(mActivityTabProvider).get();
doReturn(false).when(mTab2).canGoBack();
doReturn(true).when(mTab).canGoBack();

Assert.assertTrue(mToolbarTabController.back());
Assert.assertTrue(mToolbarTabController.canGoBack());

featureList.put(ChromeFeatureList.BACK_GESTURE_REFACTOR, true);
featureList.put(ChromeFeatureList.BACK_GESTURE_ACTIVITY_TAB_PROVIDER, false);
CachedFeatureFlags.setFeaturesForTesting(featureList);

Assert.assertFalse(mToolbarTabController.back());
Assert.assertFalse(mToolbarTabController.canGoBack());

featureList.put(ChromeFeatureList.BACK_GESTURE_REFACTOR, false);
featureList.put(ChromeFeatureList.BACK_GESTURE_ACTIVITY_TAB_PROVIDER, true);
CachedFeatureFlags.setFeaturesForTesting(featureList);

Assert.assertFalse(mToolbarTabController.back());
Assert.assertFalse(mToolbarTabController.canGoBack());

featureList.put(ChromeFeatureList.BACK_GESTURE_REFACTOR, true);
featureList.put(ChromeFeatureList.BACK_GESTURE_ACTIVITY_TAB_PROVIDER, true);
CachedFeatureFlags.setFeaturesForTesting(featureList);

Assert.assertFalse(mToolbarTabController.back());
Assert.assertFalse(mToolbarTabController.canGoBack());
}

private void initToolbarTabController() {
mToolbarTabController = new ToolbarTabControllerImpl(mTabSupplier,
mOverrideHomePageSupplier, mTrackerSupplier, mBottomControlsCoordinatorSupplier,
ToolbarManager::homepageUrl, mRunnable, mTabSupplier);
ToolbarManager::homepageUrl, mRunnable, mActivityTabProvider);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public static boolean isSecondaryActivityEnabled() {
* @return True if ActivityTabProvider should replace ChromeTabActivity#getActivityTab
*/
public static boolean shouldUseActivityTabProvider() {
return ChromeFeatureList.sBackGestureActivityTabProvider.isEnabled();
return isEnabled() || ChromeFeatureList.sBackGestureActivityTabProvider.isEnabled();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ public boolean back() {
if (controlsCoordinator != null && controlsCoordinator.onBackPressed()) {
return true;
}
Tab tab = BackPressManager.isEnabled() ? mActivityTabSupplier.get() : mTabSupplier.get();
Tab tab = BackPressManager.shouldUseActivityTabProvider() ? mActivityTabSupplier.get()
: mTabSupplier.get();
if (tab != null && tab.canGoBack()) {
NativePage nativePage = tab.getNativePage();
if (nativePage != null) {
Expand Down Expand Up @@ -148,7 +149,8 @@ boolean canGoBack() {
controlsCoordinator.getHandleBackPressChangedSupplier().get())) {
return true;
}
Tab tab = BackPressManager.isEnabled() ? mActivityTabSupplier.get() : mTabSupplier.get();
Tab tab = BackPressManager.shouldUseActivityTabProvider() ? mActivityTabSupplier.get()
: mTabSupplier.get();
return tab != null && tab.canGoBack();
}

Expand Down

0 comments on commit de37665

Please sign in to comment.