Make AndroidSyncSettings.isSyncEnabled() internal to SyncController

This method was one of the big villains of AndroidSyncSettings, because
it was easy to use it instead of some ProfileSyncService API. This CL
makes it internal to SyncController, avoiding new uses. A warning is
added to isChromeSyncEnabled() to prevent developers from thinking the
method has just been renamed. Extra care is taken not to weaken the
tests in AndroidSyncSettingsTest, replacing expectations on this method
accordingly.

Bug: 1107904
Change-Id: I6762255cf89881ac082d01e860ff0cbd91eace83
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2571101
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833401}

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

@@ -131,26 +131,12 @@ public class AndroidSyncSettings {
     }
 
     /**
-     * Checks whether sync is currently enabled from Chrome for the currently signed in account.
+     * DEPRECATED - DO NOT USE! You probably want ProfileSyncService.isSyncRequested() instead.
      *
-     * It checks both the master sync for the device, and Chrome sync setting for the given account.
-     * If no user is currently signed in it returns false.
-     *
-     * @return true if sync is on, false otherwise
-     */
-    public boolean isSyncEnabled() {
-        ThreadUtils.assertOnUiThread();
-        return mChromeSyncEnabled && doesMasterSyncSettingAllowChromeSync();
-    }
-
-    /**
-     * Checks whether sync is currently enabled for Chrome for a given account.
-     *
-     * It checks only Chrome sync setting for the given account,
-     * and ignores the master sync setting.
-     *
-     * @return true if sync is on, false otherwise
+     * @return The state of the Chrome sync setting for the given account,
+     * *ignoring* the master sync setting.
      */
