Don't toggle sync on/off on account rename.

More detais in: crbug.com/524675.

What I have discovered is that AccountManager keeps the whole syncable and sync-enabled setting across rename events. We can leverage this by making this the source of truth for sync setting.

That way, no matter when then actual sign-in occurs, it can simply check the master setting and decide if sync should be on or off without racing against sync service or the setting's cache.

I tested this and it works 5/5 times. I don't know how good this solution is but at least the success rate is higher the current implementation of check right before signout.

BUG=524675

Review URL: https://codereview.chromium.org/1454563002

Cr-Commit-Position: refs/heads/master@{#366225}
(cherry picked from commit ab28048)

Review URL: https://codereview.chromium.org/1537113003 .

Cr-Commit-Position: refs/branch-heads/2564@{#401}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}

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

@@ -236,12 +236,7 @@ private void handleAccountRename(final String oldName, final String newName) {
         // TODO(acleung): I think most of the operations need to run on the main
         // thread. May be we should have a progress Dialog?
 
-        // Before signing out, remember the current sync state and data types.
-        final boolean isSyncWanted = AndroidSyncSettings.isChromeSyncEnabled(mContext);
-        final Set<Integer> dataTypes = mProfileSyncService.getPreferredDataTypes();
-
         // TODO(acleung): Deal with passphrase or just prompt user to re-enter it?
-
         // Perform a sign-out with a callback to sign-in again.
         mSigninManager.signOut(null, new Runnable() {
             @Override
@@ -251,28 +246,21 @@ public void run() {
                 // Otherwise, if re-sign-in fails, we'll just leave chrome
                 // signed-out.
                 clearNewSignedInAccountName(mContext);
-                performResignin(newName, isSyncWanted, dataTypes);
+                performResignin(newName);
             }
         });
     }
 
-    private void performResignin(String newName,
-                                 final boolean isSyncWanted,
-                                 final Set<Integer> dataTypes) {
+    private void performResignin(String newName) {
         // This is the correct account now.
         final Account account = AccountManagerHelper.createAccountFromName(newName);
 
         mSigninManager.startSignIn(null, account, true, new SignInFlowObserver() {
             @Override
             public void onSigninComplete() {
-                mProfileSyncService.setSetupInProgress(false);
-
-                if (isSyncWanted) {
-                    mSyncController.start();
-                } else {
-                    mSyncController.stop();
+                if (mProfileSyncService != null) {
+                    mProfileSyncService.setSetupInProgress(false);
                 }
-
                 validateAccountSettings(true);
             }
 

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

@@ -5,21 +5,17 @@
 package org.chromium.chrome.browser.signin;
 
 import android.accounts.Account;
-import android.content.Context;
 import android.test.InstrumentationTestCase;
 import android.test.suitebuilder.annotation.SmallTest;
 
 import org.chromium.base.test.util.AdvancedMockContext;
+import org.chromium.base.test.util.DisabledTest;
+import org.chromium.chrome.test.util.browser.signin.MockChangeEventChecker;
 import org.chromium.sync.signin.AccountManagerHelper;
 import org.chromium.sync.signin.ChromeSigninController;
 import org.chromium.sync.test.util.AccountHolder;
 import org.chromium.sync.test.util.MockAccountManager;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
 /**
  * Instrumentation tests for {@link SigninHelper}.
  */
@@ -141,31 +137,4 @@ private String getSignedInAccountName() {
     private String getNewSignedInAccountName() {
         return SigninHelper.getNewSignedInAccountName(mContext);
     }
-
-    private static final class MockChangeEventChecker
-            implements SigninHelper.AccountChangeEventChecker {
-        private Map<String, List<String>> mEvents =
-                new HashMap<String, List<String>>();
-
-        @Override
-        public List<String> getAccountChangeEvents(
-                Context context, int index, String accountName) {
-            List<String> eventsList = getEventList(accountName);
-            return eventsList.subList(index, eventsList.size());
-        }
-
-        private void insertRenameEvent(String from, String to) {
-            List<String> eventsList = getEventList(from);
-            eventsList.add(to);
-        }
-
-        private List<String> getEventList(String account) {
-            List<String> eventsList = mEvents.get(account);
-            if (eventsList == null) {
-                eventsList = new ArrayList<String>();
-                mEvents.put(account, eventsList);
-            }
-            return eventsList;
-        }
-    }
 }

chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java

@@ -11,14 +11,21 @@
 
 import org.chromium.base.ActivityState;
 import org.chromium.base.ApplicationStatus;
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.ChromeActivity;
+import org.chromium.chrome.browser.signin.AccountIdProvider;
+import org.chromium.chrome.browser.signin.AccountTrackerService;
+import org.chromium.chrome.browser.signin.SigninHelper;
+import org.chromium.chrome.test.util.browser.signin.MockChangeEventChecker;
+import org.chromium.chrome.test.util.browser.signin.SigninTestUtil;
 import org.chromium.chrome.test.util.browser.sync.SyncTestUtil;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.content.browser.test.util.Criteria;
 import org.chromium.content.browser.test.util.CriteriaHelper;
 import org.chromium.content.browser.test.util.JavaScriptUtils;
 import org.chromium.sync.AndroidSyncSettings;
+import org.chromium.sync.signin.ChromeSigninController;
 
 import java.util.concurrent.TimeoutException;
 
@@ -122,6 +129,51 @@ public void testSignInAndOut() throws InterruptedException {
         SyncTestUtil.verifySyncIsActiveForAccount(mContext, account);
     }
 
+    @LargeTest
+    @Feature({"Sync"})
+    public void testRename() throws InterruptedException {
+        // The two accounts object that would represent the account rename.
+        final Account oldAccount = setUpTestAccountAndSignInToSync();
+        final Account newAccount = SigninTestUtil.get().addTestAccount("[email protected]");
+
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                // First, we force a call to updateAccountRenameData. In the real world,
+                // this should be called by one of our broadcast listener that listens to
+                // real account rename events instead of the mocks.
+                MockChangeEventChecker eventChecker = new MockChangeEventChecker();
+                eventChecker.insertRenameEvent(oldAccount.name, newAccount.name);
+                SigninHelper.updateAccountRenameData(mContext, eventChecker);
+
+                // Tell the fake content resolver that a rename had happen and copy over the sync
+                // settings. This would normally be done by the SystemSyncContentResolver.
+                mSyncContentResolver.renameAccounts(
+                        oldAccount, newAccount, AndroidSyncSettings.getContractAuthority(mContext));
+
+                // Inform the AccountTracker, these would normally be done by account validation
+                // or signin. We will only be calling the testing versions of it.
+                AccountIdProvider provider = AccountIdProvider.getInstance();
+                String[] accountNames = {newAccount.name};
+                String[] accountIds = {provider.getAccountId(mContext, accountNames[0])};
+                AccountTrackerService.get(mContext).syncForceRefreshForTest(
+                        accountIds, accountNames);
+
+                // Starts the rename process. Normally, this is triggered by the broadcast
+                // listener as well.
+                SigninHelper.get(mContext).validateAccountSettings(true);
+            }
+        });
+
+        CriteriaHelper.pollForCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return newAccount.equals(ChromeSigninController.get(mContext).getSignedInUser());
+            }
+        });
+        SyncTestUtil.verifySyncIsActiveForAccount(mContext, newAccount);
+    }
+
     @LargeTest
     @Feature({"Sync"})
     public void testStopAndStartSync() throws InterruptedException {

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

@@ -0,0 +1,44 @@
+// Copyright 2015 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.test.util.browser.signin;
+
+import android.content.Context;
+
+import org.chromium.chrome.browser.signin.SigninHelper;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Fake AccountChangeEventChecker for testing.
+ */
+public final class MockChangeEventChecker
+        implements SigninHelper.AccountChangeEventChecker {
+    private Map<String, List<String>> mEvents =
+            new HashMap<String, List<String>>();
+
+    @Override
+    public List<String> getAccountChangeEvents(
+            Context context, int index, String accountName) {
+        List<String> eventsList = getEventList(accountName);
+        return eventsList.subList(index, eventsList.size());
+    }
+
+    public void insertRenameEvent(String from, String to) {
+        List<String> eventsList = getEventList(from);
+        eventsList.add(to);
+    }
+
+    private List<String> getEventList(String account) {
+        List<String> eventsList = mEvents.get(account);
+        if (eventsList == null) {
+            eventsList = new ArrayList<String>();
+            mEvents.put(account, eventsList);
+        }
+        return eventsList;
+    }
+}

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

@@ -64,7 +64,14 @@ private SigninTestUtil(Instrumentation instrumentation) {
      * Add an account with the default name.
      */
     public Account addTestAccount() {
-        Account account = createTestAccount(DEFAULT_ACCOUNT);
+        return addTestAccount(DEFAULT_ACCOUNT);
+    }
+
+    /**
+     * Add an account with a given name.
+     */
+    public Account addTestAccount(String name) {
+        Account account = createTestAccount(name);
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {

sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java

@@ -123,6 +123,7 @@ public static boolean isSyncEnabled(Context context) {
      *
      * @return true if sync is on, false otherwise
      */
+    @VisibleForTesting
     public static boolean isChromeSyncEnabled(Context context) {
         ensureInitialized(context);
         return sInstance.mChromeSyncEnabled;

sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java

@@ -182,6 +182,18 @@ public void disableObserverNotifications() {
         mDisableObserverNotifications = true;
     }
 
+   /**
+     * Simulate an account rename, which copies settings to the new account.
+     */
+    public void renameAccounts(Account oldAccount, Account newAccount, String authority) {
+        int oldIsSyncable = getIsSyncable(oldAccount, authority);
+        setIsSyncable(newAccount, authority, oldIsSyncable);
+        if (oldIsSyncable == 1) {
+            setSyncAutomatically(
+                    newAccount, authority, getSyncAutomatically(oldAccount, authority));
+        }
+    }
+
     private static class AsyncSyncStatusObserver {
 
         private final SyncStatusObserver mSyncStatusObserver;