[Smart Lock, Prefs reconciliation] Prefs migration logic for desktop platforms.

chrome/browser/password_manager/password_manager_setting_migrater_service.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/password_manager/password_manager_setting_migrater_service.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/sync_driver/sync_service.h"
+#include "content/public/browser/notification_source.h"
+
+namespace {
+void ChangeOnePrefBecauseAnotherPrefHasChanged(
+    Profile* profile,
+    const std::string& other_pref_name,
+    const std::string& changed_pref_name) {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile);

[vabr (Chromium), 2015/08/12 08:25:50]

While PrefServiceSyncable cannot be removed from this code completely, there is
still value in not letting the //chrome dependencies take too deep roots in the
code. In particular here, instead of passing Profile as an argument, and
requesting PrefServiceSyncable, I suggest passing sync_driver::SyncService* as
the argument instead of Profile. The SyncService interface should be enough, and
while you still have to get the same object from the profile you get now, it
will happen lower in the call stack, and spread //chrome dependencies over less
code.

[melandory, 2015/08/13 15:49:15]

Done.

+  bool changed_pref = prefs->GetBoolean(changed_pref_name.c_str());
+  bool other_pref = prefs->GetBoolean(other_pref_name.c_str());
+  if (changed_pref != other_pref)
+    prefs->SetBoolean(other_pref_name, changed_pref);
+  // TODO(melandory): add histograms in order to track when we can stop
+  // migration.
+}
+
+}  // namespace
+
+namespace password_manager {
+
+PasswordManagerSettingMigraterService::PasswordManagerSettingMigraterService(
+    Profile* profile)
+    : profile_(profile), sync_service_(nullptr) {
+  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED,
+                 content::Source<Profile>(profile));
+}
+
+PasswordManagerSettingMigraterService::PasswordManagerSettingMigraterService(
+    Profile* profile,
+    sync_driver::SyncService* sync_service)
+    : profile_(profile), sync_service_(sync_service) {}
+
+PasswordManagerSettingMigraterService::
+    ~PasswordManagerSettingMigraterService() {}
+
+void PasswordManagerSettingMigraterService::Observe(

[vabr (Chromium), 2015/08/12 08:25:50]

Would it make sense if instead of the service observing this notification, the
factory would check ProfileManager::IsValidProfile in GetForProfile, and return
nullptr if the profile is not valid yet?

This would have two advantages:
(1) You could have the single constructor for the service, and guaranteed that
|sync_service| won't change value until your service's shutdown.
(2) You could drop the code for observing the profile notification.

[melandory, 2015/08/13 15:49:15]

Resolved by offline discussion

+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(type == chrome::NOTIFICATION_PROFILE_ADDED);

[vabr (Chromium), 2015/08/12 08:25:50]

nit: DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);

[melandory, 2015/08/13 15:49:15]

Done.

+  if (ProfileSyncServiceFactory::HasProfileSyncService(profile_))
+    sync_service_ = ProfileSyncServiceFactory::GetForProfile(profile_);
+  InitObservers();
+}
+
+void PasswordManagerSettingMigraterService::InitObservers() {
+  pref_change_registrar_.Init(profile_->GetPrefs());
+  pref_change_registrar_.Add(
+      prefs::kCredentialsEnableService,
+      base::Bind(&PasswordManagerSettingMigraterService::
+                     OnCredentialsEnableServicePrefChanged,
+                 base::Unretained(this)));
+  pref_change_registrar_.Add(
+      prefs::kPasswordManagerSavingEnabled,
+      base::Bind(&PasswordManagerSettingMigraterService::
+                     OnPasswordManagerSavingEnabledPrefChanged,
+                 base::Unretained(this)));
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  if (sync_service_ && sync_service_->CanSyncStart()) {
+    // This causes OnIsSyncingChanged to be called when the value of
+    // PrefService::IsSyncing() changes.
+    prefs->AddObserver(this);
+  } else {
+    PasswordManagerSettingMigraterService::MigrateLegacyOffState(prefs);

[vabr (Chromium), 2015/08/12 08:25:50]

nit: Can you drop PasswordManagerSettingMigraterService:: ?

[vabr (Chromium), 2015/08/12 08:25:50]

Why do we try to set the YOLO preference, when there is no sync service (or it
cannot start)?

[melandory, 2015/08/13 15:49:15]

It's point where we migrate non-sync users to the new setting. So idea that
every one will be migrated to the yolo setting even not yolo users.

[vabr (Chromium), 2015/08/14 14:11:44]

Good point, thanks for explanation!
Acknowledged.

+  }
+}
+
+void PasswordManagerSettingMigraterService::
+    OnCredentialsEnableServicePrefChanged(
+        const std::string& changed_pref_name) {
+  ChangeOnePrefBecauseAnotherPrefHasChanged(
+      profile_, prefs::kPasswordManagerSavingEnabled,
+      prefs::kCredentialsEnableService);
+}
+
+void PasswordManagerSettingMigraterService::
+    OnPasswordManagerSavingEnabledPrefChanged(
+        const std::string& changed_pref_name) {
+  ChangeOnePrefBecauseAnotherPrefHasChanged(
+      profile_, prefs::kCredentialsEnableService,
+      prefs::kPasswordManagerSavingEnabled);
+}
+
+void PasswordManagerSettingMigraterService::OnIsSyncingChanged() {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  if (prefs->IsSyncing()) {
+    MigrateLegacyOffState(prefs);
+    prefs->RemoveObserver(this);

[vabr (Chromium), 2015/08/12 12:32:09]

We should make sure that this is called also during Shutdown, if needed.

[melandory, 2015/08/13 15:49:15]

Done.

+  }
+}
+
+// static
+void PasswordManagerSettingMigraterService::MigrateLegacyOffState(

[vabr (Chromium), 2015/08/12 08:25:50]

Should we ensure that this is only called at most once?
Also, should we make sure that this is not called after Shutdown() has been
called on your service?

[melandory, 2015/08/13 15:49:15]

Done.

+    PrefService* prefs) {
+  bool old_pref = prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled);
+  if (!old_pref) {
+    prefs->SetBoolean(prefs::kCredentialsEnableService, false);
+  }
+}
+
+// static
+PasswordManagerSettingMigraterService::Factory*
+PasswordManagerSettingMigraterService::Factory::GetInstance() {
+  return Singleton<PasswordManagerSettingMigraterService::Factory>::get();
+}
+
+// static
+PasswordManagerSettingMigraterService*
+PasswordManagerSettingMigraterService::Factory::GetForProfile(
+    Profile* profile) {
+  return static_cast<PasswordManagerSettingMigraterService*>(
+      GetInstance()->GetServiceForBrowserContext(profile, true));
+}
+
+PasswordManagerSettingMigraterService::Factory::Factory()
+    : BrowserContextKeyedServiceFactory(
+          "PasswordManagerSettingMigraterService",
+          BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(ProfileSyncServiceFactory::GetInstance());
+}
+
+PasswordManagerSettingMigraterService::Factory::~Factory() {}
+
+KeyedService*
+PasswordManagerSettingMigraterService::Factory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new PasswordManagerSettingMigraterService(
+      static_cast<Profile*>(context));
+}
+
+bool PasswordManagerSettingMigraterService::Factory::
+    ServiceIsCreatedWithBrowserContext() const {
+  return true;
+}
+
+}  // namespace password_manager