Password sync refactoring: implemented ProcessSyncChanges() and GetAllSyncData()

chrome/browser/password_manager/password_syncable_service_unittest.cc

Index: chrome/browser/password_manager/password_syncable_service_unittest.cc
diff --git a/chrome/browser/password_manager/password_syncable_service_unittest.cc b/chrome/browser/password_manager/password_syncable_service_unittest.cc
index aa40d67483dda83ad301f0ef3023e96102fbfcef..33eb34cddb2b0f3ed884b14162c1f71b7a57c58d 100644
--- a/chrome/browser/password_manager/password_syncable_service_unittest.cc
+++ b/chrome/browser/password_manager/password_syncable_service_unittest.cc
@@ -95,6 +95,7 @@ class PasswordStoreDataVerifier {
   ~PasswordStoreDataVerifier() {
     EXPECT_TRUE(expected_db_add_changes_.empty());
     EXPECT_TRUE(expected_db_update_changes_.empty());
+    EXPECT_TRUE(expected_db_delete_changes_.empty());
   }
 
   class TestSyncChangeProcessor;
@@ -103,6 +104,7 @@ class PasswordStoreDataVerifier {
   void SetExpectedDBChanges(
       const SyncDataList& add_forms,
       const std::vector<autofill::PasswordForm*>& update_forms,
+      const std::vector<autofill::PasswordForm*>& delete_forms,
       MockPasswordStore* password_store);
   // Sets expected changes to TestSyncChangeProcessor.
   void SetExpectedSyncChanges(SyncChangeList list);
@@ -119,13 +121,16 @@ class PasswordStoreDataVerifier {
                         &expected_db_add_changes_);
   }
 
-  // Verifies that the |password| is present in the
-  // |expected_db_update_changes_| list.

[Ilya Sherman, 2014/02/07 22:56:35]

Rather than removing this comment, please expand it to be more like the comment
on lines 116-118.

[vasilii, 2014/02/10 19:06:50]

Done.

   PasswordStoreChangeList VerifyUpdate(const autofill::PasswordForm& password) {
     return VerifyChange(PasswordStoreChange::UPDATE, password,
                         &expected_db_update_changes_);
   }
 
+  PasswordStoreChangeList VerifyDelete(const autofill::PasswordForm& password) {
+    return VerifyChange(PasswordStoreChange::REMOVE, password,
+                        &expected_db_delete_changes_);
+  }
+
   static PasswordStoreChangeList VerifyChange(
       PasswordStoreChange::Type type,
       const autofill::PasswordForm& password,
@@ -133,6 +138,7 @@ class PasswordStoreDataVerifier {
 
   std::vector<autofill::PasswordForm> expected_db_add_changes_;
   std::vector<autofill::PasswordForm> expected_db_update_changes_;
+  std::vector<autofill::PasswordForm> expected_db_delete_changes_;
   SyncChangeList expected_sync_change_list_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordStoreDataVerifier);
@@ -163,6 +169,7 @@ class PasswordStoreDataVerifier::TestSyncChangeProcessor
 void PasswordStoreDataVerifier::SetExpectedDBChanges(
     const SyncDataList& add_forms,
     const std::vector<autofill::PasswordForm*>& update_forms,
+    const std::vector<autofill::PasswordForm*>& delete_forms,
     MockPasswordStore* password_store) {
   DCHECK(expected_db_add_changes_.empty());
   DCHECK(expected_db_update_changes_.empty());
@@ -194,6 +201,19 @@ void PasswordStoreDataVerifier::SetExpectedDBChanges(
         .Times(expected_db_update_changes_.size())
         .WillRepeatedly(Invoke(this, &PasswordStoreDataVerifier::VerifyUpdate));
   }
+
+  for (std::vector<autofill::PasswordForm*>::const_iterator i =

[Ilya Sherman, 2014/02/07 22:56:35]

nit: i -> it

[vasilii, 2014/02/10 19:06:50]

Done.

+           delete_forms.begin();
+       i != delete_forms.end(); ++i) {
+    expected_db_delete_changes_.push_back(**i);
+  }
+  if (expected_db_delete_changes_.empty()) {
+    EXPECT_CALL(*password_store, RemoveLoginImpl(_)).Times(0);
+  } else {
+    EXPECT_CALL(*password_store, RemoveLoginImpl(_))
+        .Times(expected_db_delete_changes_.size())
+        .WillRepeatedly(Invoke(this, &PasswordStoreDataVerifier::VerifyDelete));
+  }
 }
 
 void PasswordStoreDataVerifier::SetExpectedSyncChanges(SyncChangeList list) {
@@ -241,20 +261,28 @@ PasswordStoreChangeList PasswordStoreDataVerifier::VerifyChange(
   return PasswordStoreChangeList(1, PasswordStoreChange(type, password));
 }
 
-class PasswordSyncableServiceTest : public testing::Test {
+class PasswordSyncableServiceWrapper {
  public:
-  PasswordSyncableServiceTest() {}
-  virtual ~PasswordSyncableServiceTest() {}
+  PasswordSyncableServiceWrapper() {}
+  ~PasswordSyncableServiceWrapper() {}
 
-  virtual void SetUp() OVERRIDE {
+  void SetUp() {
     password_store_ = new MockPasswordStore;
     service_.reset(new MockPasswordSyncableService(password_store_));
   }
 
-  virtual void TearDown() OVERRIDE {
+  void TearDown() {
     password_store_->Shutdown();

[Nicolas Zea, 2014/02/07 21:16:47]

Can you move SetUp and TearDown into the constructor/destructor? Then you don't
have to have SetUp and TearDown in the test class either

[vasilii, 2014/02/10 19:06:50]

Done.

   }
 
+  MockPasswordStore* password_store() {
+    return password_store_;
+  }
+
+  scoped_ptr<MockPasswordSyncableService>& service() {

[Ilya Sherman, 2014/02/07 22:56:35]

Hmm, returning a scoped_ptr by reference is pretty unusual.  Would it make sense
to return a raw pointer here instead?

[vasilii, 2014/02/10 19:06:50]

In PasswordSyncableServiceTest.MergeDataAndPushBack I have to transfer ownership
of this object to MergeDataAndStartSyncing().

[Ilya Sherman, 2014/02/11 07:59:45]

I see.  It would be better -- in that it's less surprising to others reading the
code -- to separate the simple accessor method from the method that can transfer
ownership.  So, I'd recommend having two methods:

MockPasswordSyncableService* service();

// Document that this has the side-effect of NULLing out the service_.
scoped_ptr<syncer::SyncChangeProcessor> GetSyncChangeProcessor();

[vasilii, 2014/02/11 12:23:33]

Done.

+    return service_;
+  }
+
   PasswordStoreDataVerifier* verifier() {
     return &verifier_;
   }
@@ -267,13 +295,35 @@ class PasswordSyncableServiceTest : public testing::Test {
   // Sets the data that will be returned to the caller accessing password store.
   void SetPasswordStoreData(const std::vector<autofill::PasswordForm*>& forms) {
     EXPECT_CALL(*password_store_, FillAutofillableLogins(_))
-        .WillOnce(DoAll(SetArgPointee<0>(forms), Return(true)));
+        .WillOnce(DoAll(SetArgPointee<0>(forms), Return(true)))
+        .RetiresOnSaturation();
+    EXPECT_CALL(*password_store_, FillBlacklistLogins(_))
+        .WillOnce(Return(true))
+        .RetiresOnSaturation();
   }
 
  protected:
   scoped_refptr<MockPasswordStore> password_store_;
   scoped_ptr<MockPasswordSyncableService> service_;
   PasswordStoreDataVerifier verifier_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PasswordSyncableServiceWrapper);
+};
+
+class PasswordSyncableServiceTest : public testing::Test,
+                                    public PasswordSyncableServiceWrapper {
+ public:
+  PasswordSyncableServiceTest() {}
+  virtual ~PasswordSyncableServiceTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    PasswordSyncableServiceWrapper::SetUp();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    PasswordSyncableServiceWrapper::TearDown();
+  }
 };
 
 
@@ -291,15 +341,16 @@ TEST_F(PasswordSyncableServiceTest, AdditionsInBoth) {
 
   verifier()->SetExpectedDBChanges(list,
                                    std::vector<autofill::PasswordForm*>(),
-                                   password_store_);
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
   verifier()->SetExpectedSyncChanges(
       SyncChangeList(1, CreateSyncChange(*form1, SyncChange::ACTION_ADD)));
-  EXPECT_CALL(*service_, NotifyPasswordStoreOfLoginChanges(_));
+  EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_));
 
-  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
-                                     list,
-                                     ReleaseSyncChangeProcessor(),
-                                     scoped_ptr<syncer::SyncErrorFactory>());
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
 }
 
 // Sync has data that is not present in the password db.
@@ -312,14 +363,15 @@ TEST_F(PasswordSyncableServiceTest, AdditionOnlyInSync) {
 
   verifier()->SetExpectedDBChanges(list,
                                    std::vector<autofill::PasswordForm*>(),
-                                   password_store_);
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
   verifier()->SetExpectedSyncChanges(SyncChangeList());
-  EXPECT_CALL(*service_, NotifyPasswordStoreOfLoginChanges(_));
+  EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_));
 
-  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
-                                     list,
-                                     ReleaseSyncChangeProcessor(),
-                                     scoped_ptr<syncer::SyncErrorFactory>());
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
 }
 
 // Passwords db has data that is not present in sync.
