Skip to content

GitLab

T
tangled
Commit 8d72971b authored by Victor Hugo Vianna Silva's avatar Victor Hugo Vianna Silva Committed by Commit Bot
Browse files
Options

Refactor: Reduce some complexity in AndroidSyncSettings and its tests 

AndroidSyncSettings exposes a few test methods that take a callback as a
parameter, so it is possible to wait for an account update operation to
finish. Before this CL, the callback was passed a boolean representing
whether syncability was changed.

This boolean return value was being used by a single test and can be
queried in other ways, so this CL converts the callback into a closure.
Apart from this, the CL also:
- Adds inline initialization for some members in the test fixture.
- Simplifies the waiting mechanism so that individual tests don't have
to refer to CallbackHelper directly.
- Replaces android.os.StrictMode with org.chromium.base.StrictModeContext
in AndroidSyncSettings, which uses a simpler try-with syntax and is
already used by some parts of the class.

Bug: None
Change-Id: Ie7909eba57e171569fc565a02f2ed8afc487e285
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410390Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#807453}
parent 9685f973
Show whitespace changes
Inline Side-by-side
Showing with 48 additions and 75 deletions
chrome/android/java/src/org/chromium/chrome/browser/sync/AndroidSyncSettings.java
View file @ 8d72971b
......@@ -9,13 +9,11 @@ import android.annotation.SuppressLint;
import android.content.ContentResolver;
import android.content.SyncStatusObserver;
import android.os.Bundle;
import android.os.StrictMode;
import androidx.annotation.MainThread;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import org.chromium.base.Callback;
import org.chromium.base.ContextUtils;
import org.chromium.base.ObserverList;
import org.chromium.base.StrictModeContext;
......@@ -41,8 +39,6 @@ import org.chromium.components.sync.SystemSyncContentResolverDelegate;
* {@link #updateAccount(Account)} should be invoked whenever sync account is changed.
*/
public class AndroidSyncSettings {
public static final String TAG = "AndroidSyncSettings";
@SuppressLint("StaticFieldLeak")
private static AndroidSyncSettings sInstance;
......@@ -103,13 +99,12 @@ public class AndroidSyncSettings {
/**
* @param syncContentResolverDelegate an implementation of {@link SyncContentResolverDelegate}.
* @param callback Callback that will be called after updating account is finished. Boolean
* passed to the callback indicates whether syncability was changed.
* @param callback Callback that will be called after updating account is finished.
* @param account The sync account if sync is enabled, null otherwise.
*/
@VisibleForTesting
public AndroidSyncSettings(SyncContentResolverDelegate syncContentResolverDelegate,
@Nullable Callback<Boolean> callback, @Nullable Account account) {
@Nullable Runnable callback, @Nullable Account account) {
mContractAuthority = getContractAuthority();
mSyncContentResolverDelegate = syncContentResolverDelegate;
......@@ -189,11 +184,10 @@ public class AndroidSyncSettings {
/**
* Must be called when a new account is signed in.
* @param callback Callback that will be called after updating account is finished. Boolean
* passed to the callback indicates whether syncability was changed.
* @param callback Callback that will be called after updating account is finished.
*/
@VisibleForTesting
public void updateAccount(Account account, @Nullable Callback<Boolean> callback) {
public void updateAccount(Account account, @Nullable Runnable callback) {
synchronized (mLock) {
mAccount = account;
updateSyncability(callback);
......@@ -237,9 +231,10 @@ public class AndroidSyncSettings {
if (value == mChromeSyncEnabled || mAccount == null) return;
mChromeSyncEnabled = value;
StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mContractAuthority, value);
StrictMode.setThreadPolicy(oldPolicy);
try (StrictModeContext ignored = StrictModeContext.allowDiskWrites()) {
mSyncContentResolverDelegate.setSyncAutomatically(
mAccount, mContractAuthority, value);
}
}
notifyObservers();
}
......@@ -250,12 +245,12 @@ public class AndroidSyncSettings {
* This is what causes the "Chrome" option to appear in Settings -> Accounts -> Sync .
* This function must be called within a synchronized block.
*/
private void updateSyncability(@Nullable final Callback<Boolean> callback) {
private void updateSyncability(@Nullable final Runnable callback) {
boolean shouldBeSyncable = mAccount != null
&& !ChromeFeatureList.isEnabled(
ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC);
if (mIsSyncable == shouldBeSyncable) {
if (callback != null) callback.onResult(false);
if (callback != null) callback.run();
return;
}
......@@ -291,7 +286,7 @@ public class AndroidSyncSettings {
}
}
if (callback != null) callback.onResult(true);
if (callback != null) callback.run();
});
});
}
......@@ -306,7 +301,7 @@ public class AndroidSyncSettings {
boolean oldChromeSyncEnabled = mChromeSyncEnabled;
boolean oldMasterSyncEnabled = mMasterSyncEnabled;
StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
try (StrictModeContext ignored = StrictModeContext.allowDiskWrites()) {
if (mAccount != null) {
mIsSyncable =
mSyncContentResolverDelegate.getIsSyncable(mAccount, mContractAuthority)
......@@ -318,7 +313,7 @@ public class AndroidSyncSettings {
mChromeSyncEnabled = false;
}
mMasterSyncEnabled = mSyncContentResolverDelegate.getMasterSyncAutomatically();
StrictMode.setThreadPolicy(oldPolicy);
}
return oldChromeSyncEnabled != mChromeSyncEnabled
|| oldMasterSyncEnabled != mMasterSyncEnabled;
......
chrome/android/javatests/src/org/chromium/chrome/browser/sync/AndroidSyncSettingsTest.java
View file @ 8d72971b
......@@ -17,7 +17,6 @@ import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.chromium.base.Callback;
import org.chromium.base.FeatureList;
import org.chromium.base.ThreadUtils;
import org.chromium.base.test.BaseJUnit4ClassRunner;
......@@ -112,12 +111,11 @@ public class AndroidSyncSettingsTest {
private AndroidSyncSettings mAndroidSyncSettings;
private CountingMockSyncContentResolverDelegate mSyncContentResolverDelegate;
private String mAuthority;
private Account mAccount;
private Account mAlternateAccount;
private MockSyncSettingsObserver mSyncSettingsObserver;
private CallbackHelper mCallbackHelper;
private int mNumberOfCallsToWait;
private String mAuthority = AndroidSyncSettings.getContractAuthority();
@Before
public void setUp() throws Exception {
......@@ -125,19 +123,22 @@ public class AndroidSyncSettingsTest {
mNumberOfCallsToWait = 0;
mCallbackHelper = new CallbackHelper();
setupTestAccounts();
mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate();
mAuthority = AndroidSyncSettings.getContractAuthority();
FakeAccountManagerFacade fakeAccountManagerFacade = new FakeAccountManagerFacade(null);
AccountManagerFacadeProvider.setInstanceForTests(fakeAccountManagerFacade);
mAccount = AccountUtils.createAccountFromName("account@example.com");
fakeAccountManagerFacade.addAccount(mAccount);
mAlternateAccount = AccountUtils.createAccountFromName("alternate@example.com");
fakeAccountManagerFacade.addAccount(mAlternateAccount);
}
private void createAndroidSyncSettings() throws TimeoutException {
TestThreadUtils.runOnUiThreadBlocking(() -> {
mAndroidSyncSettings = new AndroidSyncSettings(mSyncContentResolverDelegate,
(Boolean result) -> mCallbackHelper.notifyCalled(), mAccount);
mAndroidSyncSettings = new AndroidSyncSettings(
mSyncContentResolverDelegate, () -> mCallbackHelper.notifyCalled(), mAccount);
});
mNumberOfCallsToWait++;
mCallbackHelper.waitForCallback(0, mNumberOfCallsToWait);
mCallbackHelper.waitForCallback(0, ++mNumberOfCallsToWait);
if (ChromeFeatureList.isEnabled(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
Assert.assertTrue(
......@@ -147,15 +148,6 @@ public class AndroidSyncSettingsTest {
}
}
private void setupTestAccounts() {
FakeAccountManagerFacade fakeAccountManagerFacade = new FakeAccountManagerFacade(null);
AccountManagerFacadeProvider.setInstanceForTests(fakeAccountManagerFacade);
mAccount = AccountUtils.createAccountFromName("account@example.com");
fakeAccountManagerFacade.addAccount(mAccount);
mAlternateAccount = AccountUtils.createAccountFromName("alternate@example.com");
fakeAccountManagerFacade.addAccount(mAlternateAccount);
}
private void setMasterSyncAllowsChromeSync() throws InterruptedException {
if (!ChromeFeatureList.isEnabled(
ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
......@@ -190,20 +182,9 @@ public class AndroidSyncSettingsTest {
});
}
private void updateAccountSync(Account account) throws TimeoutException {
updateAccount(account);
mCallbackHelper.waitForCallback(0, mNumberOfCallsToWait);
}
private void updateAccount(Account account) {
updateAccountWithCallback(account, (Boolean result) -> {
mCallbackHelper.notifyCalled();
});
}
private void updateAccountWithCallback(Account account, Callback<Boolean> callback) {
mAndroidSyncSettings.updateAccount(account, callback);
mNumberOfCallsToWait++;
private void updateAccountAndWait(Account account) throws TimeoutException {
mAndroidSyncSettings.updateAccount(account, () -> mCallbackHelper.notifyCalled());
mCallbackHelper.waitForCallback(0, ++mNumberOfCallsToWait);
}
@Test
......@@ -216,12 +197,12 @@ public class AndroidSyncSettingsTest {
Assert.assertEquals(1, mSyncContentResolverDelegate.mSetIsSyncableCalls.get());
Assert.assertEquals(1, mSyncContentResolverDelegate.mRemovePeriodicSyncCalls.get());
updateAccountSync(null);
updateAccountAndWait(null);
// mAccount was set to be not syncable.
Assert.assertEquals(2, mSyncContentResolverDelegate.mSetIsSyncableCalls.get());
Assert.assertEquals(1, mSyncContentResolverDelegate.mRemovePeriodicSyncCalls.get());
updateAccount(mAlternateAccount);
updateAccountAndWait(mAlternateAccount);
// mAlternateAccount was set to be syncable and not have periodic syncs.
Assert.assertEquals(3, mSyncContentResolverDelegate.mSetIsSyncableCalls.get());
Assert.assertEquals(2, mSyncContentResolverDelegate.mRemovePeriodicSyncCalls.get());
......@@ -333,7 +314,7 @@ public class AndroidSyncSettingsTest {
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
Assert.assertTrue("account should be synced", mAndroidSyncSettings.isSyncEnabled());
updateAccount(mAlternateAccount);
updateAccountAndWait(mAlternateAccount);
enableChromeSyncOnUiThread();
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
Assert.assertTrue(
......@@ -343,11 +324,11 @@ public class AndroidSyncSettingsTest {
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
Assert.assertFalse(
"alternate account should not be synced", mAndroidSyncSettings.isSyncEnabled());
updateAccount(mAccount);
updateAccountAndWait(mAccount);
Assert.assertTrue("account should still be synced", mAndroidSyncSettings.isSyncEnabled());
// Ensure we don't erroneously re-use cached data.
updateAccount(null);
updateAccountAndWait(null);
Assert.assertFalse(
"null account should not be synced", mAndroidSyncSettings.isSyncEnabled());
}
......@@ -384,7 +365,7 @@ public class AndroidSyncSettingsTest {
mSyncContentResolverDelegate.mGetSyncAutomaticallyCalls.get());
// Do a bunch of reads for alternate account.
updateAccount(mAlternateAccount);
updateAccountAndWait(mAlternateAccount);
mAndroidSyncSettings.doesMasterSyncSettingAllowChromeSync();
mAndroidSyncSettings.isSyncEnabled();
mAndroidSyncSettings.isChromeSyncEnabled();
......@@ -414,12 +395,12 @@ public class AndroidSyncSettingsTest {
syncSettingsObserver.receivedNotification());
syncSettingsObserver.clearNotification();
updateAccount(mAlternateAccount);
updateAccountAndWait(mAlternateAccount);
Assert.assertTrue("switching to account with different settings should notify",
syncSettingsObserver.receivedNotification());
syncSettingsObserver.clearNotification();
updateAccount(mAccount);
updateAccountAndWait(mAccount);
Assert.assertTrue("switching to account with different settings should notify",
syncSettingsObserver.receivedNotification());
......@@ -448,14 +429,11 @@ public class AndroidSyncSettingsTest {
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
updateAccountWithCallback(null, (Boolean result) -> {
Assert.assertTrue(result);
mCallbackHelper.notifyCalled();
});
mCallbackHelper.waitForCallback(0, mNumberOfCallsToWait);
updateAccountAndWait(null);
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) <= 0);
Assert.assertEquals(0, mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority));
updateAccount(mAccount);
updateAccountAndWait(mAccount);
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
}
......
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