[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(
+    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

[engedy, 2015/09/01 23:10:58]

Have you though of what metrics we could use to track this?

We would also probably need to put this behind a field trial, so that we could
migrate a small subset of users initially, and somehow judge based on said
metrics that we have done a good job at migrating.

[melandory, 2015/09/02 13:15:39]

As per offline discussion:
 * Log initial values for n days without performing migration, then roll
migration for some percentage and log initial and final values.
 * Log that pref value was changed separately for users who has performed
migration and for one who doesn't.


P.S. I'll implement this in a separate CL.

+  // migration.
+}
+
+// Change value of both kPasswordManagerSavingEnabled and
+// kCredentialsEnableService to the |new_value| only if the current value of the
+// pref differs.
+void UpdatePreferencesValuesIfNeeded(PrefService* prefs, bool new_value) {
+  bool legacy_pref_value =
+      prefs->GetBoolean(password_manager::prefs::kPasswordManagerSavingEnabled);
+  bool new_pref_value =
+      prefs->GetBoolean(password_manager::prefs::kCredentialsEnableService);
+  if (legacy_pref_value != new_value) {
+    prefs->SetBoolean(password_manager::prefs::kPasswordManagerSavingEnabled,
+                      new_value);
+  }
+  if (new_pref_value != new_value) {
+    prefs->SetBoolean(password_manager::prefs::kCredentialsEnableService,
+                      new_value);
+  }
+}
+
+void InitCurrentPrefState(PrefService* prefs,
+                          bool* legacy_pref_value,
+                          bool* new_pref_value,
+                          std::vector<bool>* initial_values) {
+  *legacy_pref_value =
+      prefs->GetBoolean(password_manager::prefs::kPasswordManagerSavingEnabled);
+  *new_pref_value =
+      prefs->GetBoolean(password_manager::prefs::kCredentialsEnableService);
+  initial_values->clear();
+  initial_values->push_back(*new_pref_value);
+  initial_values->push_back(*legacy_pref_value);
+}
+
+}  // namespace
+
+PasswordManagerSettingMigraterService::PasswordManagerSettingMigraterService(
+    Profile* profile)
+    : profile_(profile), sync_service_(nullptr) {
+  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED,

[engedy, 2015/08/31 18:17:23]

We should add a comment to summarize the discussion on the CL and explain this
cryptic line (i.e., that this is the signal that we use to wait until after
somebody deigns to create the ProfileSyncService). This can be either here, or
in the class comment in the header where we could add a paragraph describing
start-up ordering.

[melandory, 2015/09/02 13:15:39]

added to header

+                 content::Source<Profile>(profile));
+}
+
+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() {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  bool legacy_pref_value;
+  bool new_pref_value;
+  InitCurrentPrefState(prefs, &legacy_pref_value, &new_pref_value,
+                       &initial_values_);

[engedy, 2015/09/01 23:10:58]

Could we just save to an |initial_new_pref_value_| and
|initial_legacy_pref_value_| instead, and get rid of the std::vector?

[melandory, 2015/09/02 13:15:39]

Done.

+  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)));
+  if (!(sync_service_ && sync_service_->CanSyncStart())) {
+    PasswordManagerSettingMigraterService::MigrateOffState(prefs);
+  }
+  // This causes OnIsSyncingChanged to be called when the value of
+  // PrefService::IsSyncing() changes.
+  prefs->AddObserver(this);
+}
+
+void PasswordManagerSettingMigraterService::Shutdown() {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  prefs->RemoveObserver(this);
+}
+
+void PasswordManagerSettingMigraterService::
+    OnCredentialsEnableServicePrefChanged(
+        const std::string& changed_pref_name) {
+  PrefService* prefs = profile_->GetPrefs();
+  sync_data_.push_back(
+      prefs->GetBoolean(password_manager::prefs::kCredentialsEnableService));
+  ChangeOnePrefBecauseAnotherPrefHasChanged(
+      prefs, password_manager::prefs::kPasswordManagerSavingEnabled,
+      password_manager::prefs::kCredentialsEnableService);
+}
+
+void PasswordManagerSettingMigraterService::
+    OnPasswordManagerSavingEnabledPrefChanged(
+        const std::string& changed_pref_name) {
+  PrefService* prefs = profile_->GetPrefs();
+  sync_data_.push_back(prefs->GetBoolean(
+      password_manager::prefs::kPasswordManagerSavingEnabled));
+  ChangeOnePrefBecauseAnotherPrefHasChanged(
+      prefs, password_manager::prefs::kCredentialsEnableService,
+      password_manager::prefs::kPasswordManagerSavingEnabled);
+}
+
+void PasswordManagerSettingMigraterService::OnIsSyncingChanged() {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile_);
+  if (prefs->IsSyncing() && prefs->IsPrioritySyncing()) {
+    // Initial sync has finished.
+    MigrateAfterModelAssociation(prefs);
+  }
+
+  if (prefs->IsSyncing() == prefs->IsPrioritySyncing()) {
+    // Sync is not in model association step.
+    bool legacy_pref_value;
+    bool new_pref_value;
+    InitCurrentPrefState(prefs, &legacy_pref_value, &new_pref_value,
+                         &initial_values_);
+    sync_data_.clear();
+  }
+}
+
+void PasswordManagerSettingMigraterService::MigrateOffState(
+    PrefService* prefs) {
+  bool new_pref_value =
+      prefs->GetBoolean(password_manager::prefs::kCredentialsEnableService);
+  bool legacy_pref_value =
+      prefs->GetBoolean(password_manager::prefs::kPasswordManagerSavingEnabled);
+  if (new_pref_value != legacy_pref_value) {
+    // Values are not equal, so we reconcile preferences to the off state.
+    UpdatePreferencesValuesIfNeeded(prefs, false);
+  }
+}
+
+void PasswordManagerSettingMigraterService::MigrateAfterModelAssociation(
+    PrefService* prefs) {
+  if (sync_data_.empty()) {
+    MigrateOffState(prefs);
+  } else if (sync_data_.size() == 1) {
+    // Only one value has came from sync. This value should be assigned to both
+    // preferences.
+    UpdatePreferencesValuesIfNeeded(prefs, sync_data_[0]);
+  } else {
+    DCHECK_EQ(2u, initial_values_.size());
+    bool inital_new_pref_value = initial_values_[1];
+    bool initial_legacy_pref_value = initial_values_[0];
+    bool sync_new_pref_value = sync_data_[0];
+    bool sync_legacy_pref_value = sync_data_[sync_data_.size() - 1];
+    if (sync_legacy_pref_value && sync_new_pref_value) {
+      UpdatePreferencesValuesIfNeeded(prefs, true);
+    } else if (!sync_legacy_pref_value && !sync_new_pref_value) {
+      UpdatePreferencesValuesIfNeeded(prefs, false);
+    } else if (!initial_legacy_pref_value && !inital_new_pref_value) {
+      UpdatePreferencesValuesIfNeeded(prefs, true);
+    } else {
+      UpdatePreferencesValuesIfNeeded(prefs, 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());

[engedy, 2015/08/31 18:17:23]

We should add a comment to summarize the discussion on the CL and explain this
cryptic line. This can be either here, or in the class comment in the header
where we could add a paragraph describing start-up ordering.

[melandory, 2015/09/02 13:15:39]

added to header.

+}
+
+PasswordManagerSettingMigraterService::Factory::~Factory() {}
+
+KeyedService*
+PasswordManagerSettingMigraterService::Factory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new PasswordManagerSettingMigraterService(
+      static_cast<Profile*>(context));

[engedy, 2015/08/31 18:17:23]

nit: Profile::FromBrowserContext

[melandory, 2015/09/02 13:15:39]

Done.

+}
+
+bool PasswordManagerSettingMigraterService::Factory::
+    ServiceIsCreatedWithBrowserContext() const {
+  return true;
+}