Skip to content

Commit

Permalink
[M115] Add metrics recording for secondary activity back press handling
Browse files Browse the repository at this point in the history
This CL:
1. adds "Android.BackPress.SecondaryActivity" for recording the
activity which handles the back press
2. moves BackPressHelper* to c/b/back_press
3. polishes tests
4. fixes typos in some tests

(cherry picked from commit 46fe40a)

Bug: 1406012
Change-Id: I2962d061dd9c9b419ee2aaf7b8038e83bbdc4748
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4519916
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Lijin Shen <lazzzis@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1152020}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599034
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#461}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Lijin Shen authored and Chromium LUCI CQ committed Jun 7, 2023
1 parent b75a827 commit 53c4f53
Show file tree
Hide file tree
Showing 21 changed files with 195 additions and 57 deletions.
Expand Up @@ -580,9 +580,13 @@ private void preloadAlreadyLocked(String packageName, boolean inZygote) {
*
* @deprecated: please avoid using in new code:
* https://crsrc.org/c/base/android/jni_generator/README.md#testing-for-readiness-use-get
*
* TODO(crbug.com/1406012): adding back {@link VisibleForTesting} after crbug.com/1442347 is
* fixed. This method is exposed in order to unblock the test failures because of isInitialized
* loading .so file before native flags are ready. Ideally, it should be fixed by migrating
* the feature flag to CachedFlag.
*/
@Deprecated
@VisibleForTesting
public boolean isLoaded() {
return mLoadState == LoadState.LOADED
&& (!sEnableStateForTesting || mLoadStateForTesting == LoadState.LOADED);
Expand Down
1 change: 0 additions & 1 deletion chrome/android/chrome_java_sources.gni
Expand Up @@ -10,7 +10,6 @@ chrome_java_sources = [
"java/src/org/chromium/chrome/browser/AppHooksModule.java",
"java/src/org/chromium/chrome/browser/AppIndexingUtil.java",
"java/src/org/chromium/chrome/browser/ApplicationLifetime.java",
"java/src/org/chromium/chrome/browser/BackPressHelper.java",
"java/src/org/chromium/chrome/browser/BackupSigninProcessor.java",
"java/src/org/chromium/chrome/browser/BrowserRestartActivity.java",
"java/src/org/chromium/chrome/browser/ChromeActionModeHandler.java",
Expand Down
1 change: 0 additions & 1 deletion chrome/android/chrome_junit_test_java_sources.gni
Expand Up @@ -6,7 +6,6 @@ chrome_junit_test_java_sources = [
"java/src/org/chromium/chrome/browser/segmentation_platform/SignalAccumulatorTest.java",
"java/src/org/chromium/chrome/browser/tab/TabFaviconTest.java",
"junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java",
"junit/src/org/chromium/chrome/browser/BackPressHelperUnitTest.java",
"junit/src/org/chromium/chrome/browser/ChromeActionModeHandlerUnitTest.java",
"junit/src/org/chromium/chrome/browser/ChromeActivityUnitTest.java",
"junit/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java",
Expand Down
Expand Up @@ -9,10 +9,11 @@
import android.text.TextUtils;

import org.chromium.base.IntentUtils;
import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.IntentHandler;
import org.chromium.chrome.browser.SnackbarActivity;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.back_press.BackPressManager;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.bookmarks.BookmarkManagerCoordinator;
import org.chromium.chrome.browser.bookmarks.BookmarkPage;
import org.chromium.chrome.browser.bookmarks.BookmarkUiPrefs;
Expand Down Expand Up @@ -47,10 +48,11 @@ true, isIncognito, getSnackbarManager(), Profile.getLastUsedRegularProfile(),
mBookmarkManagerCoordinator.updateForUrl(url);
setContentView(mBookmarkManagerCoordinator.getView());
if (BackPressManager.isSecondaryActivityEnabled()) {
BackPressHelper.create(this, getOnBackPressedDispatcher(), mBookmarkManagerCoordinator);
BackPressHelper.create(this, getOnBackPressedDispatcher(), mBookmarkManagerCoordinator,
SecondaryActivity.BOOKMARK);
} else {
BackPressHelper.create(
this, getOnBackPressedDispatcher(), mBookmarkManagerCoordinator::onBackPressed);
BackPressHelper.create(this, getOnBackPressedDispatcher(),
mBookmarkManagerCoordinator::onBackPressed, SecondaryActivity.BOOKMARK);
}
}

Expand Down
Expand Up @@ -7,9 +7,10 @@
import android.app.Activity;
import android.os.Bundle;

import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.SnackbarActivity;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.back_press.BackPressManager;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.download.DownloadUtils;
import org.chromium.chrome.browser.download.home.DownloadManagerCoordinator;
import org.chromium.chrome.browser.download.home.DownloadManagerUiConfig;
Expand All @@ -26,7 +27,6 @@
import org.chromium.ui.permissions.AndroidPermissionDelegate;

import java.lang.ref.WeakReference;

/**
* Activity for managing downloads handled through Chrome.
*/
Expand Down Expand Up @@ -81,10 +81,10 @@ public void onCreate(Bundle savedInstanceState) {
mDownloadCoordinator.addObserver(mUiObserver);
if (BackPressManager.isSecondaryActivityEnabled()) {
BackPressHelper.create(this, getOnBackPressedDispatcher(),
mDownloadCoordinator.getBackPressHandlers());
mDownloadCoordinator.getBackPressHandlers(), SecondaryActivity.DOWNLOAD);
} else {
BackPressHelper.create(
this, getOnBackPressedDispatcher(), mDownloadCoordinator::onBackPressed);
BackPressHelper.create(this, getOnBackPressedDispatcher(),
mDownloadCoordinator::onBackPressed, SecondaryActivity.DOWNLOAD);
}
}

Expand Down
Expand Up @@ -23,6 +23,7 @@
import org.chromium.base.Promise;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.customtabs.CustomTabActivity;
import org.chromium.chrome.browser.fonts.FontPreloader;
import org.chromium.chrome.browser.metrics.UmaUtils;
Expand Down Expand Up @@ -398,6 +399,11 @@ public void onStart() {
return BackPressResult.SUCCESS;
}

@Override
public int getSecondaryActivity() {
return SecondaryActivity.FIRST_RUN;
}

// FirstRunPageDelegate:
@Override
public Bundle getProperties() {
Expand Down
Expand Up @@ -21,8 +21,9 @@
import org.chromium.base.supplier.ObservableSupplierImpl;
import org.chromium.base.supplier.OneshotSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.back_press.BackPressManager;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.customtabs.CustomTabsConnection;
import org.chromium.chrome.browser.init.AsyncInitializationActivity;
import org.chromium.chrome.browser.metrics.SimpleStartupForegroundSessionDetector;
Expand Down Expand Up @@ -109,12 +110,13 @@ public void triggerLayoutInflation() {
protected void onPreCreate() {
super.onPreCreate();
if (BackPressManager.isSecondaryActivityEnabled()) {
BackPressHelper.create(this, getOnBackPressedDispatcher(), this);
BackPressHelper.create(
this, getOnBackPressedDispatcher(), this, getSecondaryActivity());
} else {
BackPressHelper.create(this, getOnBackPressedDispatcher(), () -> {
handleBackPress();
return true;
});
}, getSecondaryActivity());
}
}

Expand Down Expand Up @@ -169,6 +171,8 @@ public ObservableSupplier<Boolean> getHandleBackPressChangedSupplier() {
@Override
public abstract @BackPressResult int handleBackPress();

public abstract @SecondaryActivity int getSecondaryActivity();

protected void flushPersistentData() {
if (mNativeInitialized) {
ProfileManagerUtils.flushPersistentDataForAllProfiles();
Expand Down
Expand Up @@ -22,6 +22,7 @@
import org.chromium.base.ThreadUtils;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.customtabs.CustomTabActivity;
import org.chromium.chrome.browser.enterprise.util.EnterpriseInfo;
import org.chromium.ui.base.LocalizationUtils;
Expand Down Expand Up @@ -220,6 +221,11 @@ public void onDestroy() {
return BackPressResult.SUCCESS;
}

@Override
public int getSecondaryActivity() {
return SecondaryActivity.LIGHTWEIGHT_FIRST_RUN;
}

private void abortFirstRunExperience() {
finish();
notifyCustomTabCallbackFirstRunIfNecessary(getIntent(), false);
Expand Down
Expand Up @@ -9,10 +9,11 @@
import androidx.annotation.VisibleForTesting;

import org.chromium.base.IntentUtils;
import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.IntentHandler;
import org.chromium.chrome.browser.SnackbarActivity;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.back_press.BackPressManager;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.history_clusters.HistoryClustersConstants;
import org.chromium.chrome.browser.profiles.Profile;

Expand All @@ -37,10 +38,11 @@ public void onCreate(Bundle savedInstanceState) {
new BrowsingHistoryBridge(Profile.getLastUsedRegularProfile()));
setContentView(mHistoryManager.getView());
if (BackPressManager.isSecondaryActivityEnabled()) {
BackPressHelper.create(this, getOnBackPressedDispatcher(), mHistoryManager);
} else {
BackPressHelper.create(
this, getOnBackPressedDispatcher(), mHistoryManager::onBackPressed);
this, getOnBackPressedDispatcher(), mHistoryManager, SecondaryActivity.HISTORY);
} else {
BackPressHelper.create(this, getOnBackPressedDispatcher(),
mHistoryManager::onBackPressed, SecondaryActivity.HISTORY);
}
}

Expand Down
Expand Up @@ -690,7 +690,6 @@ public boolean onBackPressed() {
} else if (isHistoryClustersUIShowing()) {
return mHistoryClustersCoordinator.onBackPressed();
}

return mSelectableListLayout.onBackPressed();
}

Expand Down
Expand Up @@ -34,12 +34,13 @@
import org.chromium.base.IntentUtils;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ApplicationLifetime;
import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.ChromeBaseAppCompatActivity;
import org.chromium.chrome.browser.IntentHandler;
import org.chromium.chrome.browser.LaunchIntentDispatcher;
import org.chromium.chrome.browser.accessibility.settings.ChromeAccessibilitySettingsDelegate;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.back_press.BackPressManager;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.browsing_data.ClearBrowsingDataFragmentBasic;
import org.chromium.chrome.browser.feedback.FragmentHelpAndFeedbackLauncher;
import org.chromium.chrome.browser.feedback.HelpAndFeedbackLauncherImpl;
Expand Down Expand Up @@ -398,12 +399,14 @@ private void initBackPressHandler() {
if (BackPressManager.isSecondaryActivityEnabled()) {
if (activeFragment instanceof BackPressHandler) {
BackPressHelper.create(activeFragment.getViewLifecycleOwner(),
getOnBackPressedDispatcher(), (BackPressHandler) activeFragment);
getOnBackPressedDispatcher(), (BackPressHandler) activeFragment,
SecondaryActivity.SETTINGS);
}
} else if (activeFragment instanceof BackPressHelper.ObsoleteBackPressedHandler) {
BackPressHelper.create(activeFragment.getViewLifecycleOwner(),
getOnBackPressedDispatcher(),
(BackPressHelper.ObsoleteBackPressedHandler) activeFragment);
(BackPressHelper.ObsoleteBackPressedHandler) activeFragment,
SecondaryActivity.SETTINGS);
}
}

Expand Down
Expand Up @@ -35,9 +35,9 @@
import org.chromium.base.task.TaskTraits;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.AppHooks;
import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.SyncFirstSetupCompleteSource;
import org.chromium.chrome.browser.autofill.PersonalDataManager;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.feedback.FragmentHelpAndFeedbackLauncher;
import org.chromium.chrome.browser.feedback.HelpAndFeedbackLauncher;
import org.chromium.chrome.browser.profiles.Profile;
Expand Down
Expand Up @@ -52,10 +52,12 @@

import org.chromium.base.Promise;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.HistogramWatcher;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.BackPressHelper;
import org.chromium.chrome.browser.IntentHandler;
import org.chromium.chrome.browser.back_press.BackPressHelper;
import org.chromium.chrome.browser.back_press.SecondaryActivityBackPressUma.SecondaryActivity;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.history_clusters.HistoryClustersBridge;
import org.chromium.chrome.browser.history_clusters.HistoryClustersCoordinator;
Expand Down Expand Up @@ -203,10 +205,11 @@ public void setUp() throws Exception {
// Some individual tests may override to enable this feature which is disabled by
// the class by default.
if (ChromeFeatureList.isEnabled(ChromeFeatureList.BACK_GESTURE_REFACTOR_ACTIVITY)) {
BackPressHelper.create(mLifecycleOwner, mOnBackPressedDispatcher, mHistoryManager);
BackPressHelper.create(mLifecycleOwner, mOnBackPressedDispatcher, mHistoryManager,
SecondaryActivity.HISTORY);
} else {
BackPressHelper.create(
mLifecycleOwner, mOnBackPressedDispatcher, mHistoryManager::onBackPressed);
BackPressHelper.create(mLifecycleOwner, mOnBackPressedDispatcher,
mHistoryManager::onBackPressed, SecondaryActivity.HISTORY);
}
}

Expand Down Expand Up @@ -478,6 +481,7 @@ public void testSearchView() throws Exception {

@Test
@SmallTest
@DisableFeatures({ChromeFeatureList.BACK_GESTURE_REFACTOR_ACTIVITY})
public void testSearchViewDismissedByBackPress() {
final HistoryManagerToolbar toolbar = mHistoryManager.getToolbarForTests();
View toolbarShadow = mHistoryManager.getSelectableListLayout().getToolbarShadowForTests();
Expand All @@ -494,17 +498,23 @@ public void testSearchViewDismissedByBackPress() {
Assert.assertEquals(View.GONE, toolbarSearchView.getVisibility());

// Press back press to unselect item and the search view is showing again.
var backPressRecorder = HistogramWatcher.newSingleRecordWatcher(
"Android.BackPress.SecondaryActivity", SecondaryActivity.HISTORY);
Assert.assertTrue(mHistoryManager.getHandleBackPressChangedSupplier().get());
TestThreadUtils.runOnUiThreadBlocking(mOnBackPressedDispatcher::onBackPressed);
Assert.assertFalse(mHistoryManager.getSelectionDelegateForTests().isSelectionEnabled());
Assert.assertEquals(View.GONE, toolbarShadow.getVisibility());
Assert.assertEquals(View.VISIBLE, toolbarSearchView.getVisibility());
backPressRecorder.assertExpected();

// Press back to close the search view.
var backPressRecorder2 = HistogramWatcher.newSingleRecordWatcher(
"Android.BackPress.SecondaryActivity", SecondaryActivity.HISTORY);
Assert.assertTrue(mHistoryManager.getHandleBackPressChangedSupplier().get());
TestThreadUtils.runOnUiThreadBlocking(mOnBackPressedDispatcher::onBackPressed);
Assert.assertEquals(View.GONE, toolbarShadow.getVisibility());
Assert.assertEquals(View.GONE, toolbarSearchView.getVisibility());
backPressRecorder2.assertExpected();
}

@Test
Expand Down
11 changes: 10 additions & 1 deletion chrome/browser/back_press/android/BUILD.gn
Expand Up @@ -6,8 +6,10 @@ import("//build/config/android/rules.gni")

android_library("java") {
sources = [
"java/src/org/chromium/chrome/browser/back_press/BackPressHelper.java",
"java/src/org/chromium/chrome/browser/back_press/BackPressManager.java",
"java/src/org/chromium/chrome/browser/back_press/MinimizeAppAndCloseTabBackPressHandler.java",
"java/src/org/chromium/chrome/browser/back_press/SecondaryActivityBackPressUma.java",
]

deps = [
Expand All @@ -21,11 +23,15 @@ android_library("java") {
"//content/public/android:content_full_java",
"//third_party/androidx:androidx_activity_activity_java",
"//third_party/androidx:androidx_annotation_annotation_java",
"//third_party/androidx:androidx_lifecycle_lifecycle_common_java",
]
}

robolectric_library("junit") {
sources = [ "java/src/org/chromium/chrome/browser/back_press/BackPressManagerUnitTest.java" ]
sources = [
"java/src/org/chromium/chrome/browser/back_press/BackPressHelperUnitTest.java",
"java/src/org/chromium/chrome/browser/back_press/BackPressManagerUnitTest.java",
]

deps = [
":java",
Expand All @@ -36,7 +42,10 @@ robolectric_library("junit") {
"//chrome/browser/flags:java",
"//components/browser_ui/widget/android:java",
"//third_party/androidx:androidx_activity_activity_java",
"//third_party/androidx:androidx_lifecycle_lifecycle_common_java",
"//third_party/androidx:androidx_lifecycle_lifecycle_runtime_java",
"//third_party/junit:junit",
"//third_party/mockito:mockito_java",
]
}

Expand Down

0 comments on commit 53c4f53

Please sign in to comment.