Partial revert "Make IsSyncRequested false by default"
This reverts commit 8c872e02bbc75b817e036ed714ae652e3f638fa8.
Reason for revert:
https://bugs.chromium.org/p/chromium/issues/detail?id=973770
Note: This CL does not revert the changes the the test files as
instructed by treib@ who is the author of the original CL.
Original change's description:
> Make IsSyncRequested false by default
>
> Before this CL, the value of IsSyncRequested was essentially undefined
> while Sync was disabled: It was true by default, but false if the user
> had previously enabled Sync and then disabled it again.
> This CL resolves the inconsistency by making it false by default.
>
> Bug: 906034
> Change-Id: I2ce92f5df5ebb38a34ebde43d471839ad1184961
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1508453
> Reviewed-by: Jan Krcal <jkrcal@chromium.org>
> Commit-Queue: Marc Treib <treib@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#642447}
TBR=treib@chromium.org,jkrcal@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: 906034, 973770
Change-Id: I501f563cae8ff63de21cd66d933281e0af9542ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1658571
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Kariah Davis <kariahda@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#307}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc b/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
index 53524bb..860657af 100644
--- a/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
@@ -97,7 +97,9 @@
// either by the Sync confirmation dialog or by the settings page if going
// through the advanced settings flow.
EXPECT_FALSE(GetSyncService(0)->GetUserSettings()->IsFirstSetupComplete());
- EXPECT_FALSE(GetSyncService(0)->GetUserSettings()->IsSyncRequested());
+ // TODO(crbug.com/906034,crbug.com/973770): Sort out the proper default value
+ // for IsSyncRequested().
+ // EXPECT_FALSE(GetSyncService(0)->GetUserSettings()->IsSyncRequested());
EXPECT_FALSE(GetSyncService(0)->IsSyncFeatureEnabled());
EXPECT_FALSE(GetSyncService(0)->IsSyncFeatureActive());
diff --git a/chrome/browser/ui/webui/sync_internals_browsertest.js b/chrome/browser/ui/webui/sync_internals_browsertest.js
index ede5673..9a179ff 100644
--- a/chrome/browser/ui/webui/sync_internals_browsertest.js
+++ b/chrome/browser/ui/webui/sync_internals_browsertest.js
@@ -260,8 +260,9 @@
// set.
TEST_F('SyncInternalsWebUITest', 'SyncDisabledByDefault', function() {
expectTrue(this.hasInDetails(true, 'Transport State', 'Disabled'));
- expectTrue(
- this.hasInDetails(true, 'Disable Reasons', 'Not signed in, User choice'));
+ // TODO(crbug.com/906034,crbug.com/973770): Sort out the proper default value
+ // for IsSyncRequested() and possibly add the "User choice" disable reason.
+ expectTrue(this.hasInDetails(true, 'Disable Reasons', 'Not signed in'));
expectTrue(this.hasInDetails(true, 'Username', ''));
});
diff --git a/components/sync/base/sync_prefs.cc b/components/sync/base/sync_prefs.cc
index e0233e2..db4744b 100644
--- a/components/sync/base/sync_prefs.cc
+++ b/components/sync/base/sync_prefs.cc
@@ -148,7 +148,7 @@
user_prefs::PrefRegistrySyncable* registry) {
// Actual user-controlled preferences.
registry->RegisterBooleanPref(prefs::kSyncFirstSetupComplete, false);
- registry->RegisterBooleanPref(prefs::kSyncSuppressStart, true);
+ registry->RegisterBooleanPref(prefs::kSyncSuppressStart, false);
registry->RegisterBooleanPref(prefs::kSyncKeepEverythingSynced, true);
for (UserSelectableType type : UserSelectableTypeSet::All()) {
RegisterTypeSelectedPref(registry, type);
diff --git a/components/sync/base/sync_prefs_unittest.cc b/components/sync/base/sync_prefs_unittest.cc
index 737ec95..529c387 100644
--- a/components/sync/base/sync_prefs_unittest.cc
+++ b/components/sync/base/sync_prefs_unittest.cc
@@ -77,12 +77,12 @@
EXPECT_CALL(mock_sync_pref_observer, OnSyncManagedPrefChange(false));
EXPECT_CALL(mock_sync_pref_observer, OnFirstSetupCompletePrefChange(true));
EXPECT_CALL(mock_sync_pref_observer, OnFirstSetupCompletePrefChange(false));
- EXPECT_CALL(mock_sync_pref_observer, OnSyncRequestedPrefChange(true));
EXPECT_CALL(mock_sync_pref_observer, OnSyncRequestedPrefChange(false));
+ EXPECT_CALL(mock_sync_pref_observer, OnSyncRequestedPrefChange(true));
ASSERT_FALSE(sync_prefs_->IsManaged());
ASSERT_FALSE(sync_prefs_->IsFirstSetupComplete());
- ASSERT_FALSE(sync_prefs_->IsSyncRequested());
+ ASSERT_TRUE(sync_prefs_->IsSyncRequested());
sync_prefs_->AddSyncPrefObserver(&mock_sync_pref_observer);
@@ -98,10 +98,10 @@
sync_prefs_->ClearPreferences();
EXPECT_FALSE(sync_prefs_->IsFirstSetupComplete());
- sync_prefs_->SetSyncRequested(true);
- EXPECT_TRUE(sync_prefs_->IsSyncRequested());
sync_prefs_->SetSyncRequested(false);
EXPECT_FALSE(sync_prefs_->IsSyncRequested());
+ sync_prefs_->SetSyncRequested(true);
+ EXPECT_TRUE(sync_prefs_->IsSyncRequested());
sync_prefs_->RemoveSyncPrefObserver(&mock_sync_pref_observer);
}
@@ -135,11 +135,11 @@
sync_prefs_->SetFirstSetupComplete();
EXPECT_TRUE(sync_prefs_->IsFirstSetupComplete());
- EXPECT_FALSE(sync_prefs_->IsSyncRequested());
- sync_prefs_->SetSyncRequested(true);
EXPECT_TRUE(sync_prefs_->IsSyncRequested());
sync_prefs_->SetSyncRequested(false);
EXPECT_FALSE(sync_prefs_->IsSyncRequested());
+ sync_prefs_->SetSyncRequested(true);
+ EXPECT_TRUE(sync_prefs_->IsSyncRequested());
EXPECT_EQ(base::Time(), sync_prefs_->GetLastSyncedTime());
const base::Time& now = base::Time::Now();
diff --git a/components/sync/driver/profile_sync_service_startup_unittest.cc b/components/sync/driver/profile_sync_service_startup_unittest.cc
index a5ca326..86c6928 100644
--- a/components/sync/driver/profile_sync_service_startup_unittest.cc
+++ b/components/sync/driver/profile_sync_service_startup_unittest.cc
@@ -165,8 +165,7 @@
// Should not actually start, rather just clean things up and wait
// to be enabled.
sync_service()->Initialize();
- EXPECT_EQ(SyncService::DISABLE_REASON_NOT_SIGNED_IN |
- SyncService::DISABLE_REASON_USER_CHOICE,
+ EXPECT_EQ(SyncService::DISABLE_REASON_NOT_SIGNED_IN,
sync_service()->GetDisableReasons());
EXPECT_EQ(SyncService::TransportState::DISABLED,
sync_service()->GetTransportState());
@@ -604,8 +603,7 @@
// There is no signed-in user, so also nobody has decided that Sync should be
// started.
- EXPECT_EQ(SyncService::DISABLE_REASON_NOT_SIGNED_IN |
- SyncService::DISABLE_REASON_USER_CHOICE,
+ EXPECT_EQ(SyncService::DISABLE_REASON_NOT_SIGNED_IN,
sync_service()->GetDisableReasons());
EXPECT_EQ(SyncService::TransportState::DISABLED,
sync_service()->GetTransportState());
@@ -613,7 +611,7 @@
// Sign in. Now Sync-the-transport could start, but gets deferred by default.
// Sync-the-feature still doesn't start until the user says they want it.
SimulateTestUserSignin();
- EXPECT_EQ(SyncService::DISABLE_REASON_USER_CHOICE,
+ EXPECT_EQ(SyncService::DISABLE_REASON_NONE,
sync_service()->GetDisableReasons());
EXPECT_EQ(SyncService::TransportState::START_DEFERRED,
sync_service()->GetTransportState());