[Signin][Android] Remove Account from CoreAccountInfo

Reconcile CoreAccountInfo with its C++ counterpart by removing
Android-specific Account object and using email as a string instead.
This improves consistency and simplifies creating CoreAccountInfo in
tests. CoreAccountInfo.getName() will be renamed to getEmail() in the
subsequent CL.

Bug: 1042197, 1046412
Change-Id: I99d721771841ca361df63c17f1d77be8cb450abf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2027531
Auto-Submit: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Tanmoy Mollik <triploblastic@chromium.org>
Reviewed-by: Tanmoy Mollik <triploblastic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736906}

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

@@ -22,6 +22,7 @@ import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.base.task.PostTask;
 import org.chromium.chrome.browser.externalauth.ExternalAuthUtils;
+import org.chromium.components.signin.AccountManagerFacade;
 import org.chromium.components.signin.AccountTrackerService;
 import org.chromium.components.signin.ChromeSigninController;
 import org.chromium.components.signin.base.CoreAccountInfo;
@@ -363,23 +364,44 @@ public class SigninManager
     /**
      * Starts the sign-in flow, and executes the callback when finished.
      *
-     * If an activity is provided, it is considered an "interactive" sign-in and the user can be
-     * prompted to confirm various aspects of sign-in using dialogs inside the activity.
      * The sign-in flow goes through the following steps:
      *
      *   - Wait for AccountTrackerService to be seeded.
-     *   - If interactive, confirm the account change with the user.
      *   - Wait for policy to be checked for the account.
-     *   - If interactive and the account is managed, warn the user.
      *   - If managed, wait for the policy to be fetched.
-     *   - Complete sign-in with the native SigninManager and kick off token requests.
+     *   - Complete sign-in with the native IdentityManager.
+     *   - Call the callback if provided.
+     *
+     * @param accessPoint {@link SigninAccessPoint} that initiated the sign-in flow.
+     * @param accountInfo The account to sign in to.
+     * @param callback Optional callback for when the sign-in process is finished.
+     */
+    public void signIn(@SigninAccessPoint int accessPoint, CoreAccountInfo accountInfo,
+            @Nullable SignInCallback callback) {
+        assert accountInfo != null;
+        signIn(accessPoint, AccountManagerFacade.createAccountFromName(accountInfo.getName()),
+                callback);
+    }
+
+    /**
+     * @deprecated use {@link #signIn(int, CoreAccountInfo, SignInCallback)} instead.
+     * TODO(crbug.com/1002056): Remove this version after migrating all callers to CoreAccountInfo.
+     *
+     * Starts the sign-in flow, and executes the callback when finished.
+     *
+     * The sign-in flow goes through the following steps:
+     *
+     *   - Wait for AccountTrackerService to be seeded.
+     *   - Wait for policy to be checked for the account.
+     *   - If managed, wait for the policy to be fetched.
+     *   - Complete sign-in with the native IdentityManager.
      *   - Call the callback if provided.
      *
      * @param accessPoint {@link SigninAccessPoint} that initiated the sign-in flow.
      * @param account The account to sign in to.
      * @param callback Optional callback for when the sign-in process is finished.
      */
