diff options
author | maxbogue <maxbogue@chromium.org> | 2015-05-19 11:36:32 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-05-19 18:37:26 +0000 |
commit | 80344ba46a6299d852d068ebaf26d658847fc49d (patch) | |
tree | 23d05c841caedf42eae4b671f2c498458b3f61e7 /sync/android | |
parent | 08f8097c710309712d5100af194aae04213b60e3 (diff) | |
download | chromium_src-80344ba46a6299d852d068ebaf26d658847fc49d.zip chromium_src-80344ba46a6299d852d068ebaf26d658847fc49d.tar.gz chromium_src-80344ba46a6299d852d068ebaf26d658847fc49d.tar.bz2 |
[Sync] Make it impossible to get a reference to AndroidSyncSettings.
This change is motivated by some complex test flakiness issues that were
discovered in http://crrev.com/1118833002. Making it impossible to store
a reference means that if we overwrite it for tests, we know everyone is
then using the overwritten version.
The approach here is to make every public method static and take the
context as an argument, so it can initialize the inner object if
necessary.
This CL is part 1/3 and leaves in deprecated versions of all the methods.
In part 2/3 the downstream uses of AndroidSyncSettings will be changed,
and in part 3/3 the deprecated methods will be removed upstream.
BUG=480604
Review URL: https://codereview.chromium.org/1138013008
Cr-Commit-Position: refs/heads/master@{#330570}
Diffstat (limited to 'sync/android')
3 files changed, 167 insertions, 96 deletions
diff --git a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java index 3587e76..c36cef4 100644 --- a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java +++ b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java @@ -21,7 +21,9 @@ import javax.annotation.concurrent.ThreadSafe; * * It also provides an observer to be used whenever Android sync settings change. * - * To retrieve an instance of this class, call AndroidSyncSettings.get(someContext). + * This class is a collection of static methods so that no references to its object can be + * stored. This is important because tests need to be able to overwrite the object with a + * mock content resolver and know that no references to the old one are cached. * * This class must be initialized via updateAccount() on startup if the user is signed in. */ @@ -35,7 +37,7 @@ public class AndroidSyncSettings { */ private static final Object CLASS_LOCK = new Object(); - private static AndroidSyncSettings sAndroidSyncSettings; + private static AndroidSyncSettings sInstance; private final Object mLock = new Object(); @@ -63,37 +65,26 @@ public class AndroidSyncSettings { public void androidSyncSettingsChanged(); } - /** - * A factory method for the AndroidSyncSettings. - * - * It is possible to override the {@link SyncContentResolverDelegate} to use in tests for the - * instance of the AndroidSyncSettings by calling overrideForTests(...) with - * your {@link SyncContentResolverDelegate}. - * - * @param context the ApplicationContext is retrieved from the context used as an argument. - * @return a singleton instance of the AndroidSyncSettings - */ - public static AndroidSyncSettings get(Context context) { + private static void ensureInitialized(Context context) { synchronized (CLASS_LOCK) { - if (sAndroidSyncSettings == null) { + if (sInstance == null) { SyncContentResolverDelegate contentResolver = new SystemSyncContentResolverDelegate(); - sAndroidSyncSettings = new AndroidSyncSettings(context, contentResolver); + sInstance = new AndroidSyncSettings(context, contentResolver); } } - return sAndroidSyncSettings; } @VisibleForTesting public static void overrideForTests(Context context, SyncContentResolverDelegate contentResolver) { synchronized (CLASS_LOCK) { - sAndroidSyncSettings = new AndroidSyncSettings(context, contentResolver); + sInstance = new AndroidSyncSettings(context, contentResolver); } } /** - * @param context the context + * @param context the context the ApplicationContext will be retrieved from. * @param syncContentResolverDelegate an implementation of {@link SyncContentResolverDelegate}. */ private AndroidSyncSettings(Context context, @@ -117,8 +108,9 @@ public class AndroidSyncSettings { * * @return true if sync is on, false otherwise */ - public boolean isSyncEnabled() { - return mMasterSyncEnabled && mChromeSyncEnabled; + public static boolean isSyncEnabled(Context context) { + ensureInitialized(context); + return sInstance.mMasterSyncEnabled && sInstance.mChromeSyncEnabled; } /** @@ -129,66 +121,74 @@ public class AndroidSyncSettings { * * @return true if sync is on, false otherwise */ - public boolean isChromeSyncEnabled() { - return mChromeSyncEnabled; + public static boolean isChromeSyncEnabled(Context context) { + ensureInitialized(context); + return sInstance.mChromeSyncEnabled; } /** * Checks whether the master sync flag for Android is currently enabled. */ - public boolean isMasterSyncEnabled() { - return mMasterSyncEnabled; + public static boolean isMasterSyncEnabled(Context context) { + ensureInitialized(context); + return sInstance.mMasterSyncEnabled; } /** * Make sure Chrome is syncable, and enable sync. */ - public void enableChromeSync() { - setChromeSyncEnabled(true); + public static void enableChromeSync(Context context) { + ensureInitialized(context); + sInstance.setChromeSyncEnabled(true); } /** * Disables Android Chrome sync */ - public void disableChromeSync() { - setChromeSyncEnabled(false); + public static void disableChromeSync(Context context) { + ensureInitialized(context); + sInstance.setChromeSyncEnabled(false); } /** * Must be called when a new account is signed in. */ - public void updateAccount(Account account) { - synchronized (mLock) { - mAccount = account; - updateSyncability(); + public static void updateAccount(Context context, Account account) { + ensureInitialized(context); + synchronized (sInstance.mLock) { + sInstance.mAccount = account; + sInstance.updateSyncability(); } - if (updateCachedSettings()) { - notifyObservers(); + if (sInstance.updateCachedSettings()) { + sInstance.notifyObservers(); } } /** * Returns the contract authority to use when requesting sync. */ - public String getContractAuthority() { - return mApplicationContext.getPackageName(); + public static String getContractAuthority(Context context) { + ensureInitialized(context); + return sInstance.getContractAuthority(); } /** * Add a new AndroidSyncSettingsObserver. */ - public void registerObserver(AndroidSyncSettingsObserver observer) { - synchronized (mLock) { - mObservers.addObserver(observer); + public static void registerObserver(Context context, AndroidSyncSettingsObserver observer) { + ensureInitialized(context); + synchronized (sInstance.mLock) { + sInstance.mObservers.addObserver(observer); } } /** * Remove an AndroidSyncSettingsObserver that was previously added. */ - public void unregisterObserver(AndroidSyncSettingsObserver observer) { - synchronized (mLock) { - mObservers.removeObserver(observer); + public static void unregisterObserver(Context context, AndroidSyncSettingsObserver observer) { + ensureInitialized(context); + synchronized (sInstance.mLock) { + sInstance.mObservers.removeObserver(observer); } } @@ -290,4 +290,68 @@ public class AndroidSyncSettings { observer.androidSyncSettingsChanged(); } } + + // TODO(maxbogue): make private once downstream uses are removed. + @Deprecated + public String getContractAuthority() { + return mApplicationContext.getPackageName(); + } + + // Deprecated section; to be removed once downstream no longer uses them. + + @Deprecated + public static AndroidSyncSettings get(Context context) { + ensureInitialized(context); + return sInstance; + } + + @Deprecated + public boolean isSyncEnabled() { + return mMasterSyncEnabled && mChromeSyncEnabled; + } + + @Deprecated + public boolean isChromeSyncEnabled() { + return mChromeSyncEnabled; + } + + @Deprecated + public boolean isMasterSyncEnabled() { + return mMasterSyncEnabled; + } + + @Deprecated + public void enableChromeSync() { + setChromeSyncEnabled(true); + } + + @Deprecated + public void disableChromeSync() { + setChromeSyncEnabled(false); + } + + @Deprecated + public void updateAccount(Account account) { + synchronized (mLock) { + mAccount = account; + updateSyncability(); + } + if (updateCachedSettings()) { + notifyObservers(); + } + } + + @Deprecated + public void registerObserver(AndroidSyncSettingsObserver observer) { + synchronized (mLock) { + mObservers.addObserver(observer); + } + } + + @Deprecated + public void unregisterObserver(AndroidSyncSettingsObserver observer) { + synchronized (mLock) { + mObservers.removeObserver(observer); + } + } } diff --git a/sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java b/sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java index adfd384..4a5e8e8 100644 --- a/sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java +++ b/sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java @@ -44,14 +44,11 @@ public class ChromeSigninController { private final ObserverList<Listener> mListeners = new ObserverList<Listener>(); - private final AndroidSyncSettings mAndroidSyncSettings; - private boolean mGcmInitialized; private ChromeSigninController(Context context) { mApplicationContext = context.getApplicationContext(); - mAndroidSyncSettings = AndroidSyncSettings.get(context); - mAndroidSyncSettings.updateAccount(getSignedInUser()); + AndroidSyncSettings.updateAccount(context, getSignedInUser()); } /** @@ -86,7 +83,7 @@ public class ChromeSigninController { .putString(SIGNED_IN_ACCOUNT_KEY, accountName) .apply(); // TODO(maxbogue): Move this to SigninManager. - mAndroidSyncSettings.updateAccount(getSignedInUser()); + AndroidSyncSettings.updateAccount(mApplicationContext, getSignedInUser()); } public void clearSignedInUser() { diff --git a/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java b/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java index 13c43f6..94e099b 100644 --- a/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java +++ b/sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java @@ -78,7 +78,7 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { } } - private AndroidSyncSettings mAndroid; + private Context mContext; private CountingMockSyncContentResolverDelegate mSyncContentResolverDelegate; private String mAuthority; private Account mAccount; @@ -89,16 +89,15 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { @Override protected void setUp() throws Exception { mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate(); - Context context = getInstrumentation().getTargetContext(); - setupTestAccounts(context); + mContext = getInstrumentation().getTargetContext(); + setupTestAccounts(mContext); - AndroidSyncSettings.overrideForTests(context, mSyncContentResolverDelegate); - mAndroid = AndroidSyncSettings.get(context); - mAuthority = mAndroid.getContractAuthority(); - mAndroid.updateAccount(mAccount); + AndroidSyncSettings.overrideForTests(mContext, mSyncContentResolverDelegate); + mAuthority = AndroidSyncSettings.getContractAuthority(mContext); + AndroidSyncSettings.updateAccount(mContext, mAccount); mSyncSettingsObserver = new MockSyncSettingsObserver(); - mAndroid.registerObserver(mSyncSettingsObserver); + AndroidSyncSettings.registerObserver(mContext, mSyncSettingsObserver); super.setUp(); } @@ -122,7 +121,7 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { ThreadUtils.runOnUiThreadBlocking(new Runnable() { @Override public void run() { - mAndroid.enableChromeSync(); + AndroidSyncSettings.enableChromeSync(mContext); } }); } @@ -131,7 +130,7 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { ThreadUtils.runOnUiThreadBlocking(new Runnable() { @Override public void run() { - mAndroid.disableChromeSync(); + AndroidSyncSettings.disableChromeSync(mContext); } }); } @@ -141,11 +140,13 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { public void testToggleMasterSyncFromSettings() throws InterruptedException { mSyncContentResolverDelegate.setMasterSyncAutomatically(true); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("master sync should be set", mAndroid.isMasterSyncEnabled()); + assertTrue("master sync should be set", + AndroidSyncSettings.isMasterSyncEnabled(mContext)); mSyncContentResolverDelegate.setMasterSyncAutomatically(false); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertFalse("master sync should be unset", mAndroid.isMasterSyncEnabled()); + assertFalse("master sync should be unset", + AndroidSyncSettings.isMasterSyncEnabled(mContext)); } @SmallTest @@ -160,27 +161,33 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { mSyncContentResolverDelegate.waitForLastNotificationCompleted(); mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("sync should be set", mAndroid.isSyncEnabled()); - assertTrue("sync should be set for chrome app", mAndroid.isChromeSyncEnabled()); + assertTrue("sync should be set", AndroidSyncSettings.isSyncEnabled(mContext)); + assertTrue("sync should be set for chrome app", + AndroidSyncSettings.isChromeSyncEnabled(mContext)); // Disable sync automatically for the app mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, false); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertFalse("sync should be unset", mAndroid.isSyncEnabled()); - assertFalse("sync should be unset for chrome app", mAndroid.isChromeSyncEnabled()); + assertFalse("sync should be unset", AndroidSyncSettings.isSyncEnabled(mContext)); + assertFalse("sync should be unset for chrome app", + AndroidSyncSettings.isChromeSyncEnabled(mContext)); // Re-enable sync mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("sync should be re-enabled", mAndroid.isSyncEnabled()); - assertTrue("sync should be set for chrome app", mAndroid.isChromeSyncEnabled()); + assertTrue("sync should be re-enabled", AndroidSyncSettings.isSyncEnabled(mContext)); + assertTrue("sync should be set for chrome app", + AndroidSyncSettings.isChromeSyncEnabled(mContext)); // Disabled from master sync mSyncContentResolverDelegate.setMasterSyncAutomatically(false); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertFalse("sync should be disabled due to master sync", mAndroid.isSyncEnabled()); - assertFalse("master sync should be disabled", mAndroid.isMasterSyncEnabled()); - assertTrue("sync should be set for chrome app", mAndroid.isChromeSyncEnabled()); + assertFalse("sync should be disabled due to master sync", + AndroidSyncSettings.isSyncEnabled(mContext)); + assertFalse("master sync should be disabled", + AndroidSyncSettings.isMasterSyncEnabled(mContext)); + assertTrue("sync should be set for chrome app", + AndroidSyncSettings.isChromeSyncEnabled(mContext)); } @SmallTest @@ -192,11 +199,11 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { enableChromeSyncOnUiThread(); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("account should be synced", mAndroid.isSyncEnabled()); + assertTrue("account should be synced", AndroidSyncSettings.isSyncEnabled(mContext)); disableChromeSyncOnUiThread(); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertFalse("account should not be synced", mAndroid.isSyncEnabled()); + assertFalse("account should not be synced", AndroidSyncSettings.isSyncEnabled(mContext)); } @SmallTest @@ -208,22 +215,25 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { enableChromeSyncOnUiThread(); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("account should be synced", mAndroid.isSyncEnabled()); + assertTrue("account should be synced", AndroidSyncSettings.isSyncEnabled(mContext)); - mAndroid.updateAccount(mAlternateAccount); + AndroidSyncSettings.updateAccount(mContext, mAlternateAccount); enableChromeSyncOnUiThread(); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("alternate account should be synced", mAndroid.isSyncEnabled()); + assertTrue("alternate account should be synced", + AndroidSyncSettings.isSyncEnabled(mContext)); disableChromeSyncOnUiThread(); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertFalse("alternate account should not be synced", mAndroid.isSyncEnabled()); - mAndroid.updateAccount(mAccount); - assertTrue("account should still be synced", mAndroid.isSyncEnabled()); + assertFalse("alternate account should not be synced", + AndroidSyncSettings.isSyncEnabled(mContext)); + AndroidSyncSettings.updateAccount(mContext, mAccount); + assertTrue("account should still be synced", AndroidSyncSettings.isSyncEnabled(mContext)); // Ensure we don't erroneously re-use cached data. - mAndroid.updateAccount(null); - assertFalse("null account should not be synced", mAndroid.isSyncEnabled()); + AndroidSyncSettings.updateAccount(mContext, null); + assertFalse("null account should not be synced", + AndroidSyncSettings.isSyncEnabled(mContext)); } @SmallTest @@ -235,7 +245,7 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { enableChromeSyncOnUiThread(); mSyncContentResolverDelegate.waitForLastNotificationCompleted(); - assertTrue("account should be synced", mAndroid.isSyncEnabled()); + assertTrue("account should be synced", AndroidSyncSettings.isSyncEnabled(mContext)); int masterSyncAutomaticallyCalls = mSyncContentResolverDelegate.mGetMasterSyncAutomaticallyCalls; @@ -243,9 +253,9 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { int getSyncAutomaticallyAcalls = mSyncContentResolverDelegate.mGetSyncAutomaticallyCalls; // Do a bunch of reads. - mAndroid.isMasterSyncEnabled(); - mAndroid.isSyncEnabled(); - mAndroid.isChromeSyncEnabled(); + AndroidSyncSettings.isMasterSyncEnabled(mContext); + AndroidSyncSettings.isSyncEnabled(mContext); + AndroidSyncSettings.isChromeSyncEnabled(mContext); // Ensure values were read from cache. assertEquals(masterSyncAutomaticallyCalls, @@ -255,10 +265,10 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { mSyncContentResolverDelegate.mGetSyncAutomaticallyCalls); // Do a bunch of reads for alternate account. - mAndroid.updateAccount(mAlternateAccount); - mAndroid.isMasterSyncEnabled(); - mAndroid.isSyncEnabled(); - mAndroid.isChromeSyncEnabled(); + AndroidSyncSettings.updateAccount(mContext, mAlternateAccount); + AndroidSyncSettings.isMasterSyncEnabled(mContext); + AndroidSyncSettings.isSyncEnabled(mContext); + AndroidSyncSettings.isChromeSyncEnabled(mContext); // Ensure settings were only fetched once. assertEquals(masterSyncAutomaticallyCalls + 1, @@ -273,7 +283,7 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { public void testGetContractAuthority() throws Exception { assertEquals("The contract authority should be the package name.", getInstrumentation().getTargetContext().getPackageName(), - mAndroid.getContractAuthority()); + AndroidSyncSettings.getContractAuthority(mContext)); } @SmallTest @@ -284,32 +294,32 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { mSyncContentResolverDelegate.waitForLastNotificationCompleted(); mSyncSettingsObserver.clearNotification(); - mAndroid.enableChromeSync(); + AndroidSyncSettings.enableChromeSync(mContext); assertTrue("enableChromeSync should trigger observers", mSyncSettingsObserver.receivedNotification()); mSyncSettingsObserver.clearNotification(); - mAndroid.updateAccount(mAlternateAccount); + AndroidSyncSettings.updateAccount(mContext, mAlternateAccount); assertTrue("switching to account with different settings should notify", mSyncSettingsObserver.receivedNotification()); mSyncSettingsObserver.clearNotification(); - mAndroid.updateAccount(mAccount); + AndroidSyncSettings.updateAccount(mContext, mAccount); assertTrue("switching to account with different settings should notify", mSyncSettingsObserver.receivedNotification()); mSyncSettingsObserver.clearNotification(); - mAndroid.enableChromeSync(); + AndroidSyncSettings.enableChromeSync(mContext); assertFalse("enableChromeSync shouldn't trigger observers", mSyncSettingsObserver.receivedNotification()); mSyncSettingsObserver.clearNotification(); - mAndroid.disableChromeSync(); + AndroidSyncSettings.disableChromeSync(mContext); assertTrue("disableChromeSync should trigger observers", mSyncSettingsObserver.receivedNotification()); mSyncSettingsObserver.clearNotification(); - mAndroid.disableChromeSync(); + AndroidSyncSettings.disableChromeSync(mContext); assertFalse("disableChromeSync shouldn't observers", mSyncSettingsObserver.receivedNotification()); } @@ -318,9 +328,9 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { @Feature({"Sync"}) public void testIsSyncableOnSigninAndNotOnSignout() throws InterruptedException { assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); - mAndroid.updateAccount(null); + AndroidSyncSettings.updateAccount(mContext, null); assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 0); - mAndroid.updateAccount(mAccount); + AndroidSyncSettings.updateAccount(mContext, mAccount); assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); } @@ -343,7 +353,7 @@ public class AndroidSyncSettingsTest extends InstrumentationTestCase { assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority)); // Ensure bug is fixed. - mAndroid.enableChromeSync(); + AndroidSyncSettings.enableChromeSync(mContext); assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 1); // Should still be enabled. assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority)); |