From fd45731d49e2274221be3df2bd2fb3c8b0bed613 Mon Sep 17 00:00:00 2001 From: maxbogue Date: Tue, 21 Apr 2015 14:22:08 -0700 Subject: [Sync] Ensure isSyncable is set when signed in. This is a more extensive version of http://crrev.com/1075343003 for landing on trunk. Previously, if isSyncable is false, but sync is enabled for Chrome, even if you sign in, Chrome would never set syncable to true. This CL ensures that syncable is always set to true if there is an account signed in, regardless of whether chrome sync is enabled or not. Additionally, it sets syncable to false when no account is signed in, to ensure that the user does not see a non-functional switch in the Android settings. The new AccountManager setup in the test is necessary so that the test accounts can be cleaned up by the loop in updateSyncability(). BUG=475299 TEST=Regression tests added, plus: Ensure that $ adb shell dumpsys content | grep chrome should show syncable=0 and enabled=true Sign in to Chrome. Now dumpsys should say syncable=1 and enabled=true. Review URL: https://codereview.chromium.org/1062973004 Cr-Commit-Position: refs/heads/master@{#326122} --- .../src/org/chromium/sync/AndroidSyncSettings.java | 17 ++++--- .../org/chromium/sync/AndroidSyncSettingsTest.java | 58 +++++++++++++++++++++- .../test/util/MockSyncContentResolverDelegate.java | 6 +-- 3 files changed, 68 insertions(+), 13 deletions(-) (limited to 'sync') diff --git a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java index d24366c..3587e76 100644 --- a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java +++ b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java @@ -160,6 +160,7 @@ public class AndroidSyncSettings { public void updateAccount(Account account) { synchronized (mLock) { mAccount = account; + updateSyncability(); } if (updateCachedSettings()) { notifyObservers(); @@ -193,8 +194,8 @@ public class AndroidSyncSettings { private void setChromeSyncEnabled(boolean value) { synchronized (mLock) { + updateSyncability(); if (value == mChromeSyncEnabled || mAccount == null) return; - ensureSyncable(); mChromeSyncEnabled = value; StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); @@ -205,18 +206,22 @@ public class AndroidSyncSettings { } /** - * Ensure Chrome is registered with the Android Sync Manager. + * Ensure Chrome is registered with the Android Sync Manager iff signed in. * * This is what causes the "Chrome" option to appear in Settings -> Accounts -> Sync . * This function must be called within a synchronized block. */ - private void ensureSyncable() { - if (mIsSyncable || mAccount == null) return; + private void updateSyncability() { + boolean shouldBeSyncable = mAccount != null; + if (mIsSyncable == shouldBeSyncable) return; - mIsSyncable = true; + mIsSyncable = shouldBeSyncable; StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); - mSyncContentResolverDelegate.setIsSyncable(mAccount, mContractAuthority, 1); + // Make account syncable if there is one. + if (shouldBeSyncable) { + mSyncContentResolverDelegate.setIsSyncable(mAccount, mContractAuthority, 1); + } // Disable the syncability of Chrome for all other accounts. Don't use // our cache as we're touching many accounts that aren't signed in, so this saves diff --git a/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java b/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java index aeba67f..13c43f6 100644 --- a/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java +++ b/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java @@ -12,6 +12,9 @@ import android.test.suitebuilder.annotation.SmallTest; import org.chromium.base.ThreadUtils; import org.chromium.base.test.util.Feature; import org.chromium.sync.AndroidSyncSettings.AndroidSyncSettingsObserver; +import org.chromium.sync.signin.AccountManagerHelper; +import org.chromium.sync.test.util.AccountHolder; +import org.chromium.sync.test.util.MockAccountManager; import org.chromium.sync.test.util.MockSyncContentResolverDelegate; /** @@ -81,16 +84,17 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { private Account mAccount; private Account mAlternateAccount; private MockSyncSettingsObserver mSyncSettingsObserver; + private MockAccountManager mAccountManager; @Override protected void setUp() throws Exception { mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate(); Context context = getInstrumentation().getTargetContext(); + setupTestAccounts(context); + AndroidSyncSettings.overrideForTests(context, mSyncContentResolverDelegate); mAndroid = AndroidSyncSettings.get(context); mAuthority = mAndroid.getContractAuthority(); - mAccount = new Account("account@example.com", "com.google"); - mAlternateAccount = new Account("alternateAccount@example.com", "com.google"); mAndroid.updateAccount(mAccount); mSyncSettingsObserver = new MockSyncSettingsObserver(); @@ -99,6 +103,21 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { super.setUp(); } + private void setupTestAccounts(Context context) { + mAccountManager = new MockAccountManager(context, context); + AccountManagerHelper.overrideAccountManagerHelperForTests(context, mAccountManager); + mAccount = setupTestAccount("account@example.com"); + mAlternateAccount = setupTestAccount("alternate@example.com"); + } + + private Account setupTestAccount(String accountName) { + Account account = AccountManagerHelper.createAccountFromName(accountName); + AccountHolder.Builder accountHolder = + AccountHolder.create().account(account).password("password").alwaysAccept(true); + mAccountManager.addAccountHolderExplicitly(accountHolder.build()); + return account; + } + private void enableChromeSyncOnUiThread() { ThreadUtils.runOnUiThreadBlocking(new Runnable() { @Override @@ -294,4 +313,39 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { assertFalse("disableChromeSync shouldn't observers", mSyncSettingsObserver.receivedNotification()); } + + @SmallTest + @Feature({"Sync"}) + public void testIsSyncableOnSigninAndNotOnSignout() throws InterruptedException { + assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); + mAndroid.updateAccount(null); + assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 0); + mAndroid.updateAccount(mAccount); + assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); + } + + /** + * Regression test for crbug.com/475299. + */ + @SmallTest + @Feature({"Sync"}) + public void testSyncableIsAlwaysSetWhenEnablingSync() throws InterruptedException { + // Setup bad state. + mSyncContentResolverDelegate.setMasterSyncAutomatically(true); + mSyncContentResolverDelegate.waitForLastNotificationCompleted(); + mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, 1); + mSyncContentResolverDelegate.waitForLastNotificationCompleted(); + mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true); + mSyncContentResolverDelegate.waitForLastNotificationCompleted(); + mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, 0); + mSyncContentResolverDelegate.waitForLastNotificationCompleted(); + assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 0); + assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority)); + + // Ensure bug is fixed. + mAndroid.enableChromeSync(); + assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); + // Should still be enabled. + assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority)); + } } diff --git a/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java b/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java index be2622c..ea0b003 100644 --- a/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java +++ b/sync/test/android/javatests/src/org/chromium/sync/test/util/MockSyncContentResolverDelegate.java @@ -114,10 +114,6 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg synchronized (mSyncableMapLock) { switch (syncable) { case 0: - if (mSyncAutomaticallySet.contains(key)) { - mSyncAutomaticallySet.remove(key); - } - mIsSyncableMap.put(key, false); break; case 1: @@ -141,7 +137,7 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg String key = createKey(account, authority); synchronized (mSyncableMapLock) { if (mIsSyncableMap.containsKey(key)) { - return mIsSyncableMap.containsKey(key) ? 1 : 0; + return mIsSyncableMap.get(key) ? 1 : 0; } else { return -1; } -- cgit v1.1