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

chrome/browser/password_manager/password_manager_migration_service.h

+// 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_MIGRATION_SERVICE_H_
+#define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_MIGRATION_SERVICE_H_
+
+#include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.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"
+
+class Profile;
+class ProfileSyncService;
+
+namespace password_manager {
+
+// PasswordSettingsMigrationService is responsible for reconcile Chrome password

[vabr (Chromium), 2015/08/07 08:48:02]

nit: reconciling

[melandory, 2015/08/12 09:03:23]

Done.

+// manager and Smart Lock for passwords settings.
+class PasswordSettingsMigrationService : public KeyedService,

[engedy, 2015/08/07 11:41:38]

I'd also suggest to slightly change the class name to something like
PasswordManagerSettingMigraterService, so as to better capture that this is for
PM (not for "passwords" in general) and that we are currently only migrating a
single setting.

(I am open to better name suggestion, too. :))

[melandory, 2015/08/12 09:03:24]

Done.

+                                         public PrefServiceSyncableObserver {
+ public:
+  explicit PasswordSettingsMigrationService(Profile* profile);
+  ~PasswordSettingsMigrationService() override;
+
+  // Initializes the observers: preferences observers and sync status observers.
+  void InitObservers();
+
+  class Factory : public BrowserContextKeyedServiceFactory {

[engedy, 2015/08/07 11:41:38]

nit: Declare nested types first within a section. See:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

[melandory, 2015/08/12 09:03:24]

Done.

+   public:
+    static Factory* GetInstance();
+    static PasswordSettingsMigrationService* GetForProfile(Profile* profile);
+
+   private:
+    friend struct DefaultSingletonTraits<Factory>;
+    friend class PasswordSettingsMigrationServiceTest;

[vabr (Chromium), 2015/08/07 08:48:02]

so far non-actionable note:
Friending for tests is rarely the right thing to do. Tests should test the
external API of classes, not the implementation, which may be subject to change.
There are exceptions, but I have not had chance to have a proper look yet. Feel
free to ignore this comment for now, I may post something more actionable soon.

+
+    Factory();
+    ~Factory() override;
+
+    // BrowserContextKeyedServiceFactory implementation

[engedy, 2015/08/07 11:41:38]

optional nit: Some people prefer writing just:

// BrowserContextKeyedServiceFactory:

It's up to you -- just make sure you are consistent with the other section
below.

[melandory, 2015/08/12 09:03:24]

Done.

+    KeyedService* BuildServiceInstanceFor(
+        content::BrowserContext* profile) const override;
+    bool ServiceIsCreatedWithBrowserContext() const override;
+    bool ServiceIsNULLWhileTesting() const override;
+    content::BrowserContext* GetBrowserContextToUse(
+        content::BrowserContext* context) const override;
+  };
+
+  // PrefServiceSyncableObserver:
+  void OnIsSyncingChanged() override;
+
+ private:
+  friend class PasswordSettingsMigrationServiceTest;
+
+  // For unit testing only.
+  PasswordSettingsMigrationService(Profile* profile,
+                                   ProfileSyncService* profile_sync_service);

[engedy, 2015/08/07 11:41:38]

You could promote this to be the main constructor, and inject the dependencies
in the Facgory::BuildServiceInstanceFor, that way, you would not need special
casing for tests.  

[melandory, 2015/08/12 09:03:23]

Unfortunately, can't do this anymore because of ProfileSyncService not available
by the time  Facgory::BuildServiceInstanceFor called.

What I can do is: pass ProfileSyncService* as argument of InitObservers. This
way I'll also be able to eliminate constructor, which is used for unittests
only. WDYT?

+  // Called on event of changing either PasswordManagerSavingEnabled or

[vabr (Chromium), 2015/08/07 08:48:02]

nit: blank line above

[melandory, 2015/08/12 09:03:23]

Done.

+  // CredentialEnableService. Change the value of another preference to be in

[vabr (Chromium), 2015/08/07 08:48:02]

What is CredentialEnableService?

[engedy, 2015/08/07 11:41:38]

It's the preference name. But, yes, we would probably want to keep the
|unix_hacker_style| name for it.

+  // sync with changed preference.
+  void OnPrefChanged(const std::string& other_pref_name,

[engedy, 2015/08/07 11:41:38]

nit: I think we could have two separate methods, OnLegacyPrefChanged,
OnNewPrefChanged. In my opinion, that could be more readable.

[melandory, 2015/08/12 09:03:23]

Done with correction that I changed New and Legacy with prefs constant names. Or
you think New and Legacy is better?

+                     const std::string& changed_pref_name);
+
+  // Turns off CredentialEnableService when PasswordManagerSavingEnabled is
+  // turned off.
+  void Reconcile(PrefService* prefs);

[vabr (Chromium), 2015/08/07 08:48:02]

The name does not seem to match the comment. Reconcile sounds more generic than
just "turn off when turned off". If it does what the comment says, it should be
called accordingly, otherwise the comment should be updated.

[engedy, 2015/08/07 11:41:38]

Apologies, I am afraid it was me who originally came up with the name. On second
thought, it indeed wasn't the best choice.

One naming idea is to call it: "MigrateLegacyOffState". WDYT?

[vabr (Chromium), 2015/08/07 11:56:19]

SGTM!

[melandory, 2015/08/12 09:03:24]

Done.

+
+  Profile* profile_;
+
+  ProfileSyncService* profile_sync_service_;

[vabr (Chromium), 2015/08/07 08:48:02]

Please use the parent interface from components/sync_driver/sync_service.h
instead. No need to introduce //chrome dependencies.

+  PrefChangeRegistrar pref_change_registrar_;
+
+  base::WeakPtrFactory<PasswordSettingsMigrationService> weak_factory_;

[engedy, 2015/08/07 11:41:38]

nit: Do we actually use the |weak_factory_|?

[melandory, 2015/08/12 09:03:23]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(PasswordSettingsMigrationService);
+};
+
+}  // namespace password_manager
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_MANAGER_MIGRATION_SERVICE_H_