Move SharedPrefs keys from SigninHelper to ChromePreferenceKeys.

Use SharedPreferencesManager consistently instead of SharedPreferences
directly. Move the usages to methods in SigninPreferencesManager.

Bug: 1022108, 1034960
Change-Id: I24b5d9f20d847e7c3c8573b0cd7e712e114e3d8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1988603
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733433}

chrome/android/chrome_junit_test_java_sources.gni

@@ -191,6 +191,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/signin/ConfirmSyncDataStateMachineTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SignOutDialogFragmentTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SigninManagerTest.java",
+  "junit/src/org/chromium/chrome/browser/signin/SigninPreferencesManagerTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SigninPromoUtilTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SigninUtilsStartActivityTest.java",
   "junit/src/org/chromium/chrome/browser/suggestions/SuggestionsImageFetcherTest.java",

chrome/android/java/src/org/chromium/chrome/browser/services/AccountsChangedReceiver.java

@@ -17,6 +17,7 @@ import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
 import org.chromium.chrome.browser.init.EmptyBrowserParts;
 import org.chromium.chrome.browser.signin.IdentityServicesProvider;
 import org.chromium.chrome.browser.signin.SigninHelper;
+import org.chromium.chrome.browser.signin.SigninPreferencesManager;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 /**
@@ -52,7 +53,7 @@ public class AccountsChangedReceiver extends BroadcastReceiver {
             startBrowserIfNeededAndValidateAccounts();
         } else {
             // Notify SigninHelper of changed accounts (via shared prefs).
-            SigninHelper.markAccountsChangedPref();
+            SigninPreferencesManager.getInstance().markAccountsChangedPref();
         }
     }
 
@@ -73,7 +74,7 @@ public class AccountsChangedReceiver extends BroadcastReceiver {
             public void onStartupFailure() {
                 // Startup failed. So notify SigninHelper of changed accounts via
                 // shared prefs.
-                SigninHelper.markAccountsChangedPref();
+                SigninPreferencesManager.getInstance().markAccountsChangedPref();
             }
         };
         ChromeBrowserInitializer.getInstance().handlePreNativeStartup(parts);

chrome/android/java/src/org/chromium/chrome/browser/services/GoogleServicesManager.java

@@ -17,6 +17,7 @@ import org.chromium.base.TraceEvent;
 import org.chromium.chrome.browser.signin.IdentityServicesProvider;
 import org.chromium.chrome.browser.signin.SigninHelper;
 import org.chromium.chrome.browser.signin.SigninManager;
+import org.chromium.chrome.browser.signin.SigninPreferencesManager;
 import org.chromium.chrome.browser.sync.SyncController;
 import org.chromium.components.signin.ChromeSigninController;
 import org.chromium.components.signin.metrics.SignoutReason;
@@ -101,7 +102,8 @@ public class GoogleServicesManager implements ApplicationStateListener {
     public void onMainActivityStart() {
         try {
             TraceEvent.begin("GoogleServicesManager.onMainActivityStart");
-            boolean accountsChanged = SigninHelper.checkAndClearAccountsChangedPref();
+            boolean accountsChanged =
+                    SigninPreferencesManager.getInstance().checkAndClearAccountsChangedPref();
             mSigninHelper.validateAccountSettings(accountsChanged);
         } finally {
             TraceEvent.end("GoogleServicesManager.onMainActivityStart");

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java

@@ -7,7 +7,6 @@ package org.chromium.chrome.browser.signin;
 import android.accounts.Account;
 import android.annotation.SuppressLint;
 import android.content.Context;
-import android.content.SharedPreferences;
 
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
@@ -42,17 +41,6 @@ public class SigninHelper {
 
     private static final Object LOCK = new Object();
 
-    private static final String ACCOUNTS_CHANGED_PREFS_KEY = "prefs_sync_accounts_changed";
-
-    // Key to the shared pref that holds the new account's name if the currently signed
-    // in account has been renamed.
-    private static final String ACCOUNT_RENAMED_PREFS_KEY = "prefs_sync_account_renamed";
-
-    // Key to the shared pref that holds the last read index of all the account changed
-    // events of the current signed in account.
-    private static final String ACCOUNT_RENAME_EVENT_INDEX_PREFS_KEY =
-            "prefs_sync_account_rename_event_index";
-
     @SuppressLint("StaticFieldLeak")
     private static SigninHelper sInstance;
 
@@ -101,6 +89,8 @@ public class SigninHelper {
 
     private final AccountTrackerService mAccountTrackerService;
 
+    private final SigninPreferencesManager mPrefsManager;
+
     public static SigninHelper get() {
         synchronized (LOCK) {
             if (sInstance == null) {
@@ -115,6 +105,7 @@ public class SigninHelper {
         mSigninManager = IdentityServicesProvider.get().getSigninManager();
         mAccountTrackerService = IdentityServicesProvider.get().getAccountTrackerService();
         mChromeSigninController = ChromeSigninController.get();
+        mPrefsManager = SigninPreferencesManager.getInstance();
     }
 
     public void validateAccountSettings(boolean accountsChanged) {
@@ -142,7 +133,7 @@ public class SigninHelper {
             return;
         }
 
-        String renamedAccount = getNewSignedInAccountName();
+        String renamedAccount = mPrefsManager.getNewSignedInAccountName();
         if (accountsChanged && renamedAccount != null) {
             handleAccountRename(
                     ChromeSigninController.get().getSignedInAccountName(), renamedAccount);
@@ -162,7 +153,7 @@ public class SigninHelper {
 
                 @Override
                 protected void onPostExecute(Void result) {
-                    String renamedAccount = getNewSignedInAccountName();
+                    String renamedAccount = mPrefsManager.getNewSignedInAccountName();
                     if (renamedAccount != null || mSigninManager.isOperationInProgress()) {
                         // Found account rename event or there's a sign-in/sign-out operation in
                         // progress. Restart validation process.
@@ -201,7 +192,7 @@ public class SigninHelper {
             // If Chrome dies, we can try it again on next run.
             // Otherwise, if re-sign-in fails, we'll just leave chrome
             // signed-out.
-            clearNewSignedInAccountName();
+            mPrefsManager.clearNewSignedInAccountName();
             performResignin(newName);
         }, false);
     }
@@ -232,39 +223,13 @@ public class SigninHelper {
         return false;
     }
 
-    /**
-     * Sets the ACCOUNTS_CHANGED_PREFS_KEY to true.
-     */
-    public static void markAccountsChangedPref() {
-        // The process may go away as soon as we return from onReceive but Android makes sure
-        // that in-flight disk writes from apply() complete before changing component states.
-        ContextUtils.getAppSharedPreferences()
-                .edit().putBoolean(ACCOUNTS_CHANGED_PREFS_KEY, true).apply();
-    }
-
-    /**
-     * @return The new account name of the current user. Null if it wasn't renamed.
-     */
-    public static String getNewSignedInAccountName() {
-        return (ContextUtils.getAppSharedPreferences()
-                .getString(ACCOUNT_RENAMED_PREFS_KEY, null));
-    }
-
-    private static void clearNewSignedInAccountName() {
-        ContextUtils.getAppSharedPreferences()
-                .edit()
-                .putString(ACCOUNT_RENAMED_PREFS_KEY, null)
-                .apply();
-    }
-
     private static String getLastKnownAccountName() {
         // This is the last known name of the currently signed in user.
         // It can be:
         //  1. The signed in account name known to the ChromeSigninController.
         //  2. A pending newly choosen name that is differed from the one known to
         //     ChromeSigninController but is stored in ACCOUNT_RENAMED_PREFS_KEY.
-        String name = ContextUtils.getAppSharedPreferences().getString(
-                ACCOUNT_RENAMED_PREFS_KEY, null);
+        String name = SigninPreferencesManager.getInstance().getNewSignedInAccountName();
 
         // If there is no pending rename, take the name known to ChromeSigninController.
         return name == null ? ChromeSigninController.get().getSignedInAccountName() : name;
@@ -283,10 +248,8 @@ public class SigninHelper {
 
         String newName = curName;
 
-        // This is the last read index of all the account change event.
-        int eventIndex = ContextUtils.getAppSharedPreferences().getInt(
-                ACCOUNT_RENAME_EVENT_INDEX_PREFS_KEY, 0);
-
+        SigninPreferencesManager prefsManager = SigninPreferencesManager.getInstance();
+        int eventIndex = prefsManager.getLastAccountChangedEventIndex();
         int newIndex = eventIndex;
 
         try {
@@ -318,34 +281,12 @@ public class SigninHelper {
         }
 
         if (!curName.equals(newName)) {
-            ContextUtils.getAppSharedPreferences()
-                    .edit().putString(ACCOUNT_RENAMED_PREFS_KEY, newName).apply();
+            prefsManager.setNewSignedInAccountName(newName);
         }
 
         if (newIndex != eventIndex) {
-            ContextUtils.getAppSharedPreferences()
-                    .edit().putInt(ACCOUNT_RENAME_EVENT_INDEX_PREFS_KEY, newIndex).apply();
+            prefsManager.setLastAccountChangedEventIndex(newIndex);
         }
     }
 
-    public static boolean checkAndClearAccountsChangedPref() {
-        if (ContextUtils.getAppSharedPreferences()
-                .getBoolean(ACCOUNTS_CHANGED_PREFS_KEY, false)) {
-            // Clear the value in prefs.
-            ContextUtils.getAppSharedPreferences()
-                    .edit().putBoolean(ACCOUNTS_CHANGED_PREFS_KEY, false).apply();
-            return true;
-        } else {
-            return false;
-        }
-    }
-
-    @VisibleForTesting
-    public static void resetSharedPrefs() {
-        SharedPreferences.Editor editor = ContextUtils.getAppSharedPreferences().edit();
-        editor.remove(ACCOUNT_RENAME_EVENT_INDEX_PREFS_KEY);
-        editor.remove(ACCOUNT_RENAMED_PREFS_KEY);
-        editor.remove(ACCOUNTS_CHANGED_PREFS_KEY);
-        editor.apply();
-    }
 }

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninPreferencesManager.java

@@ -14,10 +14,10 @@ import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
 import java.util.Set;
 
 /**
- * SigninPreferencesManager stores the state of the SignInPromo.
+ * SigninPreferencesManager stores the state of SharedPreferences related to account sign-in.
  */
 public class SigninPreferencesManager {
-    static final SigninPreferencesManager INSTANCE = new SigninPreferencesManager();
+    private static final SigninPreferencesManager INSTANCE = new SigninPreferencesManager();
 
     private final SharedPreferencesManager mManager;
 
@@ -32,6 +32,80 @@ public class SigninPreferencesManager {
         return INSTANCE;
     }
 
+    /**
+     * Sets the {@link ChromePreferenceKeys#SIGNIN_ACCOUNTS_CHANGED} to true.
+     */
+    public void markAccountsChangedPref() {
+        // The process may go away as soon as we return from onReceive but Android makes sure
+        // that in-flight disk writes from apply() complete before changing component states.
+        mManager.writeBoolean(ChromePreferenceKeys.SIGNIN_ACCOUNTS_CHANGED, true);
+    }
+
+    /**
+     * @return The new account name of the current user. Null if it wasn't renamed.
+     */
+    String getNewSignedInAccountName() {
+        return mManager.readString(ChromePreferenceKeys.SIGNIN_ACCOUNT_RENAMED, null);
+    }
+
+    /**
+     * Sets the new account name of the current user.
+     *
+     * @param newName the new name to write
+     */
+    void setNewSignedInAccountName(@Nullable String newName) {
+        mManager.writeString(ChromePreferenceKeys.SIGNIN_ACCOUNT_RENAMED, newName);
+    }
+
+    /**
+     * Clears the new account name of the current user.
+     */
+    void clearNewSignedInAccountName() {
+        setNewSignedInAccountName(null);
+    }
+
+    /**
+     * Sets the last read index of all the account changed events of the current signed in account.
+     *
+     * @param newIndex the new index to write
+     */
+    void setLastAccountChangedEventIndex(int newIndex) {
+        mManager.writeInt(ChromePreferenceKeys.SIGNIN_ACCOUNT_RENAME_EVENT_INDEX, newIndex);
+    }
+
+    /**
+     * @return the last read index of all the account changed events of the current signed in
+     *         account.
+     */
+    int getLastAccountChangedEventIndex() {
+        return mManager.readInt(ChromePreferenceKeys.SIGNIN_ACCOUNT_RENAME_EVENT_INDEX);
+    }
+
+    /**
+     * Gets the state of {@link ChromePreferenceKeys#SIGNIN_ACCOUNTS_CHANGED} and clears it.
+     *
+     * @return the state of {@link ChromePreferenceKeys#SIGNIN_ACCOUNTS_CHANGED} before the call.
+     */
+    public boolean checkAndClearAccountsChangedPref() {
+        if (mManager.readBoolean(ChromePreferenceKeys.SIGNIN_ACCOUNTS_CHANGED, false)) {
+            // Clear the value in prefs.
+            mManager.writeBoolean(ChromePreferenceKeys.SIGNIN_ACCOUNTS_CHANGED, false);
+            return true;
+        } else {
+            return false;
+        }
+    }
+
+    /**
+     * Clears the accounts state-related shared prefs.
+     */
+    @VisibleForTesting
+    public void clearAccountsStateSharedPrefsForTesting() {
+        mManager.removeKey(ChromePreferenceKeys.SIGNIN_ACCOUNT_RENAME_EVENT_INDEX);
+        mManager.removeKey(ChromePreferenceKeys.SIGNIN_ACCOUNT_RENAMED);
+        mManager.removeKey(ChromePreferenceKeys.SIGNIN_ACCOUNTS_CHANGED);
+    }
+
     /**
      * Returns Chrome major version number when signin promo was last shown, or 0 if version number
      * isn't known.

chrome/android/javatests/src/org/chromium/chrome/browser/signin/IdentityManagerIntegrationTest.java

@@ -88,7 +88,6 @@ public class IdentityManagerIntegrationTest {
     public void tearDown() {
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { mIdentityMutator.reloadAllAccountsFromSystemWithPrimaryAccount(null); });
-        SigninHelper.resetSharedPrefs();
         SigninTestUtil.resetSigninState();
     }
 

chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninHelperTest.java

@@ -42,36 +42,6 @@ public class SigninHelperTest {
         AccountManagerFacade.resetAccountManagerFacadeForTests();
     }
 
-    @Test
-    @SmallTest
-    @RetryOnFailure
-    public void testAccountsChangedPref() {
-        Assert.assertEquals("Should never return true before the pref has ever been set.", false,
-                SigninHelper.checkAndClearAccountsChangedPref());
-        Assert.assertEquals("Should never return true before the pref has ever been set.", false,
-                SigninHelper.checkAndClearAccountsChangedPref());
-
-        // Mark the pref as set.
-        SigninHelper.markAccountsChangedPref();
-
-        Assert.assertEquals("Should return true first time after marking accounts changed", true,
-                SigninHelper.checkAndClearAccountsChangedPref());
-        Assert.assertEquals("Should only return true first time after marking accounts changed",
-                false, SigninHelper.checkAndClearAccountsChangedPref());
-        Assert.assertEquals("Should only return true first time after marking accounts changed",
-                false, SigninHelper.checkAndClearAccountsChangedPref());
-
-        // Mark the pref as set again.
-        SigninHelper.markAccountsChangedPref();
-
-        Assert.assertEquals("Should return true first time after marking accounts changed", true,
-                SigninHelper.checkAndClearAccountsChangedPref());
-        Assert.assertEquals("Should only return true first time after marking accounts changed",
-                false, SigninHelper.checkAndClearAccountsChangedPref());
-        Assert.assertEquals("Should only return true first time after marking accounts changed",
-                false, SigninHelper.checkAndClearAccountsChangedPref());
-    }
-
     @Test
     @SmallTest
     @RetryOnFailure
@@ -151,6 +121,6 @@ public class SigninHelperTest {
     }
 
     private String getNewSignedInAccountName() {
-        return SigninHelper.getNewSignedInAccountName();
+        return SigninPreferencesManager.getInstance().getNewSignedInAccountName();
     }
 }

chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninPreferencesManagerTest.java

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.signin;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import android.support.test.filters.SmallTest;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.base.test.util.RetryOnFailure;
+
+/**
+ * Unit tests for {@link SigninPreferencesManager}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+public class SigninPreferencesManagerTest {
+    @Test
+    @SmallTest
+    @RetryOnFailure
+    public void testAccountsChangedPref() {
+        SigninPreferencesManager prefsManager = SigninPreferencesManager.getInstance();
+        assertFalse("Should never return true before the pref has ever been set.",
+                prefsManager.checkAndClearAccountsChangedPref());
+        assertFalse("Should never return true before the pref has ever been set.",
+                prefsManager.checkAndClearAccountsChangedPref());
+
+        // Mark the pref as set.
+        prefsManager.markAccountsChangedPref();
+
+        assertTrue("Should return true first time after marking accounts changed",
+                prefsManager.checkAndClearAccountsChangedPref());
+        assertFalse("Should only return true first time after marking accounts changed",
+                prefsManager.checkAndClearAccountsChangedPref());
+        assertFalse("Should only return true first time after marking accounts changed",
+                prefsManager.checkAndClearAccountsChangedPref());
+
+        // Mark the pref as set again.
+        prefsManager.markAccountsChangedPref();
+
+        assertTrue("Should return true first time after marking accounts changed",
+                prefsManager.checkAndClearAccountsChangedPref());
+        assertFalse("Should only return true first time after marking accounts changed",
+                prefsManager.checkAndClearAccountsChangedPref());
+        assertFalse("Should only return true first time after marking accounts changed",
+                prefsManager.checkAndClearAccountsChangedPref());
+    }
+}

chrome/browser/preferences/android/java/src/org/chromium/chrome/browser/preferences/ChromePreferenceKeys.java

@@ -494,6 +494,19 @@ public final class ChromePreferenceKeys {
     public static final String SHARING_LAST_SHARED_CLASS_NAME = "last_shared_class_name";
     public static final String SHARING_LAST_SHARED_PACKAGE_NAME = "last_shared_package_name";
 
+    public static final String SIGNIN_ACCOUNTS_CHANGED = "prefs_sync_accounts_changed";
+
+    /**
+     * Holds the new account's name if the currently signed in account has been renamed.
+     */
+    public static final String SIGNIN_ACCOUNT_RENAMED = "prefs_sync_account_renamed";
+
+    /**
+     * Holds the last read index of all the account changed events of the current signed in account.
+     */
+    public static final String SIGNIN_ACCOUNT_RENAME_EVENT_INDEX =
+            "prefs_sync_account_rename_event_index";
+
     /**
      * Generic signin and sync promo preferences.
      */
@@ -803,6 +816,9 @@ public final class ChromePreferenceKeys {
                 SEARCH_ENGINE_CHOICE_REQUESTED_TIMESTAMP,
                 SHARING_LAST_SHARED_CLASS_NAME,
                 SHARING_LAST_SHARED_PACKAGE_NAME,
+                SIGNIN_ACCOUNTS_CHANGED,
+                SIGNIN_ACCOUNT_RENAMED,
+                SIGNIN_ACCOUNT_RENAME_EVENT_INDEX,
                 SIGNIN_AND_SYNC_PROMO_SHOW_COUNT,
                 SIGNIN_PROMO_IMPRESSIONS_COUNT_BOOKMARKS,
                 SIGNIN_PROMO_IMPRESSIONS_COUNT_SETTINGS,

chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/signin/SigninTestUtil.java

@@ -10,7 +10,7 @@ import android.annotation.SuppressLint;
 import androidx.annotation.WorkerThread;
 
 import org.chromium.chrome.browser.signin.IdentityServicesProvider;
-import org.chromium.chrome.browser.signin.SigninHelper;
+import org.chromium.chrome.browser.signin.SigninPreferencesManager;
 import org.chromium.components.signin.AccountIdProvider;
 import org.chromium.components.signin.AccountManagerFacade;
 import org.chromium.components.signin.ChromeSigninController;
@@ -46,7 +46,6 @@ public final class SigninTestUtil {
         AccountManagerFacade.overrideAccountManagerFacadeForTests(sAccountManager);
         overrideAccountIdProvider();
         resetSigninState();
-        SigninHelper.resetSharedPrefs();
     }
 
     /**
@@ -59,7 +58,6 @@ public final class SigninTestUtil {
         }
         sAddedAccounts.clear();
         resetSigninState();
-        SigninHelper.resetSharedPrefs();
     }
 
     /**
@@ -136,6 +134,8 @@ public final class SigninTestUtil {
     public static void resetSigninState() {
         // Clear cached signed account name and accounts list.
         ChromeSigninController.get().setSignedInAccountName(null);
+
+        SigninPreferencesManager.getInstance().clearAccountsStateSharedPrefsForTesting();
     }
 
     private SigninTestUtil() {}