Commit 53c5a84a authored by Victor Hugo Vianna Silva's avatar Victor Hugo Vianna Silva Committed by Commit Bot

Remove Chrome from Android's list of auto-sync apps

After this CL, when DecoupleSyncFromAndroidMasterSync is enabled, Chrome
will no longer be in the list of auto-sync apps. In particular this toggle
will disappear [1]. Users who previously disabled that toggle will remain
with Chrome Sync disabled until they enable it again on Chrome settings.
[1] https://screenshot.googleplex.com/9uR0qxTzVzK

Context: the Android APIs setIsSyncable(..., int) and
setSyncAutomatically(..., bool) control 2 *independent* settings per
(account, app) pair, "syncable"[2] and "enabled"[3], persisted to an XML
file. The first one controls whether the toggle in the screenshot will
exist. A value >0 means yes, =0 means no and <0 means an unknown state
(where the toggle isn't shown either)[4]. What the CL does is basically
set "syncable" to 0 for any Chrome account. "enabled" can continue to be
set safely and will still mirror the sync.requested pref [5], as happens
today.
[2] https://cs.android.com/android/platform/superproject/+/eed78aea1f49cba01c737220a6f834bca7feeef7:frameworks/base/services/core/java/com/android/server/content/SyncStorageEngine.java;l=283
[3] https://cs.android.com/android/platform/superproject/+/eed78aea1f49cba01c737220a6f834bca7feeef7:frameworks/base/services/core/java/com/android/server/content/SyncStorageEngine.java;l=282
[4] https://developer.android.com/reference/android/content/ContentResolver#setIsSyncable(android.accounts.Account,%20java.lang.String,%20int)
[5] https://source.chromium.org/chromium/chromium/src/+/master:components/sync/base/sync_prefs.h;l=90

The CL also:
a) Replaces code that checked specifically whether "syncable" valued 1,
0 or -1 with more generic checks for >0, =0 or <0. 1, 0 and -1 do have
special meanings in SyncStorageEngine, but that's an implementation
detail of a lower layer. The layer where the APIs live (ContentResolver)
never ensures those values.
b) Modifies the implementation of MockSyncContentResolverDelegate to be
more consistent with the real Android behavior. E.g. one should be
allowed to set "enabled" before "syncable".

In next CLs the DecoupleSyncFromAndroidMasterSync feature toggle will be
renamed to mention auto-sync rather than master sync.

