[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 {

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

nit: Blank line below.

[melandory, 2015/08/17 16:28:55]

Done.

+void ChangeOnePrefBecauseAnotherPrefHasChanged(
+    PrefService* prefs,
+    const std::string& other_pref_name,
+    const std::string& changed_pref_name) {
+  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
+
+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(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type);
+  if (ProfileSyncServiceFactory::HasProfileSyncService(profile_))
+    sync_service_ = ProfileSyncServiceFactory::GetForProfile(profile_);
+  InitObservers();
+}
+
+void PasswordManagerSettingMigraterService::InitObservers() {
+  pref_change_registrar_.Init(profile_->GetPrefs());
+  pref_change_registrar_.Add(
+      password_manager::prefs::kCredentialsEnableService,
+      base::Bind(&PasswordManagerSettingMigraterService::
+                     OnCredentialsEnableServicePrefChanged,
+                 base::Unretained(this)));
+  pref_change_registrar_.Add(
+      password_manager::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);
+  }
+}
+
+void PasswordManagerSettingMigraterService::Shutdown() {
+  PrefService* prefs = profile_->GetPrefs();
+  MigrateLegacyOffState(prefs);

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

I suggest replacing this line with
prefs->RemoveObserver(this);

Please also make sure that it is OK to call RemoveObserver if we did not call
AddObserver (and if that's not OK, then ensure that we only call RO if we called
AO).

[melandory, 2015/08/17 16:28:55]

Yes, nothing will happen if we remove non added observer
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

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

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

nit: I would swap the order of lines 107 and 108. It probably does not change
anything, but just in case there were some posting of tasks in the future which
could cause re-entering this block, removing the observer before acting on the
observed signal prevents that.

[melandory, 2015/08/17 16:28:55]

Done.

+  }
+}
+
+// static
+void PasswordManagerSettingMigraterService::MigrateLegacyOffState(
+    PrefService* prefs) {
+  bool old_pref =
+      prefs->GetBoolean(password_manager::prefs::kPasswordManagerSavingEnabled);
+  if (!old_pref) {
+    prefs->SetBoolean(password_manager::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;
+}