Fixed SyncStatusHelper not updating observers on changes.

* SyncStatusHelper now updates observers whenever cached contents
  changes, previously only updated on content resolver notifications.

BUG=299123

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@231960 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 333 insertions, 33 deletions

diff --git a/sync/android/java/src/org/chromium/sync/notifier/SyncStatusHelper.java b/sync/android/java/src/org/chromium/sync/notifier/SyncStatusHelper.java
index cde85e2..35414b1 100644
--- a/sync/android/java/src/org/chromium/sync/notifier/SyncStatusHelper.java
+++ b/sync/android/java/src/org/chromium/sync/notifier/SyncStatusHelper.java
@@ -4,7 +4,6 @@
 
 package org.chromium.sync.notifier;
 
-
 import android.accounts.Account;
 import android.content.ContentResolver;
 import android.content.Context;
@@ -26,6 +25,8 @@ 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 SyncStatusHelper.get(someContext).
+ *
+ * All new public methods MUST call notifyObservers at the end.
  */
 @ThreadSafe
 public class SyncStatusHelper {
@@ -38,51 +39,96 @@ public class SyncStatusHelper {
      * expensive calls to Android are made.
      */
     @NotThreadSafe
-    private class CachedAccountSyncSettings {
+    @VisibleForTesting
+    public static class CachedAccountSyncSettings {
+        private final String mContractAuthority;
+        private final SyncContentResolverDelegate mSyncContentResolverWrapper;
         private Account mAccount;
+        private boolean mDidUpdate;
         private boolean mSyncAutomatically;
         private int mIsSyncable;
 
+        public CachedAccountSyncSettings(String contractAuthority,
+                SyncContentResolverDelegate contentResolverWrapper) {
+            mContractAuthority = contractAuthority;
+            mSyncContentResolverWrapper = contentResolverWrapper;
+        }
+
         private void ensureSettingsAreForAccount(Account account) {
             assert account != null;
             if (account.equals(mAccount)) return;
             updateSyncSettingsForAccount(account);
+            mDidUpdate = true;
         }
 
-        public void updateSyncSettingsForAccount(Account account) {
-            if (account == null) return;
-            mAccount = account;
+        public void clearUpdateStatus() {
+            mDidUpdate = false;
+        }
 
-            StrictMode.ThreadPolicy oldPolicy = temporarilyAllowDiskWritesAndDiskReads();
-            mSyncAutomatically = mSyncContentResolverWrapper.getSyncAutomatically(
-                    account, mContractAuthority);
-            mIsSyncable = mSyncContentResolverWrapper.getIsSyncable(account, mContractAuthority);
-            StrictMode.setThreadPolicy(oldPolicy);
+        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 == 0) return;
+            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 = temporarilyAllowDiskWritesAndDiskReads();
+            mSyncAutomatically = mSyncContentResolverWrapper.getSyncAutomatically(
+                    account, mContractAuthority);
+            mIsSyncable = mSyncContentResolverWrapper.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 = temporarilyAllowDiskWritesAndDiskReads();
             mSyncContentResolverWrapper.setIsSyncable(account, mContractAuthority, 1);
             StrictMode.setThreadPolicy(oldPolicy);
+            mDidUpdate = true;
         }
 
-        public void setSyncAutomatically(Account account, boolean value) {
-            ensureSettingsAreForAccount(account);
-            if (mSyncAutomatically == value) return;
-
+        @VisibleForTesting
+        protected void setSyncAutomaticallyInternal(Account account, boolean value) {
             mSyncAutomatically = value;
             StrictMode.ThreadPolicy oldPolicy = temporarilyAllowDiskWritesAndDiskReads();
             mSyncContentResolverWrapper.setSyncAutomatically(account, mContractAuthority, value);
             StrictMode.setThreadPolicy(oldPolicy);
+            mDidUpdate = true;
         }
     }
 
@@ -108,7 +154,7 @@ public class SyncStatusHelper {
     private boolean mCachedMasterSyncAutomatically;
 
     // Instantiation of SyncStatusHelper is guarded by a lock so volatile is unneeded.
-    private final CachedAccountSyncSettings mCachedSettings = new CachedAccountSyncSettings();
+    private CachedAccountSyncSettings mCachedSettings;
 
     private final ObserverList<SyncSettingsChangedObserver> mObservers =
             new ObserverList<SyncSettingsChangedObserver>();
@@ -125,10 +171,12 @@ public class SyncStatusHelper {
      * @param syncContentResolverWrapper an implementation of SyncContentResolverWrapper
      */
     private SyncStatusHelper(Context context,
-            SyncContentResolverDelegate syncContentResolverWrapper) {
+            SyncContentResolverDelegate syncContentResolverWrapper,
+            CachedAccountSyncSettings cachedAccountSettings) {
         mApplicationContext = context.getApplicationContext();
         mSyncContentResolverWrapper = syncContentResolverWrapper;
         mContractAuthority = getContractAuthority();
+        mCachedSettings = cachedAccountSettings;
 
         updateMasterSyncAutomaticallySetting();
 
@@ -159,8 +207,11 @@ public class SyncStatusHelper {
     public static SyncStatusHelper get(Context context) {
         synchronized (INSTANCE_LOCK) {
             if (sSyncStatusHelper == null) {
-                sSyncStatusHelper = new SyncStatusHelper(context,
-                        new SystemSyncContentResolverDelegate());
+                SyncContentResolverDelegate contentResolverDelegate =
+                        new SystemSyncContentResolverDelegate();
+                CachedAccountSyncSettings cache = new CachedAccountSyncSettings(
+                        context.getPackageName(), contentResolverDelegate);
+                sSyncStatusHelper = new SyncStatusHelper(context, contentResolverDelegate, cache);
             }
         }
         return sSyncStatusHelper;
@@ -175,15 +226,26 @@ public class SyncStatusHelper {
      */
     @VisibleForTesting
     public static void overrideSyncStatusHelperForTests(Context context,
-            SyncContentResolverDelegate syncContentResolverWrapper) {
+            SyncContentResolverDelegate syncContentResolverWrapper,
+            CachedAccountSyncSettings cachedAccountSettings) {
         synchronized (INSTANCE_LOCK) {
             if (sSyncStatusHelper != null) {
                 throw new IllegalStateException("SyncStatusHelper already exists");
             }
-            sSyncStatusHelper = new SyncStatusHelper(context, syncContentResolverWrapper);
+            sSyncStatusHelper = new SyncStatusHelper(context, syncContentResolverWrapper,
+                    cachedAccountSettings);
         }
     }
 
+    @VisibleForTesting
+    public static void overrideSyncStatusHelperForTests(Context context,
+            SyncContentResolverDelegate syncContentResolverWrapper) {
+        CachedAccountSyncSettings cachedAccountSettings = new CachedAccountSyncSettings(
+                context.getPackageName(), syncContentResolverWrapper);
+        overrideSyncStatusHelperForTests(context, syncContentResolverWrapper,
+                cachedAccountSettings);
+    }
+
     /**
      * Returns the contract authority to use when requesting sync.
      */
@@ -216,9 +278,14 @@ public class SyncStatusHelper {
      */
     public boolean isSyncEnabled(Account account) {
         if (account == null) return false;
+        boolean returnValue;
         synchronized (mCachedSettings) {
-            return mCachedMasterSyncAutomatically && mCachedSettings.getSyncAutomatically(account);
+            returnValue = mCachedMasterSyncAutomatically &&
+                mCachedSettings.getSyncAutomatically(account);
         }
+
+        notifyObservers();
+        return returnValue;
     }
 
     /**
@@ -244,9 +311,14 @@ public class SyncStatusHelper {
      */
     public boolean isSyncEnabledForChrome(Account account) {
         if (account == null) return false;
+
+        boolean returnValue;
         synchronized (mCachedSettings) {
-            return mCachedSettings.getSyncAutomatically(account);
+            returnValue = mCachedSettings.getSyncAutomatically(account);
         }
+
+        notifyObservers();
+        return returnValue;
     }
 
     /**
@@ -271,6 +343,8 @@ public class SyncStatusHelper {
         synchronized (mCachedSettings) {
             mCachedSettings.setSyncAutomatically(account, true);
         }
+
+        notifyObservers();
     }
 
     /**
@@ -282,6 +356,8 @@ public class SyncStatusHelper {
         synchronized (mCachedSettings) {
             mCachedSettings.setSyncAutomatically(account, false);
         }
+
+        notifyObservers();
     }
 
     /**
@@ -327,11 +403,7 @@ public class SyncStatusHelper {
                     mCachedSettings.updateSyncSettingsForAccount(
                             ChromeSigninController.get(mApplicationContext).getSignedInUser());
                 }
-
-                // Notify anyone else that's interested
-                for (SyncSettingsChangedObserver observer: mObservers) {
-                    observer.syncSettingsChanged();
-                }
+                notifyObservers();
             }
         }
     }
@@ -354,4 +426,20 @@ public class SyncStatusHelper {
         StrictMode.setThreadPolicy(newPolicy.build());
         return oldPolicy;
     }
+
+    private boolean getAndClearDidUpdateStatus() {
+        boolean didGetStatusUpdate;
+        synchronized (mCachedSettings) {
+            didGetStatusUpdate = mCachedSettings.getDidUpdateStatus();
+            mCachedSettings.clearUpdateStatus();
+        }
+        return didGetStatusUpdate;
+    }
+
+    private void notifyObservers() {
+        if (!getAndClearDidUpdateStatus()) return;
+        for (SyncSettingsChangedObserver observer: mObservers) {
+            observer.syncSettingsChanged();
+        }
+    }
 }
diff --git a/sync/android/javatests/src/org/chromium/sync/notifier/signin/SyncStatusHelperTest.java b/sync/android/javatests/src/org/chromium/sync/notifier/signin/SyncStatusHelperTest.java
index 746cd17..44007cb 100644
--- a/sync/android/javatests/src/org/chromium/sync/notifier/signin/SyncStatusHelperTest.java
+++ b/sync/android/javatests/src/org/chromium/sync/notifier/signin/SyncStatusHelperTest.java
@@ -5,12 +5,15 @@
 package org.chromium.sync.notifier.signin;
 
 import android.accounts.Account;
+import android.content.Context;
 import android.test.InstrumentationTestCase;
 import android.test.suitebuilder.annotation.SmallTest;
 
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.Feature;
 import org.chromium.sync.notifier.SyncStatusHelper;
+import org.chromium.sync.notifier.SyncStatusHelper.CachedAccountSyncSettings;
+import org.chromium.sync.notifier.SyncStatusHelper.SyncSettingsChangedObserver;
 import org.chromium.sync.signin.ChromeSigninController;
 import org.chromium.sync.test.util.MockSyncContentResolverDelegate;
 
@@ -21,6 +24,8 @@ public class SyncStatusHelperTest extends InstrumentationTestCase {
         private int mGetMasterSyncAutomaticallyCalls;
         private int mGetSyncAutomaticallyCalls;
         private int mGetIsSyncableCalls;
+        private int mSetIsSyncableCalls;
+        private int mSetSyncAutomaticallyCalls;
 
         @Override
         public boolean getMasterSyncAutomatically() {
@@ -39,23 +44,86 @@ public class SyncStatusHelperTest extends InstrumentationTestCase {
             mGetIsSyncableCalls++;
             return super.getIsSyncable(account, authority);
         }
+
+        @Override
+        public void setIsSyncable(Account account, String authority, int syncable) {
+            mSetIsSyncableCalls++;
+            super.setIsSyncable(account, authority, syncable);
+        }
+
+        @Override
+        public void setSyncAutomatically(Account account, String authority, boolean sync) {
+            mSetSyncAutomaticallyCalls++;
+            super.setSyncAutomatically(account, authority, sync);
+        }
     }
 
-    private SyncStatusHelper mHelper;
+    private static class CountingCachedAccountSyncSettings extends CachedAccountSyncSettings {
+        private int mUpdateSyncSettingsForAccountInternalCalls;
+        private int mSetIsSyncableInternalCalls;
+        private int mSetSyncAutomaticallyInternalCalls;
 
-    private CountingMockSyncContentResolverDelegate mSyncContentResolverDelegate;
+        public CountingCachedAccountSyncSettings(String contractAuthority,
+                MockSyncContentResolverDelegate contentResolverWrapper) {
+            super(contractAuthority, contentResolverWrapper);
+        }
 
-    private String mAuthority;
+        @Override
+        protected void updateSyncSettingsForAccountInternal(Account account) {
+            mUpdateSyncSettingsForAccountInternalCalls++;
+            super.updateSyncSettingsForAccountInternal(account);
+        }
 
-    private Account mTestAccount;
+        @Override
+        protected void setIsSyncableInternal(Account account) {
+            mSetIsSyncableInternalCalls++;
+            super.setIsSyncableInternal(account);
+        }
+
+        @Override
+        protected void setSyncAutomaticallyInternal(Account account, boolean value) {
+            mSetSyncAutomaticallyInternalCalls++;
+            super.setSyncAutomaticallyInternal(account, value);
+        }
+
+        public void resetCount() {
+            mUpdateSyncSettingsForAccountInternalCalls = 0;
+        }
+    }
+
+    private static class MockSyncSettingsObserver implements SyncSettingsChangedObserver {
+        private boolean mReceivedNotification;
+
+        public void clearNotification() {
+            mReceivedNotification = false;
+        }
 
+        public boolean didReceiveNotification() {
+            return mReceivedNotification;
+        }
+
+        @Override
+        public void syncSettingsChanged() {
+            mReceivedNotification = true;
+        }
+    }
+
+    private SyncStatusHelper mHelper;
+    private CountingMockSyncContentResolverDelegate mSyncContentResolverDelegate;
+    private String mAuthority;
+    private Account mTestAccount;
     private Account mAlternateTestAccount;
+    private CountingCachedAccountSyncSettings mCachedAccountSyncSettings;
+    private MockSyncSettingsObserver mSyncSettingsObserver;
 
     @Override
     protected void setUp() throws Exception {
         mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate();
+        Context context = getInstrumentation().getTargetContext();
+        mCachedAccountSyncSettings = new CountingCachedAccountSyncSettings(
+                context.getPackageName(), mSyncContentResolverDelegate);
         SyncStatusHelper.overrideSyncStatusHelperForTests(
-                getInstrumentation().getTargetContext(), mSyncContentResolverDelegate);
+                context, mSyncContentResolverDelegate, mCachedAccountSyncSettings);
         mHelper = SyncStatusHelper.get(getInstrumentation().getTargetContext());
         // Need to set the signed in account name to ensure that sync settings notifications
         // update the right account.
@@ -66,6 +134,10 @@ public class SyncStatusHelperTest extends InstrumentationTestCase {
                 .getContractAuthority();
         mTestAccount = new Account("account@example.com", "com.google");
         mAlternateTestAccount = new Account("alternateAccount@example.com", "com.google");
+
+        mSyncSettingsObserver = new MockSyncSettingsObserver();
+        mHelper.registerSyncSettingsChangedObserver(mSyncSettingsObserver);
+
         super.setUp();
     }
 
@@ -241,4 +313,144 @@ public class SyncStatusHelperTest extends InstrumentationTestCase {
                 getInstrumentation().getTargetContext().getPackageName(),
                 mHelper.getContractAuthority());
     }
+
+    @SmallTest
+    @Feature({"Sync"})
+    public void testCachedAccountSyncSettingsExitEarly() throws InterruptedException {
+        mSyncContentResolverDelegate.disableObserverNotifications();
+
+        mCachedAccountSyncSettings.updateSyncSettingsForAccount(null);
+        assertTrue("Update sync settings failed to exit early", mCachedAccountSyncSettings.
+                mUpdateSyncSettingsForAccountInternalCalls == 0);
+
+        mCachedAccountSyncSettings.updateSyncSettingsForAccount(mTestAccount);
+        assertTrue("Update sync settings should not have exited early", mCachedAccountSyncSettings.
+                mUpdateSyncSettingsForAccountInternalCalls == 1);
+
+        mCachedAccountSyncSettings.setIsSyncable(mTestAccount);
+        assertTrue("setIsSyncable should not have exited early",
+                mCachedAccountSyncSettings.mSetIsSyncableInternalCalls == 1);
+
+        mCachedAccountSyncSettings.setIsSyncable(mTestAccount);
+        assertTrue("setIsSyncable failed to exit early", mCachedAccountSyncSettings.
+                mSetIsSyncableInternalCalls == 1);
+
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, true);
+        assertTrue("setSyncAutomatically should not have to exited early",
+                mCachedAccountSyncSettings.mSetSyncAutomaticallyInternalCalls == 1);
+
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, true);
+        assertTrue("setSyncAutomatically failed to exit early",
+                mCachedAccountSyncSettings.mSetSyncAutomaticallyInternalCalls == 1);
+
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, false);
+        assertTrue("setSyncAutomatically should not have to exited early",
+                mCachedAccountSyncSettings.mSetSyncAutomaticallyInternalCalls == 2);
+
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, false);
+        assertTrue("setSyncAutomatically failed to exit early",
+                mCachedAccountSyncSettings.mSetSyncAutomaticallyInternalCalls == 2);
+    }
+
+    @SmallTest
+    @Feature({"Sync"})
+    public void testCachedAccountSyncSettingsDidUpdate() throws InterruptedException {
+        // Since we're just testing the cache we disable observer notifications to prevent
+        // notifications to SyncStatusHelper from mutating it.
+        mSyncContentResolverDelegate.disableObserverNotifications();
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.getSyncAutomatically(mTestAccount);
+        assertTrue("getSyncAutomatically on un-populated cache failed to update DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.getSyncAutomatically(mTestAccount);
+        assertFalse("getSyncAutomatically on populated cache updated DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.updateSyncSettingsForAccount(mAlternateTestAccount);
+        assertTrue("updateSyncSettingsForAccount failed to update DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+
+        mCachedAccountSyncSettings.updateSyncSettingsForAccount(mTestAccount);
+        assertTrue("updateSyncSettingsForAccount failed to update DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+
+        mCachedAccountSyncSettings.updateSyncSettingsForAccount(mTestAccount);
+        assertFalse("updateSyncSettingsForAccount updated DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.setIsSyncable(mTestAccount);
+        assertTrue("setIsSyncable failed to update DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.setIsSyncable(mTestAccount);
+        assertFalse("setIsSyncable updated DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, true);
+        assertTrue("setSyncAutomatically failed to update DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, true);
+        assertFalse("setSyncAutomatically updated DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, false);
+        assertTrue("setSyncAutomatically failed to update DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+
+        mCachedAccountSyncSettings.clearUpdateStatus();
+        mCachedAccountSyncSettings.setSyncAutomatically(mTestAccount, false);
+        assertFalse("setSyncAutomatically updated DidUpdate flag",
+                mCachedAccountSyncSettings.getDidUpdateStatus());
+    }
+
+    @SmallTest
+    @Feature({"Sync"})
+    public void testSyncStatusHelperPostsNotifications() throws InterruptedException {
+        // Turn on syncability.
+        mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
+        mSyncContentResolverDelegate.waitForLastNotificationCompleted();
+
+        mSyncSettingsObserver.clearNotification();
+        mHelper.isSyncEnabled(mAlternateTestAccount);
+        assertTrue("isSyncEnabled on wrongly populated cache did not trigger observers",
+                mSyncSettingsObserver.didReceiveNotification());
+
+        mSyncSettingsObserver.clearNotification();
+        mHelper.isSyncEnabled(mTestAccount);
+        assertTrue("isSyncEnabled on wrongly populated cache did not trigger observers",
+                mSyncSettingsObserver.didReceiveNotification());
+
+        mSyncSettingsObserver.clearNotification();
+        mHelper.enableAndroidSync(mTestAccount);
+        assertTrue("enableAndroidSync did not trigger observers",
+                mSyncSettingsObserver.didReceiveNotification());
+
+        mSyncSettingsObserver.clearNotification();
+        mHelper.enableAndroidSync(mTestAccount);
+        assertFalse("enableAndroidSync triggered observers",
+                mSyncSettingsObserver.didReceiveNotification());
+
+        mSyncSettingsObserver.clearNotification();
+        mHelper.disableAndroidSync(mTestAccount);
+        assertTrue("disableAndroidSync did not trigger observers",
+                mSyncSettingsObserver.didReceiveNotification());
+
+        mSyncSettingsObserver.clearNotification();
+        mHelper.disableAndroidSync(mTestAccount);
+        assertFalse("disableAndroidSync triggered observers",
+                mSyncSettingsObserver.didReceiveNotification());
+    }
 }