Bug: 1105795
Change-Id: I091981506ff64c33a6c259862eee07d28825527f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2366895
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800273}
parent eb0227e2
...@@ -109,8 +109,8 @@ public class AndroidSyncSettings { ...@@ -109,8 +109,8 @@ public class AndroidSyncSettings {
mSyncContentResolverDelegate = syncContentResolverDelegate; mSyncContentResolverDelegate = syncContentResolverDelegate;
mAccount = account; mAccount = account;
updateSyncability(callback);
updateCachedSettings(); updateCachedSettings();
updateSyncability(callback);
mSyncContentResolverDelegate.addStatusChangeListener( mSyncContentResolverDelegate.addStatusChangeListener(
ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS, ContentResolver.SYNC_OBSERVER_TYPE_SETTINGS,
...@@ -234,7 +234,9 @@ public class AndroidSyncSettings { ...@@ -234,7 +234,9 @@ public class AndroidSyncSettings {
* This function must be called within a synchronized block. * This function must be called within a synchronized block.
*/ */
private void updateSyncability(@Nullable final Callback<Boolean> callback) { private void updateSyncability(@Nullable final Callback<Boolean> callback) {
boolean shouldBeSyncable = mAccount != null; boolean shouldBeSyncable = mAccount != null
&& !ChromeFeatureList.isEnabled(
ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC);
if (mIsSyncable == shouldBeSyncable) { if (mIsSyncable == shouldBeSyncable) {
if (callback != null) callback.onResult(false); if (callback != null) callback.onResult(false);
return; return;
...@@ -249,6 +251,8 @@ public class AndroidSyncSettings { ...@@ -249,6 +251,8 @@ public class AndroidSyncSettings {
// This reduces unnecessary resource usage. See http://crbug.com/480688 for details. // This reduces unnecessary resource usage. See http://crbug.com/480688 for details.
mSyncContentResolverDelegate.removePeriodicSync( mSyncContentResolverDelegate.removePeriodicSync(
mAccount, mContractAuthority, Bundle.EMPTY); mAccount, mContractAuthority, Bundle.EMPTY);
} else if (mAccount != null) {
mSyncContentResolverDelegate.setIsSyncable(mAccount, mContractAuthority, 0);
} }
} }
...@@ -307,7 +311,7 @@ public class AndroidSyncSettings { ...@@ -307,7 +311,7 @@ public class AndroidSyncSettings {
if (mAccount != null) { if (mAccount != null) {
mIsSyncable = mIsSyncable =
mSyncContentResolverDelegate.getIsSyncable(mAccount, mContractAuthority) mSyncContentResolverDelegate.getIsSyncable(mAccount, mContractAuthority)
== 1; > 0;
mChromeSyncEnabled = mSyncContentResolverDelegate.getSyncAutomatically( mChromeSyncEnabled = mSyncContentResolverDelegate.getSyncAutomatically(
mAccount, mContractAuthority); mAccount, mContractAuthority);
} else { } else {
......
...@@ -137,7 +137,12 @@ public class AndroidSyncSettingsTest { ...@@ -137,7 +137,12 @@ public class AndroidSyncSettingsTest {
mCallbackHelper.waitForCallback(0, mNumberOfCallsToWait); mCallbackHelper.waitForCallback(0, mNumberOfCallsToWait);
mAuthority = mAndroidSyncSettings.getContractAuthority(); mAuthority = mAndroidSyncSettings.getContractAuthority();
Assert.assertEquals(1, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority)); if (ChromeFeatureList.isEnabled(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
Assert.assertTrue(
mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) <= 0);
} else {
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
}
mSyncSettingsObserver = new MockSyncSettingsObserver(); mSyncSettingsObserver = new MockSyncSettingsObserver();
mAndroidSyncSettings.registerObserver(mSyncSettingsObserver); mAndroidSyncSettings.registerObserver(mSyncSettingsObserver);
...@@ -429,8 +434,9 @@ public class AndroidSyncSettingsTest { ...@@ -429,8 +434,9 @@ public class AndroidSyncSettingsTest {
@Test @Test
@SmallTest @SmallTest
@Feature({"Sync"}) @Feature({"Sync"})
@Features.DisableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
public void testIsSyncableOnSigninAndNotOnSignout() throws TimeoutException { public void testIsSyncableOnSigninAndNotOnSignout() throws TimeoutException {
Assert.assertEquals(1, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority)); Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
updateAccountWithCallback(null, (Boolean result) -> { updateAccountWithCallback(null, (Boolean result) -> {
Assert.assertTrue(result); Assert.assertTrue(result);
...@@ -440,7 +446,7 @@ public class AndroidSyncSettingsTest { ...@@ -440,7 +446,7 @@ public class AndroidSyncSettingsTest {
Assert.assertEquals(0, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority)); Assert.assertEquals(0, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority));
updateAccount(mAccount); updateAccount(mAccount);
Assert.assertEquals(1, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority)); Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
} }
/** /**
...@@ -449,6 +455,7 @@ public class AndroidSyncSettingsTest { ...@@ -449,6 +455,7 @@ public class AndroidSyncSettingsTest {
@Test @Test
@SmallTest @SmallTest
@Feature({"Sync"}) @Feature({"Sync"})
@Features.DisableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
public void testSyncableIsAlwaysSetWhenEnablingSync() throws InterruptedException { public void testSyncableIsAlwaysSetWhenEnablingSync() throws InterruptedException {
// Setup bad state. // Setup bad state.
mSyncContentResolverDelegate.setMasterSyncAutomatically(true); mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
...@@ -459,12 +466,12 @@ public class AndroidSyncSettingsTest { ...@@ -459,12 +466,12 @@ public class AndroidSyncSettingsTest {
mSyncContentResolverDelegate.waitForLastNotificationCompleted(); mSyncContentResolverDelegate.waitForLastNotificationCompleted();
mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, 0); mSyncContentResolverDelegate.setIsSyncable(mAccount, mAuthority, 0);
mSyncContentResolverDelegate.waitForLastNotificationCompleted(); mSyncContentResolverDelegate.waitForLastNotificationCompleted();
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) == 0); Assert.assertEquals(0, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority));
Assert.assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority)); Assert.assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority));
// Ensure bug is fixed. // Ensure bug is fixed.
enableChromeSyncOnUiThread(); enableChromeSyncOnUiThread();
Assert.assertEquals(1, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority)); Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
// Should still be enabled. // Should still be enabled.
Assert.assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority)); Assert.assertTrue(mSyncContentResolverDelegate.getSyncAutomatically(mAccount, mAuthority));
} }
......
...@@ -25,10 +25,12 @@ import java.util.concurrent.Semaphore; ...@@ -25,10 +25,12 @@ import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
/** /**
* Mock implementation of the {@link SyncContentResolverDelegate}. * Fake implementation of the {@link SyncContentResolverDelegate}.
* *
* This implementation only supports status change listeners for the type * This implementation only supports status change listeners for the type
* SYNC_OBSERVER_TYPE_SETTINGS. * SYNC_OBSERVER_TYPE_SETTINGS. Differently from the real implementation, it
* doesn't allow calling the auto-sync methods (e.g. getIsSyncable()) with a null
* account.
*/ */
public class MockSyncContentResolverDelegate implements SyncContentResolverDelegate { public class MockSyncContentResolverDelegate implements SyncContentResolverDelegate {
private final Set<String> mSyncAutomaticallySet; private final Set<String> mSyncAutomaticallySet;
...@@ -84,21 +86,15 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg ...@@ -84,21 +86,15 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
@Override @Override
public boolean getSyncAutomatically(Account account, String authority) { public boolean getSyncAutomatically(Account account, String authority) {
String key = createKey(account, authority);
synchronized (mSyncableMapLock) { synchronized (mSyncableMapLock) {
return mSyncAutomaticallySet.contains(key); return mSyncAutomaticallySet.contains(createKey(account, authority));
} }
} }
@Override @Override
public void setSyncAutomatically(Account account, String authority, boolean sync) { public void setSyncAutomatically(Account account, String authority, boolean sync) {
String key = createKey(account, authority);
synchronized (mSyncableMapLock) { synchronized (mSyncableMapLock) {
if (!mIsSyncableMap.containsKey(key) || !mIsSyncableMap.get(key)) { String key = createKey(account, authority);
throw new IllegalArgumentException("Account " + account
+ " is not syncable for authority " + authority
+ ". Can not set sync state to " + sync);
}
if (sync) { if (sync) {
mSyncAutomaticallySet.add(key); mSyncAutomaticallySet.add(key);
} else if (mSyncAutomaticallySet.contains(key)) { } else if (mSyncAutomaticallySet.contains(key)) {
...@@ -110,24 +106,14 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg ...@@ -110,24 +106,14 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
@Override @Override
public void setIsSyncable(Account account, String authority, int syncable) { public void setIsSyncable(Account account, String authority, int syncable) {
String key = createKey(account, authority);
synchronized (mSyncableMapLock) { synchronized (mSyncableMapLock) {
switch (syncable) { String key = createKey(account, authority);
case 0: if (syncable > 0) {
mIsSyncableMap.put(key, false); mIsSyncableMap.put(key, true);
break; } else if (syncable == 0) {
case 1: mIsSyncableMap.put(key, false);
mIsSyncableMap.put(key, true); } else if (mIsSyncableMap.containsKey(key)) {
break; mIsSyncableMap.remove(key);
case -1:
if (mIsSyncableMap.containsKey(key)) {
mIsSyncableMap.remove(key);
}
break;
default:
throw new IllegalArgumentException(
"Unable to understand syncable argument: " + syncable);
} }
} }
notifyObservers(); notifyObservers();
...@@ -135,13 +121,12 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg ...@@ -135,13 +121,12 @@ public class MockSyncContentResolverDelegate implements SyncContentResolverDeleg
@Override @Override
public int getIsSyncable(Account account, String authority) { public int getIsSyncable(Account account, String authority) {
String key = createKey(account, authority);
synchronized (mSyncableMapLock) { synchronized (mSyncableMapLock) {
String key = createKey(account, authority);
if (mIsSyncableMap.containsKey(key)) { if (mIsSyncableMap.containsKey(key)) {
return mIsSyncableMap.get(key) ? 1 : 0; return mIsSyncableMap.get(key) ? 1 : 0;
} else {
return -1;
} }
return -1;
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment