Rewrite AndroidSyncSettings to be significantly simpler.

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

Index: sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
diff --git a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
index 99d66dafce0a1de90c3110e85d7b68fc802c9e8a..30bb35a4cfcc89a6bb33c2f6b650a9223cc25d87 100644
--- a/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
+++ b/sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java
@@ -13,9 +13,7 @@ import android.os.StrictMode;
 import org.chromium.base.ObserverList;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.sync.signin.AccountManagerHelper;
-import org.chromium.sync.signin.ChromeSigninController;
 
-import javax.annotation.concurrent.NotThreadSafe;
 import javax.annotation.concurrent.ThreadSafe;
 
 /**
@@ -24,119 +22,14 @@ 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).
- *
- * All new public methods MUST call notifyObservers at the end.
  */
 @ThreadSafe
 public class AndroidSyncSettings {
 
-    /**
-     * In-memory holder of the sync configurations for a given account. On each
-     * access, updates the cache if the account has changed. This lazy-updating
-     * model is appropriate as the account changes rarely but may not be known
-     * when initially constructed. So long as we keep a single account, no
-     * expensive calls to Android are made.
-     */
-    @NotThreadSafe
-    @VisibleForTesting
-    public static class CachedAccountSyncSettings {
-        private final String mContractAuthority;
-        private final SyncContentResolverDelegate mSyncContentResolverDelegate;
-        private Account mAccount;
-        private boolean mDidUpdate;
-        private boolean mSyncAutomatically;
-        private int mIsSyncable;
-
-        public CachedAccountSyncSettings(String contractAuthority,
-                SyncContentResolverDelegate contentResolverWrapper) {
-            mContractAuthority = contractAuthority;
-            mSyncContentResolverDelegate = contentResolverWrapper;
-        }
-
-        private void ensureSettingsAreForAccount(Account account) {
-            assert account != null;
-            if (account.equals(mAccount)) return;
-            updateSyncSettingsForAccount(account);
-            mDidUpdate = true;
-        }
-
-        public void clearUpdateStatus() {
-            mDidUpdate = false;
-        }
-
-        public boolean getDidUpdateStatus() {
-            return mDidUpdate;
-        }
-
-        // Calling this method may have side-effects.
-        public boolean getSyncAutomatically(Account account) {
-            ensureSettingsAreForAccount(account);
-            return mSyncAutomatically;
-        }
-
-        public void updateSyncSettingsForAccount(Account account) {
-            if (account == null) return;
-            updateSyncSettingsForAccountInternal(account);
-        }
-
-        public void setIsSyncable(Account account) {
-            ensureSettingsAreForAccount(account);
-            if (mIsSyncable == 1) return;
-            setIsSyncableInternal(account);
-        }
-
-        public void setSyncAutomatically(Account account, boolean value) {
-            ensureSettingsAreForAccount(account);
-            if (mSyncAutomatically == value) return;
-            setSyncAutomaticallyInternal(account, value);
-        }
-
-        @VisibleForTesting
-        protected void updateSyncSettingsForAccountInternal(Account account) {
-            // Null check here otherwise Findbugs complains.
-            if (account == null) return;
-
-            boolean oldSyncAutomatically = mSyncAutomatically;
-            int oldIsSyncable = mIsSyncable;
-            Account oldAccount = mAccount;
-
-            mAccount = account;
-
-            StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
-            mSyncAutomatically = mSyncContentResolverDelegate.getSyncAutomatically(
-                    account, mContractAuthority);
-            mIsSyncable = mSyncContentResolverDelegate.getIsSyncable(account, mContractAuthority);
-            StrictMode.setThreadPolicy(oldPolicy);
-            mDidUpdate = (oldIsSyncable != mIsSyncable)
-                || (oldSyncAutomatically != mSyncAutomatically)
-                || (!account.equals(oldAccount));
-        }
-
-        @VisibleForTesting
-        protected void setIsSyncableInternal(Account account) {
-            mIsSyncable = 1;
-            StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
-            mSyncContentResolverDelegate.setIsSyncable(account, mContractAuthority, 1);
-            StrictMode.setThreadPolicy(oldPolicy);
-            mDidUpdate = true;
-        }
-
-        @VisibleForTesting
-        protected void setSyncAutomaticallyInternal(Account account, boolean value) {
-            mSyncAutomatically = value;
-            StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
-            mSyncContentResolverDelegate.setSyncAutomatically(account, mContractAuthority, value);
-            StrictMode.setThreadPolicy(oldPolicy);
-            mDidUpdate = true;
-        }
-    }
-
     public static final String TAG = "AndroidSyncSettings";
 
-    /**
-     * Lock for ensuring singleton instantiation across threads.
-     */
-    private static final Object INSTANCE_LOCK = new Object();
+    // Lock for ensuring singleton instantiation across threads.
+    private static final Object LOCK = new Object();
 
     private static AndroidSyncSettings sAndroidSyncSettings;
 
@@ -146,10 +39,13 @@ public class AndroidSyncSettings {
 
     private final SyncContentResolverDelegate mSyncContentResolverDelegate;
 
-    private boolean mCachedMasterSyncAutomatically;
+    private Account mAccount = null;
+
+    private boolean mIsSyncable = false;
+
+    private boolean mChromeSyncEnabled = false;
 
-    // Instantiation of AndroidSyncSettings is guarded by a lock so volatile is unneeded.
-    private CachedAccountSyncSettings mCachedSettings;
+    private boolean mMasterSyncEnabled = false;
 
     private final ObserverList<AndroidSyncSettingsObserver> mObservers =
             new ObserverList<AndroidSyncSettingsObserver>();
@@ -162,125 +58,49 @@ public class AndroidSyncSettings {
     }
 
     /**
-     * @param context the context
-     * @param syncContentResolverDelegate an implementation of {@link SyncContentResolverDelegate}.
-     */
-    private AndroidSyncSettings(Context context,
-            SyncContentResolverDelegate syncContentResolverDelegate,
-            CachedAccountSyncSettings cachedAccountSettings) {
-        mApplicationContext = context.getApplicationContext();
-        mSyncContentResolverDelegate = syncContentResolverDelegate;
-        mContractAuthority = getContractAuthority();
-        mCachedSettings = cachedAccountSettings;
-
-        updateMasterSyncAutomaticallySetting();
-
-        mSyncContentResolverDelegate.addStatusChangeListener(
-                ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS,
-                new AndroidSyncSettingsChangedObserver());
-    }
-
-    private void updateMasterSyncAutomaticallySetting() {
-        StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
-        synchronized (mCachedSettings) {
-            mCachedMasterSyncAutomatically = mSyncContentResolverDelegate
-                    .getMasterSyncAutomatically();
-        }
-        StrictMode.setThreadPolicy(oldPolicy);
-    }
-
-    /**
      * 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 overrideAndroidSyncSettingsForTests(...) with
+     * 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) {
-        synchronized (INSTANCE_LOCK) {
+        synchronized (LOCK) {
             if (sAndroidSyncSettings == null) {
-                SyncContentResolverDelegate contentResolverDelegate =
+                SyncContentResolverDelegate contentResolver =
                         new SystemSyncContentResolverDelegate();
-                CachedAccountSyncSettings cache = new CachedAccountSyncSettings(
-                        context.getPackageName(), contentResolverDelegate);
-                sAndroidSyncSettings = new AndroidSyncSettings(
-                        context, contentResolverDelegate, cache);
+                sAndroidSyncSettings = new AndroidSyncSettings(context, contentResolver);
             }
         }
         return sAndroidSyncSettings;
     }
 
-    /**
-     * Tests might want to consider overriding the context and {@link SyncContentResolverDelegate}
-     * so they do not use the real ContentResolver in Android.
-     *
-     * @param context the context to use
-     * @param syncContentResolverDelegate the {@link SyncContentResolverDelegate} to use
-     */
     @VisibleForTesting
-    public static void overrideAndroidSyncSettingsForTests(Context context,
-            SyncContentResolverDelegate syncContentResolverDelegate,
-            CachedAccountSyncSettings cachedAccountSettings) {
-        synchronized (INSTANCE_LOCK) {
-            if (sAndroidSyncSettings != null) {
-                throw new IllegalStateException("AndroidSyncSettings already exists");
-            }
-            sAndroidSyncSettings = new AndroidSyncSettings(context, syncContentResolverDelegate,
-                    cachedAccountSettings);
+    public static void overrideForTests(Context context,
+            SyncContentResolverDelegate contentResolver) {
+        synchronized (LOCK) {
+            sAndroidSyncSettings = new AndroidSyncSettings(context, contentResolver);
         }
     }
 
-    @VisibleForTesting
-    public static void overrideAndroidSyncSettingsForTests(Context context,
-            SyncContentResolverDelegate syncContentResolverDelegate) {
-        CachedAccountSyncSettings cachedAccountSettings = new CachedAccountSyncSettings(
-                context.getPackageName(), syncContentResolverDelegate);
-        overrideAndroidSyncSettingsForTests(context, syncContentResolverDelegate,
-                cachedAccountSettings);
-    }
-
     /**
-     * Returns the contract authority to use when requesting sync.
-     */
-    public String getContractAuthority() {
-        return mApplicationContext.getPackageName();
-    }
-
-    /**
-     * Add a new AndroidSyncSettingsObserver.
-     */
-    public void registerObserver(AndroidSyncSettingsObserver observer) {
-        mObservers.addObserver(observer);
-    }
-
-    /**
-     * Remove an AndroidSyncSettingsObserver that was previously added.
+     * @param context the context
+     * @param syncContentResolverDelegate an implementation of {@link SyncContentResolverDelegate}.
      */
-    public void unregisterObserver(AndroidSyncSettingsObserver observer) {
-        mObservers.removeObserver(observer);
-    }
+    private AndroidSyncSettings(Context context,
+            SyncContentResolverDelegate syncContentResolverDelegate) {
+        mApplicationContext = context.getApplicationContext();
+        mSyncContentResolverDelegate = syncContentResolverDelegate;
+        mContractAuthority = getContractAuthority();
 
-    /**
-     * Checks whether sync is currently enabled from Chrome for a given account.
-     *
-     * It checks both the master sync for the device, and Chrome sync setting for the given account.
-     *
-     * @param account the account to check if Chrome sync is enabled on.
-     * @return true if sync is on, false otherwise
-     */
-    public boolean isSyncEnabled(Account account) {
-        if (account == null) return false;
-        boolean returnValue;
-        synchronized (mCachedSettings) {
-            returnValue = mCachedMasterSyncAutomatically
-                    && mCachedSettings.getSyncAutomatically(account);
-        }
+        updateCachedSettings();
 
-        notifyObserversIfAccountSettingsChanged();
-        return returnValue;
+        mSyncContentResolverDelegate.addStatusChangeListener(
+                ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS,
+                new AndroidSyncSettingsChangedObserver());
     }
 
     /**
@@ -292,7 +112,7 @@ public class AndroidSyncSettings {
      * @return true if sync is on, false otherwise
      */
     public boolean isSyncEnabled() {
-        return isSyncEnabled(ChromeSigninController.get(mApplicationContext).getSignedInUser());
+        return mMasterSyncEnabled && mChromeSyncEnabled;
     }
 
     /**
@@ -304,76 +124,94 @@ public class AndroidSyncSettings {
      * @param account the account to check if Chrome sync is enabled on.
      * @return true if sync is on, false otherwise
      */
-    public boolean isChromeSyncEnabled(Account account) {
-        if (account == null) return false;
-
-        boolean returnValue;
-        synchronized (mCachedSettings) {
-            returnValue = mCachedSettings.getSyncAutomatically(account);
-        }
-
-        notifyObserversIfAccountSettingsChanged();
-        return returnValue;
+    public boolean isChromeSyncEnabled() {
+        return mChromeSyncEnabled;
     }
 
     /**
-     * Checks whether the master sync flag for Android is currently set.
-     *
-     * @return true if the global master sync is on, false otherwise
+     * Checks whether the master sync flag for Android is currently enabled.
      */
     public boolean isMasterSyncEnabled() {
-        synchronized (mCachedSettings) {
-            return mCachedMasterSyncAutomatically;
-        }
+        return mMasterSyncEnabled;
     }
 
     /**
      * Make sure Chrome is syncable, and enable sync.
-     *
-     * @param account the account to enable sync on
      */
-    public void enableChromeSync(Account account) {
-        makeSyncable(account);
+    public void enableChromeSync() {
+        makeSyncable();
+        setChromeSyncEnabled(true);

[nyquist, 2015/02/06 03:03:56]

This class is marked as @ThreadSafe. Is it still safe to call makeSyncable and
setChromeSyncEnabled in two different calls?

[maxbogue, 2015/02/10 09:20:00]

makeSyncable is now ensureSyncable and is called from within
setChromeSyncEnabled().

+    }
+
+    /**
+     * Disables Android Chrome sync
+     */
+    public void disableChromeSync() {
+        setChromeSyncEnabled(false);
+    }
 
-        synchronized (mCachedSettings) {
-            mCachedSettings.setSyncAutomatically(account, true);
+    /**
+     * Must be called when a new account is signed in.
+     */
+    public void updateAccount(Account account) {
+        mAccount = account;
+        if (updateCachedSettings()) {
+            notifyObservers();
         }
+    }
 
-        notifyObserversIfAccountSettingsChanged();
+    /**
+     * Returns the contract authority to use when requesting sync.
+     */
+    public String getContractAuthority() {
+        return mApplicationContext.getPackageName();
     }
 
     /**
-     * Disables Android Chrome sync
-     *
-     * @param account the account to disable Chrome sync on
+     * Add a new AndroidSyncSettingsObserver.
      */
-    public void disableChromeSync(Account account) {
-        synchronized (mCachedSettings) {
-            mCachedSettings.setSyncAutomatically(account, false);
-        }
+    public void registerObserver(AndroidSyncSettingsObserver observer) {
+        mObservers.addObserver(observer);

[nyquist, 2015/02/06 03:03:56]

I believe ObserverList is marked @NotThreadSafe, but this class is marked
@ThreadSafe, so I think you need to wrap this using synchronized (and for
unregister and iterations probably).

[maxbogue, 2015/02/10 09:20:00]

Done for register and unregister.

+    }
+
+    /**
+     * Remove an AndroidSyncSettingsObserver that was previously added.
+     */
+    public void unregisterObserver(AndroidSyncSettingsObserver observer) {
+        mObservers.removeObserver(observer);
+    }
+
+    private synchronized void setChromeSyncEnabled(boolean value) {

[nyquist, 2015/02/06 03:03:56]

This synchronizes on the instance of this object, which we don't have control
over. Other users of this object could use the lock for their own purposes.
Create an internal lock that you fully control instead. Same for the methods
below.

[maxbogue, 2015/02/10 09:20:00]

Done.

+        if (value == mChromeSyncEnabled || mAccount == null) return;
+        mChromeSyncEnabled = value;
+
+        StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
+        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mContractAuthority, value);
+        StrictMode.setThreadPolicy(oldPolicy);
 
-        notifyObserversIfAccountSettingsChanged();
+        notifyObservers();
     }
 
     /**
-     * Register with Android Sync Manager. This is what causes the "Chrome" option to appear in
-     * Settings -> Accounts / Sync .
+     * Register Chrome with the Android Sync Manager.
      *
-     * @param account the account to enable Chrome sync on
+     * This is what causes the "Chrome" option to appear in Settings -> Accounts / Sync .
      */
-    private void makeSyncable(Account account) {
-        synchronized (mCachedSettings) {
-            mCachedSettings.setIsSyncable(account);
-        }
+    private synchronized void makeSyncable() {
+        if (mIsSyncable || mAccount == null) return;
+
+        mIsSyncable = true;
 
         StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
+        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
         // extra calls to Android sync configuration.
         Account[] googleAccounts = AccountManagerHelper.get(mApplicationContext)
                 .getGoogleAccounts();
         for (Account accountToSetNotSyncable : googleAccounts) {
-            if (!accountToSetNotSyncable.equals(account)
+            if (!accountToSetNotSyncable.equals(mAccount)
                     && mSyncContentResolverDelegate.getIsSyncable(
                             accountToSetNotSyncable, mContractAuthority) > 0) {
                 mSyncContentResolverDelegate.setIsSyncable(accountToSetNotSyncable,
@@ -391,37 +229,40 @@ public class AndroidSyncSettings {
     private class AndroidSyncSettingsChangedObserver implements SyncStatusObserver {
         @Override
         public void onStatusChanged(int which) {
-            if (ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS == which) {
-                // Sync settings have changed; update our in-memory caches
-                synchronized (mCachedSettings) {
-                    mCachedSettings.updateSyncSettingsForAccount(
-                            ChromeSigninController.get(mApplicationContext).getSignedInUser());
-                }
-
-                boolean oldMasterSyncEnabled = isMasterSyncEnabled();
-                updateMasterSyncAutomaticallySetting();
-                boolean didMasterSyncChanged = oldMasterSyncEnabled != isMasterSyncEnabled();
-                // Notify observers if MasterSync or account level settings change.
-                if (didMasterSyncChanged || getAndClearDidUpdateStatus()) {
+            if (which == ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS) {
+                // Sync settings have changed; update our cached values.
+                if (updateCachedSettings()) {
+                    // If something actually changed, tell our observers.
                     notifyObservers();
                 }
             }
         }
     }
 
-    private boolean getAndClearDidUpdateStatus() {
-        boolean didGetStatusUpdate;
-        synchronized (mCachedSettings) {
-            didGetStatusUpdate = mCachedSettings.getDidUpdateStatus();
-            mCachedSettings.clearUpdateStatus();
-        }
-        return didGetStatusUpdate;
-    }
+    /**
+     * Update the three cached settings from the content resolver.
+     *
+     * @return Whether either chromeSyncEnabled or masterSyncEnabled changed.
+     */
+    private synchronized boolean updateCachedSettings() {
+        boolean oldChromeSyncEnabled = mChromeSyncEnabled;
+        boolean oldMasterSyncEnabled = mMasterSyncEnabled;
 
-    private void notifyObserversIfAccountSettingsChanged() {
-        if (getAndClearDidUpdateStatus()) {
-            notifyObservers();
+        StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
+        if (mAccount != null) {
+            mIsSyncable = mSyncContentResolverDelegate.getIsSyncable(
+                    mAccount, mContractAuthority) == 1;
+            mChromeSyncEnabled = mSyncContentResolverDelegate.getSyncAutomatically(
+                    mAccount, mContractAuthority);
+        } else {
+            mIsSyncable = false;
+            mChromeSyncEnabled = false;
         }
+        mMasterSyncEnabled = mSyncContentResolverDelegate.getMasterSyncAutomatically();
+        StrictMode.setThreadPolicy(oldPolicy);
+
+        return oldChromeSyncEnabled != mChromeSyncEnabled
+            || oldMasterSyncEnabled != mMasterSyncEnabled;
     }
 
     private void notifyObservers() {
@@ -429,4 +270,21 @@ public class AndroidSyncSettings {
             observer.androidSyncSettingsChanged();
         }
     }
+
+    @Deprecated
+    @VisibleForTesting
+    public static void overrideAndroidSyncSettingsForTests(Context context,
+            SyncContentResolverDelegate contentResolver) {
+        overrideForTests(context, contentResolver);
+    }
+
+    @Deprecated
+    public boolean isChromeSyncEnabled(Account account) {
+        return isChromeSyncEnabled();
+    }
+
+    @Deprecated
+    public boolean isSyncEnabled(Account account) {
+        return isSyncEnabled();
+    }
 }