@@ -332,16 +384,17 @@ TEST_F(PasswordSyncableServiceTest, AdditionOnlyInPasswordStore) {
 
   verifier()->SetExpectedDBChanges(SyncDataList(),
                                    std::vector<autofill::PasswordForm*>(),
-                                   password_store_);
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
   verifier()->SetExpectedSyncChanges(
       SyncChangeList(1, CreateSyncChange(*form1, SyncChange::ACTION_ADD)));
   EXPECT_CALL(*service_,
               NotifyPasswordStoreOfLoginChanges(PasswordStoreChangeList()));
 
-  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
-                                     SyncDataList(),
-                                     ReleaseSyncChangeProcessor(),
-                                     scoped_ptr<syncer::SyncErrorFactory>());
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      SyncDataList(),
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
 }
 
 // Both passwords db and sync contain the same data.
@@ -354,15 +407,16 @@ TEST_F(PasswordSyncableServiceTest, BothInSync) {
 
   verifier()->SetExpectedDBChanges(SyncDataList(),
                                    std::vector<autofill::PasswordForm*>(),
-                                   password_store_);
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
   verifier()->SetExpectedSyncChanges(SyncChangeList());
   EXPECT_CALL(*service_,
               NotifyPasswordStoreOfLoginChanges(PasswordStoreChangeList()));
 
-  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
-                                     SyncDataList(1, CreateSyncData("abc")),
-                                     ReleaseSyncChangeProcessor(),
-                                     scoped_ptr<syncer::SyncErrorFactory>());
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      SyncDataList(1, CreateSyncData("abc")),
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
 }
 
 // Both passwords db and sync have the same data but they need to be merged
