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

chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc

Index: chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc
diff --git a/chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc b/chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5163cfaf51632360d579f7e40ae7a2e415bfab13
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc
@@ -0,0 +1,245 @@
+// 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 "base/json/json_string_value_serializer.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "base/values.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/login/session/user_session_manager.h"
+#include "chrome/browser/password_manager/password_manager_setting_migrater_service.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/sync/profile_sync_service_mock.h"
+#include "chrome/test/base/testing_pref_service_syncable.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "sync/api/fake_sync_change_processor.h"
+#include "sync/api/sync_error_factory.h"
+#include "sync/api/sync_error_factory_mock.h"
+#include "sync/internal_api/public/attachments/attachment_service_proxy_for_test.h"
+#include "sync/protocol/preference_specifics.pb.h"
+#include "sync/protocol/priority_preference_specifics.pb.h"
+#include "sync/protocol/sync.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+syncer::SyncData CreatePrefSyncData(const std::string& name, bool value) {
+  std::string serialized;
+  JSONStringValueSerializer json(&serialized);
+  base::FundamentalValue bool_value(value);
+  json.Serialize(bool_value);
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::PreferenceSpecifics* pref = nullptr;
+  if (name == password_manager::prefs::kPasswordManagerSavingEnabled) {
+    pref = specifics.mutable_preference();
+  } else if (name == password_manager::prefs::kCredentialsEnableService) {
+    pref = specifics.mutable_priority_preference()->mutable_preference();
+  }
+  if (pref) {

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

Is this only to handle wrong preference names gracefully? Or could pref be null
even after line 42 or 44 is executed?
If it cannot be null for the right pref names, I suggest replacing the if with
DCHECK(pref) << "Wrong preference name: " << name;
or alternatively not checking pref at all, and passing an enum instead of
|name|, and deriving the string from the enum.

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

Done.

+    pref->set_name(name);
+    pref->set_value(serialized);
+  }
+  return syncer::SyncData::CreateRemoteData(
+      1, specifics, base::Time(), syncer::AttachmentIdList(),
+      syncer::AttachmentServiceProxyForTest::Create());
+}
+
+void StartSyncing(PrefServiceSyncable* prefs,

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

Please comment on what this function does.

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

Done.

+                  const std::string& name,
+                  syncer::SyncDataList sync_data_list) {
+  syncer::ModelType type = syncer::UNSPECIFIED;
+  if (name == password_manager::prefs::kPasswordManagerSavingEnabled)
+    type = syncer::PREFERENCES;
+  else if (name == password_manager::prefs::kCredentialsEnableService)
+    type = syncer::PRIORITY_PREFERENCES;
+  syncer::SyncableService* sync = prefs->GetSyncableService(type);

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

nit: Insert

ASSERT_NE(syncer::UNSPECIFIED, type) << "Wrong preference name: " << name;

or do the change with the enum suggested above.

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

Done.

+  sync->MergeDataAndStartSyncing(
+      type, sync_data_list, scoped_ptr<syncer::SyncChangeProcessor>(
+                                new syncer::FakeSyncChangeProcessor),
+      scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock));
+}
+
+}  // namespace
+
+namespace password_manager {
+
+class PasswordManagerSettingMigraterServiceTest : public testing::Test {
+ public:
+  PasswordManagerSettingMigraterServiceTest() : service_(&profile_) {}
+
+  void SetUp() override {
+    ProfileSyncServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+        &profile_, &ProfileSyncServiceMock::BuildMockProfileSyncService);
+    EXPECT_CALL(*profile_sync_service(), CanSyncStart())
+        .WillRepeatedly(testing::Return(true));
+  }
+  Profile* profile() { return &profile_; }

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

nit: Please insert blank lines between methods.

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

Done.

+  PasswordManagerSettingMigraterService* service() { return &service_; }
+  ProfileSyncServiceMock* profile_sync_service() {
+    return static_cast<ProfileSyncServiceMock*>(
+        ProfileSyncServiceFactory::GetInstance()->GetForProfile(&profile_));
+  }
+
+  void NotifyProfileAdded() {
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_PROFILE_ADDED, content::Source<Profile>(&profile_),
+        content::NotificationService::NoDetails());
+  }
+
+  void TestOnLocalChange(const std::string& name, bool value) {
+    NotifyProfileAdded();
+    PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+    prefs->SetBoolean(name, value);
+    EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), value);

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

nit: Swap the order of operands at EXPECT_EQ -- the first one needs to be the
expected value, the second on the actual output of the tested code.

Please check this throughout the test.

(While equivalence is a symmetric relation, the side-effect of failing EXPECT_EQ
is not -- the first value is reported as expected, and the second as actual.)

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

Done.

