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

chrome/browser/password_manager/password_manager_setting_migrater_service.h

Index: chrome/browser/password_manager/password_manager_setting_migrater_service.h
diff --git a/chrome/browser/password_manager/password_manager_setting_migrater_service.h b/chrome/browser/password_manager/password_manager_setting_migrater_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..6e9759fd5dfc4b4527b642f718a25224bf963833
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_setting_migrater_service.h
@@ -0,0 +1,102 @@
+// 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.
+
+#ifndef CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_SETTING_MIGRATER_SERVICE_H_
+#define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_SETTING_MIGRATER_SERVICE_H_
+
+#include "base/memory/singleton.h"
+#include "base/prefs/pref_change_registrar.h"
+#include "chrome/browser/prefs/pref_service_syncable_observer.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+class Profile;
+
+namespace sync_driver {
+class SyncService;
+}
+
+namespace password_manager {

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

nit: By convention, code in //chrome mostly does not use namespaces, and
password manager code should be in its namespace only in the component. To keep
consistency, I suggest not creating the new code in //chrome in namespace
password_manager. Feel free to use "using" declarations in the .cc files to
avoid lengthy expressions when referencing code from the component.

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

Done.

+
+// PasswordManagerSettingMigraterService is responsible for reconciling Chrome
+// password manager and Smart Lock for passwords settings.
+// Note: componetization of this service currently is impossible, because it
+// depends on PrefServiceSyncable.
+class PasswordManagerSettingMigraterService
+    : public KeyedService,
+      public PrefServiceSyncableObserver,
+      public content::NotificationObserver {
+ public:
+  class Factory : public BrowserContextKeyedServiceFactory {
+   public:
+    static Factory* GetInstance();
+    static PasswordManagerSettingMigraterService* GetForProfile(
+        Profile* profile);
+
+   private:
+    friend struct DefaultSingletonTraits<Factory>;
+    friend class PasswordManagerSettingMigraterServiceTest;
+
+    Factory();
+    ~Factory() override;
+
+    // BrowserContextKeyedServiceFactory
+    KeyedService* BuildServiceInstanceFor(
+        content::BrowserContext* profile) const override;
+    bool ServiceIsCreatedWithBrowserContext() const override;
+  };
+
+  explicit PasswordManagerSettingMigraterService(Profile* profile);
+  ~PasswordManagerSettingMigraterService() override;
+
+  // Initializes the observers: preferences observers and sync status observers.
+  void InitObservers();
+
+  // PrefServiceSyncableObserver
+  void OnIsSyncingChanged() override;
+
+  // content::NotificationObserver
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
+ private:
+  friend class PasswordManagerSettingMigraterServiceTest;
+
+  // For unit testing only.
+  PasswordManagerSettingMigraterService(Profile* profile,

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

As engedy@ noted before -- why not make this the only constructor, and inject
the production sync service through it as well?

Any test-specific changes in production are usually a bad sign, forking
production code for test means that the less of the production code gets tested.

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

Found another way how to inject mock ProfileSyncService.

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

Great, I like that it now comes with the notification as in production code.

+                                        sync_driver::SyncService* sync_service);
+
+  // Called on event of changing kCredentialsEnableService preference. Changes
+  // the value of kPasswordManagerSavingEnabled preference to be in sync with
+  // kCredentialsEnableService.
+  void OnCredentialsEnableServicePrefChanged(
+      const std::string& changed_pref_name);
+
+  // Called on event of changing kPasswordManagerSavingEnabled preference.
+  // Changes the value of kCredentialsEnableService preference to be in sync
+  // with kPasswordManagerSavingEnabled.
+  void OnPasswordManagerSavingEnabledPrefChanged(
+      const std::string& changed_pref_name);
+
+  // Turns off CredentialsEnableService when PasswordManagerSavingEnabled is
+  // turned off.
+  void MigrateLegacyOffState(PrefService* prefs);
+
+  Profile* profile_;

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

The member pointers which are not touched elsewhere than during construction
should be marked const:
Profile* const profile_;
sync_driver::SyncService* const sync_service_;

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

Done.

Can't do this for sync_service_, because it nit initialized in constructor.

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

Acknowledged.

+
+  sync_driver::SyncService* sync_service_;
+  PrefChangeRegistrar pref_change_registrar_;
+
+  // Used to register for notification that profile creation is complete.
+  content::NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordManagerSettingMigraterService);
+};
+
+}  // namespace password_manager
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_SETTING_MIGRATER_SERVICE_H_