@@ -378,16 +432,17 @@ TEST_F(PasswordSyncableServiceTest, Merge) {
 
   verifier()->SetExpectedDBChanges(SyncDataList(),
                                    forms,
-                                   password_store_);
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
   verifier()->SetExpectedSyncChanges(
       SyncChangeList(1, CreateSyncChange(*form1, SyncChange::ACTION_UPDATE)));
 
-  EXPECT_CALL(*service_, NotifyPasswordStoreOfLoginChanges(_));
+  EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_));
 
-  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
-                                     SyncDataList(1, CreateSyncData("abc")),
-                                     ReleaseSyncChangeProcessor(),
-                                     scoped_ptr<syncer::SyncErrorFactory>());
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      SyncDataList(1, CreateSyncData("abc")),
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
 }
 
 // Initiate sync due to local DB changes.
@@ -416,14 +471,123 @@ TEST_F(PasswordSyncableServiceTest, PasswordStoreChanges) {
 
   verifier()->SetExpectedDBChanges(SyncDataList(),
                                    std::vector<autofill::PasswordForm*>(),
-                                   password_store_);
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
   verifier()->SetExpectedSyncChanges(sync_list);
 
   PasswordStoreChangeList list;
   list.push_back(PasswordStoreChange(PasswordStoreChange::ADD, form1));
   list.push_back(PasswordStoreChange(PasswordStoreChange::UPDATE, form2));
   list.push_back(PasswordStoreChange(PasswordStoreChange::REMOVE, form3));
-  service_->ActOnPasswordStoreChanges(list);
+  service()->ActOnPasswordStoreChanges(list);
+}
+
+// Process all types of changes from sync.
+TEST_F(PasswordSyncableServiceTest, ProcessSyncChanges) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;

[Ilya Sherman, 2014/02/07 22:56:35]

nit: Please use a scoped_ptr here.  If for some reason you're super opposed to
using a scoped_ptr, then please swap the space and the asterisk (" *form1" -> "*
form1").

[vasilii, 2014/02/10 19:06:50]

Done.

+  form1->signon_realm = "abc";
+  form1->action = GURL("http://foo.com");
+  form1->date_created = base::Time::Now();
+  autofill::PasswordForm *form2 = new autofill::PasswordForm;

[Ilya Sherman, 2014/02/07 22:56:35]

Ditto.

[vasilii, 2014/02/10 19:06:50]

Done.

+  form2->signon_realm = "xyz";
+  form2->action = GURL("http://bar.com");
+  form2->date_created = base::Time::Now();
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+  forms.push_back(form2);
+  SetPasswordStoreData(forms);
+
+  SyncData add_data = CreateSyncData("def");
+  std::vector<autofill::PasswordForm*> updated_passwords(1, form1);
+  std::vector<autofill::PasswordForm*> deleted_passwords(1, form2);
+  verifier()->SetExpectedDBChanges(SyncDataList(1, add_data),
+                                   updated_passwords,
+                                   deleted_passwords,
+                                   password_store());
+  EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_));

[Ilya Sherman, 2014/02/07 22:56:35]

nit: Please move this to be roughly right above the call that triggers this
expectation.

