Fix prefs registration in SyncPrefs.

All preferences should be registered _before_ a PrefService is created,
so reading other prefs during registration is a no-no.

The previous code dynamically set a default for the
kSyncKeepEverythingSynced preference and for each data type preference
dynamically based on whether or not sync setup is complete, but during
sync setup each of these would be explicitly set anyway, overriding
the default.

The one complexity is that there is code in
ProfileSyncService::TrySyncDatatypePrefRecovery that attempts to
recover from an old bug, and it depends on checking whether the value
is default. We remove one of the checks it used, which was redundant
(the needed check is just whether the pref is still at its default
value, regardless of that default value).

TBR=ben@chromium.org
BUG=155525


Review URL: https://chromiumcodereview.appspot.com/12315053

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@184800 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 11 insertions, 28 deletions

diff --git a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
index d7dd7fd..ffa7bca 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
@@ -139,14 +139,12 @@ TEST_F(PrefProviderTest, Incognito) {
   scoped_refptr<PrefRegistrySyncable> registry(new PrefRegistrySyncable);
   PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
 
-  Profile::RegisterUserPrefs(registry);
   chrome::RegisterUserPrefs(regular_prefs, registry);
 
   builder.WithUserPrefs(otr_user_prefs);
   scoped_refptr<PrefRegistrySyncable> otr_registry(new PrefRegistrySyncable);
   PrefServiceSyncable* otr_prefs = builder.CreateSyncable(otr_registry);
 
-  Profile::RegisterUserPrefs(otr_registry);
   chrome::RegisterUserPrefs(otr_prefs, otr_registry);
 
   TestingProfile::Builder profile_builder;
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 494facb9..f096ffe 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -448,7 +448,6 @@ void ExtensionServiceTestBase::InitializeExtensionService(
       pref_file, loop_.message_loop_proxy());
   scoped_refptr<PrefRegistrySyncable> registry(new PrefRegistrySyncable);
   scoped_ptr<PrefServiceSyncable> prefs(builder.CreateSyncable(registry));
-  Profile::RegisterUserPrefs(registry);
   chrome::RegisterUserPrefs(prefs.get(), registry);
   profile_builder.SetPrefService(prefs.Pass());
   profile_builder.SetPath(profile_path);
diff --git a/chrome/browser/policy/user_policy_signin_service_unittest.cc b/chrome/browser/policy/user_policy_signin_service_unittest.cc
index aaa674c..0fc0510 100644
--- a/chrome/browser/policy/user_policy_signin_service_unittest.cc
+++ b/chrome/browser/policy/user_policy_signin_service_unittest.cc
@@ -99,7 +99,6 @@ class UserPolicySigninServiceTest : public testing::Test {
     // up a UserCloudPolicyManager with a MockUserCloudPolicyStore.
     scoped_ptr<TestingPrefServiceSyncable> prefs(
         new TestingPrefServiceSyncable());
-    Profile::RegisterUserPrefs(prefs->registry());
     chrome::RegisterUserPrefs(prefs.get(), prefs->registry());
     TestingProfile::Builder builder;
     builder.SetPrefService(scoped_ptr<PrefServiceSyncable>(prefs.Pass()));
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index e671e31..dac9cce 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -58,6 +58,7 @@
 #include "chrome/browser/printing/cloud_print/cloud_print_url.h"
 #include "chrome/browser/printing/print_dialog_cloud.h"
 #include "chrome/browser/profiles/chrome_version_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_impl.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -269,7 +270,7 @@ void RegisterUserPrefs(PrefService* user_prefs,
   BookmarkPromptPrefs::RegisterUserPrefs(registry);
   bookmark_utils::RegisterUserPrefs(registry);
   BrowserInstantController::RegisterUserPrefs(user_prefs, registry);
-  browser_sync::SyncPrefs::RegisterUserPrefs(user_prefs, registry);
+  browser_sync::SyncPrefs::RegisterUserPrefs(registry);
   ChromeContentBrowserClient::RegisterUserPrefs(registry);
   ChromeDownloadManagerDelegate::RegisterUserPrefs(registry);
   ChromeVersionService::RegisterUserPrefs(registry);
@@ -292,6 +293,7 @@ void RegisterUserPrefs(PrefService* user_prefs,
   PasswordManager::RegisterUserPrefs(registry);
   PrefProxyConfigTrackerImpl::RegisterUserPrefs(registry);
   PrefsTabHelper::RegisterUserPrefs(registry);
+  Profile::RegisterUserPrefs(registry);
   ProfileImpl::RegisterUserPrefs(registry);
   PromoResourceService::RegisterUserPrefs(registry);
   ProtocolHandlerRegistry::RegisterUserPrefs(registry);
diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
index b41fea5..e23498d 100644
--- a/chrome/browser/profiles/profile_impl.cc
+++ b/chrome/browser/profiles/profile_impl.cc
@@ -388,8 +388,6 @@ ProfileImpl::ProfileImpl(
          create_mode == CREATE_MODE_SYNCHRONOUS);
   bool async_prefs = create_mode == CREATE_MODE_ASYNCHRONOUS;
 
-  Profile::RegisterUserPrefs(pref_registry_);
-
   {
     // On startup, preference loading is always synchronous so a scoped timer
     // will work here.
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index ee4690a..5c17fb0 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -250,8 +250,6 @@ void ProfileSyncService::TrySyncDatatypePrefRecovery() {
   PrefService* const pref_service = profile_->GetPrefs();
   if (!pref_service)
     return;
-  if (sync_prefs_.HasKeepEverythingSynced())
-    return;
   const syncer::ModelTypeSet registered_types = GetRegisteredDataTypes();
   if (sync_prefs_.GetPreferredDataTypes(registered_types).Size() > 1)
     return;
diff --git a/chrome/browser/sync/sync_prefs.cc b/chrome/browser/sync/sync_prefs.cc
index 4a9f430..3d9be89 100644
--- a/chrome/browser/sync/sync_prefs.cc
+++ b/chrome/browser/sync/sync_prefs.cc
@@ -43,8 +43,7 @@ SyncPrefs::~SyncPrefs() {
 }
 
 // static
-void SyncPrefs::RegisterUserPrefs(PrefService* prefs,
-                                  PrefRegistrySyncable* registry) {
+void SyncPrefs::RegisterUserPrefs(PrefRegistrySyncable* registry) {
   // TODO(joi): Remove |prefs| parameter.
   registry->RegisterBooleanPref(prefs::kSyncHasSetupCompleted,
                                 false,
@@ -56,18 +55,11 @@ void SyncPrefs::RegisterUserPrefs(PrefService* prefs,
                               0,
                               PrefRegistrySyncable::UNSYNCABLE_PREF);
 
-  // If you've never synced before, or if you're using Chrome OS or Android,
-  // all datatypes are on by default.
-  // TODO(nick): Perhaps a better model would be to always default to false,
-  // and explicitly call SetDataTypes() when the user shows the wizard.
-#if defined(OS_CHROMEOS) || defined(OS_ANDROID)
-  bool enable_by_default = true;
-#else
-  bool enable_by_default = !prefs->HasPrefPath(prefs::kSyncHasSetupCompleted);
-#endif
-
+  // All datatypes are on by default, but this gets set explicitly
+  // when you configure sync (when turning it on), in
+  // ProfileSyncService::OnUserChoseDatatypes.
   registry->RegisterBooleanPref(prefs::kSyncKeepEverythingSynced,
-                                enable_by_default,
+                                true,
                                 PrefRegistrySyncable::UNSYNCABLE_PREF);
 
   syncer::ModelTypeSet user_types = syncer::UserTypes();
@@ -82,7 +74,7 @@ void SyncPrefs::RegisterUserPrefs(PrefService* prefs,
 
   for (syncer::ModelTypeSet::Iterator it = user_types.First();
        it.Good(); it.Inc()) {
-    RegisterDataTypePreferredPref(registry, it.Get(), enable_by_default);
+    RegisterDataTypePreferredPref(registry, it.Get(), true);
   }
 
   registry->RegisterBooleanPref(prefs::kSyncManaged,
diff --git a/chrome/browser/sync/sync_prefs.h b/chrome/browser/sync/sync_prefs.h
index b0b2fb0..21b0c13 100644
--- a/chrome/browser/sync/sync_prefs.h
+++ b/chrome/browser/sync/sync_prefs.h
@@ -54,8 +54,7 @@ class SyncPrefs : NON_EXPORTED_BASE(public base::NonThreadSafe),
 
   virtual ~SyncPrefs();
 
-  static void RegisterUserPrefs(PrefService* prefs,
-                                PrefRegistrySyncable* registry);
+  static void RegisterUserPrefs(PrefRegistrySyncable* registry);
 
   // Checks if sync is enabled for the profile that owns |io_data|. This must
   // be invoked on the IO thread, and can be used to check if sync is enabled
diff --git a/chrome/browser/sync/sync_prefs_unittest.cc b/chrome/browser/sync/sync_prefs_unittest.cc
index ea114f2..aa19c1a 100644
--- a/chrome/browser/sync/sync_prefs_unittest.cc
+++ b/chrome/browser/sync/sync_prefs_unittest.cc
@@ -21,8 +21,7 @@ using ::testing::StrictMock;
 class SyncPrefsTest : public testing::Test {
  protected:
   virtual void SetUp() OVERRIDE {
-    SyncPrefs::RegisterUserPrefs(&pref_service_,
-                                 pref_service_.registry());
+    SyncPrefs::RegisterUserPrefs(pref_service_.registry());
   }
 
   TestingPrefServiceSyncable pref_service_;
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index e59c63c..970d629 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -556,7 +556,6 @@ void TestingProfile::CreateTestingPrefService() {
   DCHECK(!prefs_.get());
   testing_prefs_ = new TestingPrefServiceSyncable();
   prefs_.reset(testing_prefs_);
-  Profile::RegisterUserPrefs(testing_prefs_->registry());
   chrome::RegisterUserPrefs(testing_prefs_, testing_prefs_->registry());
 }