+    @Deprecated
     public boolean isChromeSyncEnabled() {
         ThreadUtils.assertOnUiThread();
         return mChromeSyncEnabled;

chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java

@@ -100,9 +100,8 @@ public class SyncController
         mProfileSyncService.setSyncAllowedByPlatform(
                 AndroidSyncSettings.get().doesMasterSyncSettingAllowChromeSync());
 
-        boolean isSyncEnabled = AndroidSyncSettings.get().isSyncEnabled();
-        if (isSyncEnabled == mProfileSyncService.isSyncRequested()) return;
-        if (isSyncEnabled) {
+        if (isSyncEnabledInAndroidSyncSettings() == mProfileSyncService.isSyncRequested()) return;
+        if (isSyncEnabledInAndroidSyncSettings()) {
             mProfileSyncService.requestStart();
             return;
         }
@@ -138,11 +137,11 @@ public class SyncController
     public void syncStateChanged() {
         ThreadUtils.assertOnUiThread();
         if (mProfileSyncService.isSyncRequested()) {
-            if (!AndroidSyncSettings.get().isSyncEnabled()) {
+            if (!isSyncEnabledInAndroidSyncSettings()) {
                 AndroidSyncSettings.get().enableChromeSync();
             }
         } else {
-            if (AndroidSyncSettings.get().isSyncEnabled()) {
+            if (isSyncEnabledInAndroidSyncSettings()) {
                 // Both Android's master and Chrome sync setting are enabled, so we want to disable
                 // the Chrome sync setting to match isSyncRequested. We have to be careful not to
                 // disable it when isSyncRequested becomes false due to master sync being disabled
@@ -193,4 +192,13 @@ public class SyncController
         }
         mProfileSyncService.setSessionsId(SESSION_TAG_PREFIX + uniqueTag);
     }
+
+    /**
+     * Checks both the master sync for the device, and Chrome sync setting for the given account.
+     * If no user is currently signed in it returns false.
+     */
+    private boolean isSyncEnabledInAndroidSyncSettings() {
+        return AndroidSyncSettings.get().doesMasterSyncSettingAllowChromeSync()
+                && AndroidSyncSettings.get().isChromeSyncEnabled();
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/sync/AndroidSyncSettingsTest.java

@@ -10,7 +10,6 @@ import static org.mockito.Mockito.when;
 
 import static org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils.getDoesMasterSyncAllowSyncOnUiThread;
 import static org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread;
-import static org.chromium.chrome.browser.sync.AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread;
 
 import android.accounts.Account;
 import android.os.Bundle;
@@ -221,17 +220,17 @@ public class AndroidSyncSettingsTest {
 
         // First sync
         mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
-        Assert.assertTrue(getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(getIsChromeSyncEnabledOnUiThread());
 
         // Disable sync automatically for the app
         mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, false);
-        Assert.assertFalse(getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertFalse(getIsChromeSyncEnabledOnUiThread());
 
         // Re-enable sync
         mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
-        Assert.assertTrue(getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(getIsChromeSyncEnabledOnUiThread());
 
         // Disable master sync
@@ -239,10 +238,8 @@ public class AndroidSyncSettingsTest {
         Assert.assertTrue(getIsChromeSyncEnabledOnUiThread());
         if (ChromeFeatureList.isEnabled(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
             Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
-            Assert.assertTrue(getIsSyncEnabledOnUiThread());
         } else {
             Assert.assertFalse(getDoesMasterSyncAllowSyncOnUiThread());
-            Assert.assertFalse(getIsSyncEnabledOnUiThread());
         }
     }
 
@@ -255,10 +252,10 @@ public class AndroidSyncSettingsTest {
         createAndroidSyncSettings();
 
         TestThreadUtils.runOnUiThreadBlocking(() -> mAndroidSyncSettings.enableChromeSync());
-        Assert.assertTrue(getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(getIsChromeSyncEnabledOnUiThread());
 
         TestThreadUtils.runOnUiThreadBlocking(() -> mAndroidSyncSettings.disableChromeSync());
-        Assert.assertFalse(getIsSyncEnabledOnUiThread());
+        Assert.assertFalse(getIsChromeSyncEnabledOnUiThread());
     }
 
     @Test
@@ -271,20 +268,20 @@ public class AndroidSyncSettingsTest {
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             mAndroidSyncSettings.enableChromeSync();
-            Assert.assertTrue(mAndroidSyncSettings.isSyncEnabled());
+            Assert.assertTrue(mAndroidSyncSettings.isChromeSyncEnabled());
 
             mAndroidSyncSettings.updateAccount(mAlternateAccount);
             mAndroidSyncSettings.enableChromeSync();
-            Assert.assertTrue(mAndroidSyncSettings.isSyncEnabled());
+            Assert.assertTrue(mAndroidSyncSettings.isChromeSyncEnabled());
 
             mAndroidSyncSettings.disableChromeSync();
-            Assert.assertFalse(mAndroidSyncSettings.isSyncEnabled());
+            Assert.assertFalse(mAndroidSyncSettings.isChromeSyncEnabled());
             mAndroidSyncSettings.updateAccount(mAccount);
-            Assert.assertTrue(mAndroidSyncSettings.isSyncEnabled());
+            Assert.assertTrue(mAndroidSyncSettings.isChromeSyncEnabled());
 
             // Ensure we don't erroneously re-use cached data.
             mAndroidSyncSettings.updateAccount(null);
-            Assert.assertFalse(mAndroidSyncSettings.isSyncEnabled());
+            Assert.assertFalse(mAndroidSyncSettings.isChromeSyncEnabled());
         });
     }
 
@@ -298,7 +295,7 @@ public class AndroidSyncSettingsTest {
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             mAndroidSyncSettings.enableChromeSync();
-            Assert.assertTrue(mAndroidSyncSettings.isSyncEnabled());
+            Assert.assertTrue(mAndroidSyncSettings.isChromeSyncEnabled());
 
             int masterSyncAutomaticallyCalls =
                     mSyncContentResolverDelegate.mGetMasterSyncAutomaticallyCalls.get();
@@ -308,7 +305,6 @@ public class AndroidSyncSettingsTest {
 
             // Do a bunch of reads.
             mAndroidSyncSettings.doesMasterSyncSettingAllowChromeSync();
-            mAndroidSyncSettings.isSyncEnabled();
             mAndroidSyncSettings.isChromeSyncEnabled();
 
             // Ensure values were read from cache.
@@ -322,7 +318,6 @@ public class AndroidSyncSettingsTest {
             // Do a bunch of reads for alternate account.
             mAndroidSyncSettings.updateAccount(mAlternateAccount);
             mAndroidSyncSettings.doesMasterSyncSettingAllowChromeSync();
-            mAndroidSyncSettings.isSyncEnabled();
             mAndroidSyncSettings.isChromeSyncEnabled();
 
             // Ensure settings were only fetched once.
@@ -422,28 +417,24 @@ public class AndroidSyncSettingsTest {
         // Master sync is disabled on startup and the user hasn't gone through the decoupling flow
         // in the past.
         mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
-        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             when(mProfileSyncService.getDecoupledFromAndroidMasterSync()).thenReturn(false);
         });
 
-        // Sync must remain disabled as long as master sync is not re-enabled.
+        // Sync must remain disallowed as long as master sync is not re-enabled.
         createAndroidSyncSettings();
         Assert.assertFalse(getDoesMasterSyncAllowSyncOnUiThread());
-        Assert.assertFalse(getIsSyncEnabledOnUiThread());
 
-        // Re-enable master sync at least once. Sync is now re-enabled and decoupled from master
+        // Re-enable master sync at least once. Sync is now allowed and decoupled from master
         // sync.
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> doNothing().when(mProfileSyncService).setDecoupledFromAndroidMasterSync());
         mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
         Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
-        Assert.assertTrue(getIsSyncEnabledOnUiThread());
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> verify(mProfileSyncService).setDecoupledFromAndroidMasterSync());
         mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
         Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
-        Assert.assertTrue(getIsSyncEnabledOnUiThread());
     }
 
     @Test
@@ -454,7 +445,6 @@ public class AndroidSyncSettingsTest {
         // Master sync is disabled on startup, but the user has gone through the decoupling flow in
         // the past and has the DecoupleSyncFromAndroidMasterSync feature enabled.
         mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
-        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             when(mProfileSyncService.getDecoupledFromAndroidMasterSync()).thenReturn(true);
         });
@@ -463,7 +453,6 @@ public class AndroidSyncSettingsTest {
         // sync being disabled.
         createAndroidSyncSettings();
         Assert.assertTrue(getDoesMasterSyncAllowSyncOnUiThread());
-        Assert.assertTrue(getIsSyncEnabledOnUiThread());
     }
 
     @Test
@@ -474,15 +463,13 @@ public class AndroidSyncSettingsTest {
         // The user went through the master sync decoupling flow in the past, but has the
         // DecoupleSyncFromAndroidMasterSync feature disabled.
         mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
-        mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             when(mProfileSyncService.getDecoupledFromAndroidMasterSync()).thenReturn(true);
         });
 
         // Chrome should ignore that a previous decoupling happened. Sync should again respect
-        // master sync and stay disabled.
+        // master sync, so it's not allowed.
         createAndroidSyncSettings();
         Assert.assertFalse(getDoesMasterSyncAllowSyncOnUiThread());
-        Assert.assertFalse(getIsSyncEnabledOnUiThread());
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/sync/AndroidSyncSettingsTestUtils.java

@@ -26,11 +26,6 @@ public class AndroidSyncSettingsTestUtils {
                 () -> SyncContentResolverDelegate.overrideForTests(delegate));
     }
 
-    public static boolean getIsSyncEnabledOnUiThread() {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(
-                () -> AndroidSyncSettings.get().isSyncEnabled());
-    }
-
     public static boolean getIsChromeSyncEnabledOnUiThread() {
         return TestThreadUtils.runOnUiThreadBlockingNoException(
                 () -> AndroidSyncSettings.get().isChromeSyncEnabled());

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

@@ -155,7 +155,8 @@ public class SyncTest {
                 mSyncTestRule.setUpAccountAndEnableSyncForTesting());
         String authority = AndroidSyncSettings.getContractAuthority();
 
-        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
 
         // Disabling Android sync should turn Chrome sync engine off.
@@ -175,7 +176,8 @@ public class SyncTest {
     public void testStopAndStartSyncThroughAndroidMasterSync() {
         mSyncTestRule.setUpAccountAndEnableSyncForTesting();
 
-        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
 
         // Disabling Android's master sync should turn Chrome sync engine off.
@@ -197,7 +199,8 @@ public class SyncTest {
                 mSyncTestRule.setUpAccountAndEnableSyncForTesting());
         String authority = AndroidSyncSettings.getContractAuthority();
 
-        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
         Assert.assertTrue(SyncTestUtil.canSyncFeatureStart());
 
@@ -231,7 +234,8 @@ public class SyncTest {
                 mSyncTestRule.setUpAccountAndEnableSyncForTesting());
         String authority = AndroidSyncSettings.getContractAuthority();
 
-        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getIsChromeSyncEnabledOnUiThread());
+        Assert.assertTrue(AndroidSyncSettingsTestUtils.getDoesMasterSyncAllowSyncOnUiThread());
         Assert.assertTrue(SyncTestUtil.isSyncRequested());
         Assert.assertTrue(SyncTestUtil.canSyncFeatureStart());