[vasilii, 2014/02/10 19:06:50]

Done.

+
+  SyncChangeList list;
+  list.push_back(SyncChange(FROM_HERE,
+                            syncer::SyncChange::ACTION_ADD,
+                            add_data));
+  list.push_back(SyncChange(FROM_HERE,
+                            syncer::SyncChange::ACTION_UPDATE,
+                            CreateSyncData("abc")));
+  list.push_back(CreateSyncChange(*form2, syncer::SyncChange::ACTION_DELETE));
+  service()->ProcessSyncChanges(FROM_HERE, list);
+}
+
+// Retrives sync data from the model.
+TEST_F(PasswordSyncableServiceTest, GetAllSyncData) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;

[Ilya Sherman, 2014/02/07 22:56:35]

Ditto.

[vasilii, 2014/02/10 19:06:50]

Done.

+  form1->signon_realm = "abc";
+  form1->action = GURL("http://foo.com");
+  autofill::PasswordForm *form2 = new autofill::PasswordForm;

[Ilya Sherman, 2014/02/07 22:56:35]

Ditto.

[vasilii, 2014/02/10 19:06:50]

Done.

+  form2->signon_realm = "xyz";
+  form2->action = GURL("http://bar.com");
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+  forms.push_back(form2);
+  SetPasswordStoreData(forms);
+
+  SyncDataList expected_list;
+  expected_list.push_back(SyncDataFromPassword(*form1));
+  expected_list.push_back(SyncDataFromPassword(*form2));
+
+  verifier()->SetExpectedDBChanges(SyncDataList(),
+                                   std::vector<autofill::PasswordForm*>(),
+                                   std::vector<autofill::PasswordForm*>(),
+                                   password_store());
+
+  SyncDataList actual_list = service()->GetAllSyncData(syncer::PASSWORDS);
+  EXPECT_EQ(expected_list.size(), actual_list.size());
+  for (SyncDataList::iterator i(actual_list.begin()), j(expected_list.begin());
+       i != actual_list.end() && j != expected_list.end(); ++i, ++j) {
+    PasswordsEqual(GetPasswordSpecifics(*j), GetPasswordSpecifics(*i));
+  }
+}
+
+// Creates 2 PasswordSyncableService instances, merges the content of the first
+// one to the second one and back.
+TEST_F(PasswordSyncableServiceTest, MergeDataAndPushBack) {

[Nicolas Zea, 2014/02/07 21:16:47]

Nice.

[vasilii, 2014/02/10 19:06:50]

Done.

+  autofill::PasswordForm *form1 = new autofill::PasswordForm;

[Ilya Sherman, 2014/02/07 22:56:35]

Ditto.

[vasilii, 2014/02/10 19:06:50]

Done.

+  form1->signon_realm = "abc";
+  form1->action = GURL("http://foo.com");
+  SetPasswordStoreData(std::vector<autofill::PasswordForm*>(1, form1));
+
+  PasswordSyncableServiceWrapper other_service_wrapper;
+  other_service_wrapper.SetUp();
+  autofill::PasswordForm *form2 = new autofill::PasswordForm;
+  form2->signon_realm = "xyz";
+  form2->action = GURL("http://bar.com");
+  // SetPasswordStoreData will be called twice: from GetAllSyncData() and
+  // ProcessSyncChanges().
+  other_service_wrapper.SetPasswordStoreData(
+      std::vector<autofill::PasswordForm*>(1, form2));
+  other_service_wrapper.SetPasswordStoreData(
+      std::vector<autofill::PasswordForm*>(1,
+                                           new autofill::PasswordForm(*form2)));
+
+  verifier()->SetExpectedDBChanges(
+      SyncDataList(1, SyncDataFromPassword(*form2)),
+      std::vector<autofill::PasswordForm*>(),
+      std::vector<autofill::PasswordForm*>(),
+      password_store());
+  other_service_wrapper.verifier()->SetExpectedDBChanges(
+      SyncDataList(1, SyncDataFromPassword(*form1)),
+      std::vector<autofill::PasswordForm*>(),
+      std::vector<autofill::PasswordForm*>(),
+      other_service_wrapper.password_store());
+  EXPECT_CALL(*service(), NotifyPasswordStoreOfLoginChanges(_));
+  EXPECT_CALL(*other_service_wrapper.service(),
+              NotifyPasswordStoreOfLoginChanges(_));
+
+  service()->MergeDataAndStartSyncing(
+      syncer::PASSWORDS,
+      other_service_wrapper.service()->GetAllSyncData(syncer::PASSWORDS),
+      other_service_wrapper.service().PassAs<syncer::SyncChangeProcessor>(),
+      scoped_ptr<syncer::SyncErrorFactory>());
+  other_service_wrapper.TearDown();
 }
 
 }  // namespace