[Settings] Update site settings to use the method to finish
To support the single-activity mode where a single instance of the
settings activity is used throughout user navigation, settings pages
should use the new method to finish the current fragment.
We used to have GroupedWebsitesActivityHolder that uses WeakReference to
keep track of an activity to finish when the user needs to go back 2
steps, but we have a few problems with it:
- The new API takes a Fragment, not an Activity, to call.
- It is incorrect to use WeakReference in the single-activity mode
because the same instance of SettingsActivity is shared across
screens.
Therefore, this patch removes GroupedWebsitesActivityHolder, and instead
modifies GroupedWebsitesSettings to keep track of a paused fragment that
can be retrieved by GroupedWebsitesSettings.getPausedInstance().
Bug: b/356743945
Change-Id: Iad51b4d84354177389876a9c05a6162b80b51b9d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5890712
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1361477}
diff --git a/components/browser_ui/site_settings/android/BUILD.gn b/components/browser_ui/site_settings/android/BUILD.gn
index 101d6700..b220698 100644
--- a/components/browser_ui/site_settings/android/BUILD.gn
+++ b/components/browser_ui/site_settings/android/BUILD.gn
@@ -67,7 +67,6 @@
"java/src/org/chromium/components/browser_ui/site_settings/CookiesInfo.java",
"java/src/org/chromium/components/browser_ui/site_settings/DesktopSiteMetrics.java",
"java/src/org/chromium/components/browser_ui/site_settings/ForwardingManagedPreferenceDelegate.java",
- "java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesActivityHolder.java",
"java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesSettings.java",
"java/src/org/chromium/components/browser_ui/site_settings/LocalStorageInfo.java",
"java/src/org/chromium/components/browser_ui/site_settings/LocationCategory.java",
diff --git a/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesActivityHolder.java b/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesActivityHolder.java
deleted file mode 100644
index 3b5d331..0000000
--- a/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesActivityHolder.java
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright 2023 The Chromium Authors
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.components.browser_ui.site_settings;
-
-import android.app.Activity;
-
-import androidx.annotation.Nullable;
-
-import org.chromium.base.ThreadUtils;
-
-import java.lang.ref.WeakReference;
-
-// A singleton class that holds a WeakReference to the Activity object of GroupedWebsiteSettings.
-// Needed to be able to go to the 'All Sites' level when clearing data in SingleWebsiteSettings.
-class GroupedWebsitesActivityHolder {
- @Nullable private WeakReference<Activity> mActivity;
-
- private static GroupedWebsitesActivityHolder sInstance;
-
- private GroupedWebsitesActivityHolder() {}
- ;
-
- public static GroupedWebsitesActivityHolder getInstance() {
- ThreadUtils.assertOnUiThread();
- if (sInstance == null) sInstance = new GroupedWebsitesActivityHolder();
- return sInstance;
- }
-
- public void setActivity(Activity activity) {
- mActivity = new WeakReference<Activity>(activity);
- }
-
- public Activity getActivity() {
- return mActivity.get();
- }
-}
diff --git a/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesSettings.java b/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesSettings.java
index 8701d86..a1016e4 100644
--- a/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesSettings.java
+++ b/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/GroupedWebsitesSettings.java
@@ -16,6 +16,7 @@
import androidx.preference.PreferenceCategory;
import org.chromium.base.Callback;
+import org.chromium.base.ThreadUtils;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.ObservableSupplierImpl;
@@ -39,12 +40,25 @@
public static final String PREF_SITES_IN_GROUP = "sites_in_group";
public static final String PREF_RESET_GROUP = "reset_group_button";
+ private static GroupedWebsitesSettings sPausedInstance;
+
private WebsiteGroup mSiteGroup;
private Dialog mConfirmationDialog;
private final ObservableSupplierImpl<String> mPageTitle = new ObservableSupplierImpl<>();
+ /**
+ * Returns a paused instance of GroupedWebsitesSettings, if any.
+ *
+ * <p>This is used by {@link SingleWebsiteSettings} to go to the 'All Sites' level when clearing
+ * data.
+ */
+ public static GroupedWebsitesSettings getPausedInstance() {
+ ThreadUtils.assertOnUiThread();
+ return sPausedInstance;
+ }
+
@Override
public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
// Remove this Preference if it gets restored without a valid SiteSettingsDelegate. This
@@ -87,11 +101,26 @@
}
@Override
+ public void onResume() {
+ super.onResume();
+ sPausedInstance = null;
+ }
+
+ @Override
+ public void onPause() {
+ super.onPause();
+ sPausedInstance = this;
+ }
+
+ @Override
+ public void onDestroy() {
+ super.onDestroy();
+ sPausedInstance = null;
+ }
+
+ @Override
public boolean onPreferenceTreeClick(Preference preference) {
if (preference instanceof WebsiteRowPreference) {
- // Handle a click on one of the sites in this group.
- // Save the current activity, so it's accessible from the SingleWebsiteSettings.
- GroupedWebsitesActivityHolder.getInstance().setActivity(getActivity());
((WebsiteRowPreference) preference)
.handleClick(getArguments(), /* fromGrouped= */ true);
}
@@ -158,17 +187,13 @@
private final Runnable mDataClearedCallback =
() -> {
- Activity activity = getActivity();
- if (activity == null || activity.isFinishing()) {
- return;
- }
// TODO(crbug.com/40231223): This always navigates the user back to the "All sites"
// page regardless of whether there are any non-resettable permissions left in the
// sites within the group. Consider calculating those and refreshing the screen in
// place for a slightly smoother user experience. However, due to the complexity
// involved in refreshing the already fetched data and a very marginal benefit, it
// may not be worth it.
- getActivity().finish();
+ getSettingsLauncher().finishCurrentFragment(this);
};
@VisibleForTesting
diff --git a/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/SingleWebsiteSettings.java b/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/SingleWebsiteSettings.java
index d264647a..dd88eef3 100644
--- a/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/SingleWebsiteSettings.java
+++ b/components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/SingleWebsiteSettings.java
@@ -1287,10 +1287,12 @@
private void popBackIfNoSettings() {
if (!hasPermissionsPreferences() && !hasUsagePreferences() && getActivity() != null) {
- getActivity().finish();
- if (mFromGrouped) {
- Activity groupActivity = GroupedWebsitesActivityHolder.getInstance().getActivity();
- if (groupActivity != null) groupActivity.finish();
+ // Save the paused fragment before finishing the current fragment as it may cause the
+ // paused fragment to resume.
+ GroupedWebsitesSettings groupFragment = GroupedWebsitesSettings.getPausedInstance();
+ getSettingsLauncher().finishCurrentFragment(this);
+ if (mFromGrouped && groupFragment != null) {
+ getSettingsLauncher().finishCurrentFragment(groupFragment);
}
}
}
@@ -1374,10 +1376,12 @@
DeleteBrowsingDataAction.SITES_SETTINGS_PAGE,
DeleteBrowsingDataAction.MAX_VALUE);
if (finishActivityImmediately) {
- getActivity().finish();
- if (mFromGrouped) {
- Activity groupActivity = GroupedWebsitesActivityHolder.getInstance().getActivity();
- if (groupActivity != null) groupActivity.finish();
+ // Save the paused fragment before finishing the current fragment as it may cause the
+ // paused fragment to resume.
+ GroupedWebsitesSettings groupFragment = GroupedWebsitesSettings.getPausedInstance();
+ getSettingsLauncher().finishCurrentFragment(this);
+ if (mFromGrouped && groupFragment != null) {
+ getSettingsLauncher().finishCurrentFragment(groupFragment);
}
}
}
