ProfileSyncService cleanup: Remove platform_sync_allowed_provider_
ProfileSyncService::platform_sync_allowed_provider_ was a callback that
returned whether Android's "Master Sync" toggle is enabled, and so Sync
shouldn't start up. However, PSS didn't know when the value changed, so
the callback was kinda useless, and PSS could easily get into an
inconsistent state (where there is a disable reason, but Sync is still
running). Things were working out in practice only because the Android
UI code also called RequestStart/Stop as necessary.
So, since PSS needs change notifications anyway, let's make that
explicit: This CL replaces the callback by a bool, which can be set
explicitly.
Bug: 867901
Change-Id: I5b7e9f4657d1f565d9643984eb52ec1df26b5c90
Reviewed-on: https://chromium-review.googlesource.com/c/1269870
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599195}diff --git a/chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java b/chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
index 16e6432..33daafb 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
@@ -153,10 +153,6 @@
return get().mNativeProfileSyncServiceAndroid;
}
- public void signOut() {
- nativeSignOutSync(mNativeProfileSyncServiceAndroid);
- }
-
/**
* Sets the the machine tag used by session sync.
*/
@@ -373,12 +369,6 @@
nativeSetFirstSetupComplete(mNativeProfileSyncServiceAndroid);
}
- // TODO(maxbogue): Remove when downstream is updated to use the above.
- @Deprecated
- public void setSyncSetupCompleted() {
- nativeSetFirstSetupComplete(mNativeProfileSyncServiceAndroid);
- }
-
public boolean isFirstSetupComplete() {
return nativeIsFirstSetupComplete(mNativeProfileSyncServiceAndroid);
}
@@ -455,6 +445,10 @@
nativeRequestStop(mNativeProfileSyncServiceAndroid);
}
+ public void setSyncAllowedByPlatform(boolean allowed) {
+ nativeSetSyncAllowedByPlatform(mNativeProfileSyncServiceAndroid, allowed);
+ }
+
/**
* Flushes the sync directory.
*/
@@ -556,8 +550,9 @@
private native long nativeInit();
private native void nativeRequestStart(long nativeProfileSyncServiceAndroid);
private native void nativeRequestStop(long nativeProfileSyncServiceAndroid);
+ private native void nativeSetSyncAllowedByPlatform(
+ long nativeProfileSyncServiceAndroid, boolean allowed);
private native void nativeFlushDirectory(long nativeProfileSyncServiceAndroid);
- private native void nativeSignOutSync(long nativeProfileSyncServiceAndroid);
private native void nativeSetSyncSessionsId(long nativeProfileSyncServiceAndroid, String tag);
private native int nativeGetAuthError(long nativeProfileSyncServiceAndroid);
private native int nativeGetProtocolErrorClientAction(long nativeProfileSyncServiceAndroid);
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java b/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
index b09e101..9e28c6f 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
@@ -146,6 +146,13 @@
* Updates sync to reflect the state of the Android sync settings.
*/
private void updateSyncStateFromAndroid() {
+ // Note: |isChromeSyncEnabled| maps to SyncRequested, and
+ // |isMasterSyncEnabled| maps to *both* SyncRequested and
+ // SyncAllowedByPlatform.
+ // TODO(crbug.com/867901): Don't mix these two concepts.
+
+ mProfileSyncService.setSyncAllowedByPlatform(AndroidSyncSettings.isMasterSyncEnabled());
+
boolean isSyncEnabled = AndroidSyncSettings.isSyncEnabled();
if (isSyncEnabled == mProfileSyncService.isSyncRequested()) return;
if (isSyncEnabled) {
diff --git a/chrome/browser/sync/profile_sync_service_android.cc b/chrome/browser/sync/profile_sync_service_android.cc
index e93f595e..653d86b 100644
--- a/chrome/browser/sync/profile_sync_service_android.cc
+++ b/chrome/browser/sync/profile_sync_service_android.cc
@@ -109,9 +109,6 @@
bool ProfileSyncServiceAndroid::Init() {
if (sync_service_) {
sync_service_->AddObserver(this);
- sync_service_->SetPlatformSyncAllowedProvider(
- base::Bind(&ProfileSyncServiceAndroid::IsSyncAllowedByAndroid,
- base::Unretained(this)));
return true;
} else {
return false;
@@ -121,8 +118,6 @@
ProfileSyncServiceAndroid::~ProfileSyncServiceAndroid() {
if (sync_service_) {
sync_service_->RemoveObserver(this);
- sync_service_->SetPlatformSyncAllowedProvider(
- ProfileSyncService::PlatformSyncAllowedProvider());
}
}
@@ -163,11 +158,11 @@
sync_service_->RequestStop(ProfileSyncService::KEEP_DATA);
}
-void ProfileSyncServiceAndroid::SignOutSync(JNIEnv* env,
- const JavaParamRef<jobject>&) {
- DCHECK_CURRENTLY_ON(BrowserThread::UI);
- DCHECK(profile_);
- sync_service_->RequestStop(ProfileSyncService::CLEAR_DATA);
+void ProfileSyncServiceAndroid::SetSyncAllowedByPlatform(
+ JNIEnv* env,
+ const JavaParamRef<jobject>& obj,
+ jboolean allowed) {
+ sync_service_->SetSyncAllowedByPlatform(allowed);
}
jboolean ProfileSyncServiceAndroid::IsSyncActive(
diff --git a/chrome/browser/sync/profile_sync_service_android.h b/chrome/browser/sync/profile_sync_service_android.h
index 4908b648..a6abe012 100644
--- a/chrome/browser/sync/profile_sync_service_android.h
+++ b/chrome/browser/sync/profile_sync_service_android.h
@@ -53,8 +53,9 @@
const base::android::JavaParamRef<jobject>& obj);
void RequestStop(JNIEnv* env,
const base::android::JavaParamRef<jobject>& obj);
- void SignOutSync(JNIEnv* env,
- const base::android::JavaParamRef<jobject>& obj);
+ void SetSyncAllowedByPlatform(JNIEnv* env,
+ const base::android::JavaParamRef<jobject>& obj,
+ jboolean allowed);
jboolean IsSyncActive(JNIEnv* env,
const base::android::JavaParamRef<jobject>& obj);
jboolean IsEngineInitialized(JNIEnv* env,
diff --git a/components/browser_sync/profile_sync_service.cc b/components/browser_sync/profile_sync_service.cc
index f837eaf..a3afd40 100644
--- a/components/browser_sync/profile_sync_service.cc
+++ b/components/browser_sync/profile_sync_service.cc
@@ -197,6 +197,7 @@
std::make_unique<syncer::HttpBridgeNetworkResources>()),
start_behavior_(init_params.start_behavior),
passphrase_prompt_triggered_by_version_(false),
+ sync_allowed_by_platform_(true),
sync_enabled_weak_factory_(this),
weak_factory_(this) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -1526,10 +1527,10 @@
return engine_ && engine_->IsCryptographerReady(trans);
}
-void ProfileSyncService::SetPlatformSyncAllowedProvider(
- const PlatformSyncAllowedProvider& platform_sync_allowed_provider) {
+void ProfileSyncService::SetSyncAllowedByPlatform(bool allowed) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- platform_sync_allowed_provider_ = platform_sync_allowed_provider;
+ sync_allowed_by_platform_ = allowed;
+ // TODO(crbug.com/867901): Start or stop Sync as needed here.
}
void ProfileSyncService::ConfigureDataTypeManager(
@@ -2023,8 +2024,7 @@
bool ProfileSyncService::IsSyncAllowedByPlatform() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- return platform_sync_allowed_provider_.is_null() ||
- platform_sync_allowed_provider_.Run();
+ return sync_allowed_by_platform_;
}
void ProfileSyncService::RequestStop(SyncStopDataFate data_fate) {
diff --git a/components/browser_sync/profile_sync_service.h b/components/browser_sync/profile_sync_service.h
index c49bd41..99062d5 100644
--- a/components/browser_sync/profile_sync_service.h
+++ b/components/browser_sync/profile_sync_service.h
@@ -160,7 +160,6 @@
public syncer::UnrecoverableErrorHandler,
public GaiaCookieManagerService::Observer {
public:
- using PlatformSyncAllowedProvider = base::RepeatingCallback<bool()>;
using SigninScopedDeviceIdCallback = base::RepeatingCallback<std::string()>;
// NOTE: Used in a UMA histogram, do not reorder etc.
@@ -456,9 +455,8 @@
// SyncPrefs. Will return the empty string if no bootstrap token exists.
std::string GetCustomPassphraseKey() const;
- // Set the provider for whether sync is currently allowed by the platform.
- void SetPlatformSyncAllowedProvider(
- const PlatformSyncAllowedProvider& platform_sync_allowed_provider);
+ // Set whether sync is currently allowed by the platform.
+ void SetSyncAllowedByPlatform(bool allowed);
// Sometimes we need to wait for tasks on the sync thread in tests.
base::MessageLoop* GetSyncLoopForTest() const;
@@ -754,9 +752,8 @@
// IsPassphrasePrompted sync preference.
bool passphrase_prompt_triggered_by_version_;
- // An object that lets us check whether sync is currently allowed on this
- // platform.
- PlatformSyncAllowedProvider platform_sync_allowed_provider_;
+ // Whether sync is currently allowed on this platform.
+ bool sync_allowed_by_platform_;
// This weak factory invalidates its issued pointers when Sync is disabled.
base::WeakPtrFactory<ProfileSyncService> sync_enabled_weak_factory_;