-    // TODO(crbug.com/1002056) SigninManager.Signin should use CoreAccountInfo as a parameter.
+    @Deprecated
     public void signIn(@SigninAccessPoint int accessPoint, Account account,
             @Nullable SignInCallback callback) {
         assert isSignInAllowed() : "Sign-in isn't allowed!";
@@ -459,7 +481,7 @@ public class SigninManager
         // sync tries to start without being signed in natively and crashes.
         ChromeSigninController.get().setSignedInAccountName(
                 mSignInState.mCoreAccountInfo.getName());
-        enableSync(mSignInState.mCoreAccountInfo.getAccount());
+        enableSync(mSignInState.mCoreAccountInfo);
 
         if (mSignInState.mCallback != null) {
             mSignInState.mCallback.onSignInComplete();
@@ -681,10 +703,11 @@ public class SigninManager
         SigninManagerJni.get().stopApplyingCloudPolicy(mNativeSigninManagerAndroid);
     }
 
-    private void enableSync(Account account) {
+    private void enableSync(CoreAccountInfo accountInfo) {
         // Cache the signed-in account name. This must be done after the native call, otherwise
         // sync tries to start without being signed in the native code and crashes.
-        mAndroidSyncSettings.updateAccount(account);
+        mAndroidSyncSettings.updateAccount(
+                AccountManagerFacade.createAccountFromName(accountInfo.getName()));
         mAndroidSyncSettings.enableChromeSync();
     }
 

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

@@ -4,7 +4,6 @@
 
 package org.chromium.chrome.browser.signin;
 
-import android.accounts.Account;
 import android.support.test.filters.MediumTest;
 
 import org.junit.After;
@@ -45,14 +44,16 @@ public class IdentityManagerIntegrationTest {
     @Rule
     public AccountManagerTestRule mAccountManagerTestRule = new AccountManagerTestRule();
 
-    private static final Account TEST_ACCOUNT1 =
-            AccountManagerFacade.createAccountFromName("foo@gmail.com");
-    private static final Account TEST_ACCOUNT2 =
-            AccountManagerFacade.createAccountFromName("bar@gmail.com");
+    private static final String TEST_ACCOUNT1 = "foo@gmail.com";
+    private static final String TEST_ACCOUNT2 = "bar@gmail.com";
     private static final AccountHolder TEST_ACCOUNT_HOLDER_1 =
-            AccountHolder.builder(TEST_ACCOUNT1).alwaysAccept(true).build();
+            AccountHolder.builder(AccountManagerFacade.createAccountFromName(TEST_ACCOUNT1))
+                    .alwaysAccept(true)
+                    .build();
     private static final AccountHolder TEST_ACCOUNT_HOLDER_2 =
-            AccountHolder.builder(TEST_ACCOUNT2).alwaysAccept(true).build();
+            AccountHolder.builder(AccountManagerFacade.createAccountFromName(TEST_ACCOUNT2))
+                    .alwaysAccept(true)
+                    .build();
 
     private CoreAccountInfo mTestAccount1;
     private CoreAccountInfo mTestAccount2;
@@ -110,10 +111,10 @@ public class IdentityManagerIntegrationTest {
     private void initializeTestAccounts() {
         AccountIdProvider provider = AccountIdProvider.getInstance();
 
-        String account1Id = provider.getAccountId(TEST_ACCOUNT1.name);
+        String account1Id = provider.getAccountId(TEST_ACCOUNT1);
         mTestAccount1 =
                 new CoreAccountInfo(new CoreAccountId(account1Id), TEST_ACCOUNT1, account1Id);
-        String account2Id = provider.getAccountId(TEST_ACCOUNT2.name);
+        String account2Id = provider.getAccountId(TEST_ACCOUNT2);
         mTestAccount2 =
                 new CoreAccountInfo(new CoreAccountId(account2Id), TEST_ACCOUNT2, account2Id);
     }

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

@@ -106,8 +106,8 @@ public class SigninSignoutIntegrationTest {
         verify(mSignInStateObserverMock, never()).onSignedOut();
         assertSignedIn();
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            Assert.assertEquals(account,
-                    mSigninManager.getIdentityManager().getPrimaryAccountInfo().getAccount());
+            Assert.assertEquals(account.name,
+                    mSigninManager.getIdentityManager().getPrimaryAccountInfo().getName());
         });
     }
 

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

@@ -20,8 +20,6 @@ import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.MockitoAnnotations.initMocks;
 
-import android.accounts.Account;
-
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -33,7 +31,6 @@ import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.base.test.DisableNativeTestRule;
 import org.chromium.base.test.util.JniMocker;
 import org.chromium.chrome.browser.externalauth.ExternalAuthUtils;
-import org.chromium.components.signin.AccountManagerFacade;
 import org.chromium.components.signin.AccountTrackerService;
 import org.chromium.components.signin.base.CoreAccountId;
 import org.chromium.components.signin.base.CoreAccountInfo;
@@ -90,8 +87,8 @@ public class SigninManagerTest {
                 new SigninManager(0 /* nativeSigninManagerAndroid */, mAccountTrackerService,
                         mIdentityManager, mIdentityMutator, androidSyncSettings, externalAuthUtils);
 
-        mAccount = new CoreAccountInfo(new CoreAccountId("gaia-id-user"),
-                AccountManagerFacade.createAccountFromName("user@domain.com"), "gaia-id-user");
+        mAccount = new CoreAccountInfo(
+                new CoreAccountId("gaia-id-user"), "user@domain.com", "gaia-id-user");
     }
 
     @Test
@@ -218,9 +215,8 @@ public class SigninManagerTest {
 
     @Test
     public void callbackNotifiedOnSignin() {
-        CoreAccountInfo account = new CoreAccountInfo(new CoreAccountId("test_at_gmail.com"),
-                new Account("test@gmail.com", AccountManagerFacade.GOOGLE_ACCOUNT_TYPE),
-                "test_at_gmail.com");
+        CoreAccountInfo account = new CoreAccountInfo(
+                new CoreAccountId("test_at_gmail.com"), "test@gmail.com", "test_at_gmail.com");
 
         // No need to seed accounts to the native code.
         doReturn(true).when(mAccountTrackerService).checkAndSeedSystemAccounts();
@@ -238,7 +234,7 @@ public class SigninManagerTest {
 
         mSigninManager.onFirstRunCheckDone(); // Allow sign-in.
 
-        mSigninManager.signIn(SigninAccessPoint.UNKNOWN, account.getAccount(), null);
+        mSigninManager.signIn(SigninAccessPoint.UNKNOWN, account, null);
         assertTrue(mSigninManager.isOperationInProgress());
         AtomicInteger callCount = new AtomicInteger(0);
         mSigninManager.runAfterOperationInProgress(callCount::incrementAndGet);
@@ -251,9 +247,8 @@ public class SigninManagerTest {
 
     @Test(expected = AssertionError.class)
     public void failIfAlreadySignedin() {
-        CoreAccountInfo account = new CoreAccountInfo(new CoreAccountId("test_at_gmail.com"),
-                new Account("test@gmail.com", AccountManagerFacade.GOOGLE_ACCOUNT_TYPE),
-                "test_at_gmail.com");
+        CoreAccountInfo account = new CoreAccountInfo(
+                new CoreAccountId("test_at_gmail.com"), "test@gmail.com", "test_at_gmail.com");
 
         // No need to seed accounts to the native code.
         doReturn(true).when(mAccountTrackerService).checkAndSeedSystemAccounts();
@@ -268,7 +263,7 @@ public class SigninManagerTest {
 
         mSigninManager.onFirstRunCheckDone(); // Allow sign-in.
 
-        mSigninManager.signIn(SigninAccessPoint.UNKNOWN, account.getAccount(), null);
+        mSigninManager.signIn(SigninAccessPoint.UNKNOWN, account, null);
         assertTrue(mSigninManager.isOperationInProgress());
 
         // The following should throw an assertion error

components/signin/public/android/java/src/org/chromium/components/signin/base/CoreAccountInfo.java

@@ -4,59 +4,37 @@
 
 package org.chromium.components.signin.base;
 
-import android.accounts.Account;
-
 import androidx.annotation.NonNull;
 
 import org.chromium.base.annotations.CalledByNative;
-import org.chromium.components.signin.AccountManagerFacade;
 
 /**
  * Structure storing the core information about a Google account that is always known. The {@link
  * CoreAccountInfo} for a given user is almost always the same but it may change in some rare cases.
- * For example, the {@link android.accounts.Account} will change if a user changes email.
- *
- * This class has a native counterpart called CoreAccountInfo. There are several differences between
- * these two classes:
- * - Android class additionally exposes {@link android.accounts.Account} object for interactions
- * with the system.
- * - Android class has the "account name" whereas the native class has "email". This is the same
- * string, only the naming in different.
+ * For example, the email will change if the user changes email.
+ * This class has a native counterpart called CoreAccountInfo.
  */
 public class CoreAccountInfo {
     private final CoreAccountId mId;
-    private final Account mAccount;
+    private final String mEmail;
     private final String mGaiaId;
 
     /**
      * Constructs a CoreAccountInfo with the provided parameters
-     * @param id A CoreAccountId associated with the account, equal to either account.name or
-     *         gaiaId.
-     * @param account Android account.
+     * @param id A CoreAccountId associated with the account, equal to either email or gaiaId.
+     * @param email The email of the account.
      * @param gaiaId String representation of the Gaia ID. Must not be an email address.
      */
-    public CoreAccountInfo(
-            @NonNull CoreAccountId id, @NonNull Account account, @NonNull String gaiaId) {
-        assert id != null;
-        assert account != null;
-        assert gaiaId != null;
-        assert !gaiaId.contains("@");
-
-        mId = id;
-        mAccount = account;
-        mGaiaId = gaiaId;
-    }
-
     @CalledByNative
-    private CoreAccountInfo(
-            @NonNull CoreAccountId id, @NonNull String name, @NonNull String gaiaId) {
+    public CoreAccountInfo(
+            @NonNull CoreAccountId id, @NonNull String email, @NonNull String gaiaId) {
         assert id != null;
-        assert name != null;
+        assert email != null;
         assert gaiaId != null;
         assert !gaiaId.contains("@");
 
         mId = id;
-        mAccount = AccountManagerFacade.createAccountFromName(name);
+        mEmail = email;
         mGaiaId = gaiaId;
     }
 
@@ -69,11 +47,11 @@ public class CoreAccountInfo {
     }
 
     /**
-     * Returns a name of the current account.
+     * Returns the email of the current account.
      */
     @CalledByNative
     public String getName() {
-        return mAccount.name;
+        return mEmail;
     }
 
     /**
@@ -84,13 +62,6 @@ public class CoreAccountInfo {
         return mGaiaId;
     }
 
-    /**
-     * Returns {@link android.accounts.Account} object holding a name of the current account.
-     */
-    public Account getAccount() {
-        return mAccount;
-    }
-
     @Override
     public String toString() {
         return String.format("CoreAccountInfo{id[%s], name[%s]}", getId(), getName());
@@ -98,13 +69,15 @@ public class CoreAccountInfo {
 
     @Override
     public int hashCode() {
-        return 31 * mId.hashCode() + mAccount.hashCode();
+        int result = 31 * mId.hashCode() + mEmail.hashCode();
+        return 31 * result + mGaiaId.hashCode();
     }
 
     @Override
     public boolean equals(Object obj) {
         if (!(obj instanceof CoreAccountInfo)) return false;
         CoreAccountInfo other = (CoreAccountInfo) obj;
-        return mId.equals(other.mId) && mAccount.equals(other.mAccount);
+        return mId.equals(other.mId) && mEmail.equals(other.mEmail)
+                && mGaiaId.equals(other.mGaiaId);
     }
 }