+    EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), value);
+  }
+
+  // Checks that if Sync data doesn't contain new value for
+  // kCredentialsEnableService or kPasswordManagerSavingEnabled, then their
+  // |value| stays the same after Sync merges the data.
+  void TestEmptySyncDataDoesntAffectPrefValues(bool value) {
+    NotifyProfileAdded();
+    PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+    EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), value);
+    EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), value);
+    // Empty data comes from sync.
+    StartSyncing(prefs, prefs::kPasswordManagerSavingEnabled,
+                 syncer::SyncDataList());
+    StartSyncing(prefs, prefs::kCredentialsEnableService,
+                 syncer::SyncDataList());
+    EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), value);
+    EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), value);
+  }
+
+  // Checks that if |new_value| for the preference named |name| has came from
+  // sync, then both kCredentialsEnableService and kPasswordManagerSavingEnabled
+  // have |new_value| as value.
+  void TestReconcileOnNewPrefValueArrivesFromSync(const std::string& name,
+                                                  bool new_value) {
+    NotifyProfileAdded();
+    PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+    EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true);

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

Why is the old value always true?

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

Will add another set of tests for the other case.

+    EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), true);
+    syncer::SyncDataList sync_data_list;
+    sync_data_list.push_back(CreatePrefSyncData(name, new_value));
+    StartSyncing(prefs, name, sync_data_list);
+    EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), new_value);
+    EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled),
+              new_value);
+  }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  PasswordManagerSettingMigraterService service_;
+};
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnLocalChangeOfPasswordManagerSavingEnabledOn) {
+  TestOnLocalChange(prefs::kPasswordManagerSavingEnabled, true);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnLocalChangeOfPasswordManagerSavingEnabledOff) {
+  TestOnLocalChange(prefs::kPasswordManagerSavingEnabled, false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnLocalChangeOfCredentialsEnableServiceOn) {
+  TestOnLocalChange(prefs::kCredentialsEnableService, true);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnLocalChangeOfCredentialsEnableServiceOff) {
+  TestOnLocalChange(prefs::kCredentialsEnableService, false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnSyncMergeEmptyDataFinished) {
+  // kPasswordManagerSavingEnabled is set to "off" and kCredentialsEnableService
+  // is created for the first time with default value "on". Data which has came
+  // from sync doesn't contain changes for these preferences, so after sync has
+  // finished merging, both preferences should be "off".
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+  prefs->SetBoolean(prefs::kPasswordManagerSavingEnabled, false);
+  EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true);
+  NotifyProfileAdded();
+  EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true);
+  EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), false);
+  // Empty data comes from sync.
+  StartSyncing(prefs, prefs::kPasswordManagerSavingEnabled,
+               syncer::SyncDataList());
+  StartSyncing(prefs, prefs::kCredentialsEnableService, syncer::SyncDataList());
+  EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), false);
+  EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnSyncMergeEmptyDataFinishedAllOn) {
+  TestEmptySyncDataDoesntAffectPrefValues(true);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileOnSyncMergeEmptyDataFinishedAllOff) {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+  prefs->SetBoolean(prefs::kPasswordManagerSavingEnabled, false);
+  prefs->SetBoolean(prefs::kCredentialsEnableService, false);
+  TestEmptySyncDataDoesntAffectPrefValues(false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileWithPasswordManagerSavingEnabledOffFromSync) {
+  TestReconcileOnNewPrefValueArrivesFromSync(
+      prefs::kPasswordManagerSavingEnabled, false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileWithPasswordManagerSavingEnabledOnFromSync) {
+  TestReconcileOnNewPrefValueArrivesFromSync(
+      prefs::kPasswordManagerSavingEnabled, true);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileWithCredentialsEnableServiceOffFromSync) {
+  TestReconcileOnNewPrefValueArrivesFromSync(prefs::kCredentialsEnableService,
+                                             false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileWithCredentialsEnableServiceOnFromSync) {
+  TestReconcileOnNewPrefValueArrivesFromSync(prefs::kCredentialsEnableService,
+                                             true);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileWhenSyncIsNotExpectedPasswordManagerEnabledOff) {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+  prefs->SetBoolean(prefs::kPasswordManagerSavingEnabled, false);
+  EXPECT_CALL(*profile_sync_service(), CanSyncStart())
+      .WillRepeatedly(testing::Return(false));
+  NotifyProfileAdded();
+  EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), false);
+  EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), false);
+}
+
+TEST_F(PasswordManagerSettingMigraterServiceTest,
+       ReconcileWhenSyncIsNotExpectedPasswordManagerEnabledOn) {
+  PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile());
+  prefs->SetBoolean(prefs::kPasswordManagerSavingEnabled, true);
+  EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true);
+  EXPECT_CALL(*profile_sync_service(), CanSyncStart())
+      .WillRepeatedly(testing::Return(false));
+  NotifyProfileAdded();
+  EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true);
+  EXPECT_EQ(prefs->GetBoolean(prefs::kPasswordManagerSavingEnabled), true);
+}
+
+}  // namespace password_manager