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

chrome/browser/password_manager/password_syncable_service.cc

Index: chrome/browser/password_manager/password_syncable_service.cc
diff --git a/chrome/browser/password_manager/password_syncable_service.cc b/chrome/browser/password_manager/password_syncable_service.cc
index aa4b3aaf14921b1ff13a9288406f2ea41be08715..cee4b6445c399f3de4f1c389db6d8ccf35abf9b5 100644
--- a/chrome/browser/password_manager/password_syncable_service.cc
+++ b/chrome/browser/password_manager/password_syncable_service.cc
@@ -115,29 +115,15 @@ syncer::SyncMergeResult PasswordSyncableService::MergeDataAndStartSyncing(
   sync_processor_ = sync_processor.Pass();
 
   ScopedVector<autofill::PasswordForm> password_entries;
-  if (!password_store_->FillAutofillableLogins(&password_entries.get())) {
-    // Password store often fails to load passwords. Track failures with UMA.
-    // (http://crbug.com/249000)
-    UMA_HISTOGRAM_ENUMERATION("Sync.LocalDataFailedToLoad",
-                              syncer::PASSWORDS,
-                              syncer::MODEL_TYPE_COUNT);
+  PasswordEntryMap new_local_entries;
+  if (!ReadFromPasswordStore(&password_entries, &new_local_entries)) {
+    DCHECK(sync_error_factory_);
     merge_result.set_error(sync_error_factory_->CreateAndUploadError(
         FROM_HERE,
         "Failed to get passwords from store."));
     return merge_result;
   }
 
-  PasswordEntryMap new_local_entries;
-  for (PasswordForms::iterator it = password_entries.begin();
-       it != password_entries.end(); ++it) {
-     autofill::PasswordForm* password_form = *it;
-     // We add all the db entries as |new_local_entries| initially. During
-     // model association entries that match a sync entry will be
-     // removed and this list will only contain entries that are not in sync.

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

This was a useful comment, IMO.  It would be nice to preserve it.

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

Done.

-     new_local_entries.insert(
-         std::make_pair(MakePasswordSyncTag(*password_form), password_form));
-  }
-
   merge_result.set_num_items_before_association(new_local_entries.size());
 
   // List that contains the entries that are known only to sync.
@@ -160,7 +146,8 @@ syncer::SyncMergeResult PasswordSyncableService::MergeDataAndStartSyncing(
   }
 
   WriteToPasswordStore(new_sync_entries.get(),
-                       updated_sync_entries.get());
+                       updated_sync_entries.get(),
+                       PasswordForms());
 
   merge_result.set_num_items_after_association(
       merge_result.num_items_before_association() + new_sync_entries.size());
@@ -190,18 +177,79 @@ void PasswordSyncableService::StopSyncing(syncer::ModelType type) {
 
 syncer::SyncDataList PasswordSyncableService::GetAllSyncData(
     syncer::ModelType type) const {
+  DCHECK_EQ(syncer::PASSWORDS, type);
+  ScopedVector<autofill::PasswordForm> password_entries;
+  ReadFromPasswordStore(&password_entries, NULL);
+
   syncer::SyncDataList sync_data;
+  for (PasswordForms::iterator i = password_entries.begin();

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

nit: i -> it

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

Done.

+       i != password_entries.end(); ++i) {
+    sync_data.push_back(SyncDataFromPassword(**i));
+  }
   return sync_data;
 }
 
 syncer::SyncError PasswordSyncableService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
-  syncer::SyncError error(FROM_HERE,
-                          syncer::SyncError::UNRECOVERABLE_ERROR,
-                          "Password Syncable Service Not Implemented.",
-                          syncer::PASSWORDS);
-  return error;
+  // The |db_entries_map| and associated vectors are filled only in case of
+  // add/update change.
+  PasswordEntryMap db_entries_map;
+  ScopedVector<autofill::PasswordForm> password_db_entries;
+  ScopedVector<autofill::PasswordForm> new_sync_entries;
+  ScopedVector<autofill::PasswordForm> updated_sync_entries;
+  ScopedVector<autofill::PasswordForm> deleted_entries;
+  bool has_read_passwords = false;
+
+  for (syncer::SyncChangeList::const_iterator it = change_list.begin();
+       it != change_list.end();
+       ++it) {
+    switch (it->change_type()) {
+      case syncer::SyncChange::ACTION_ADD:
+      case syncer::SyncChange::ACTION_UPDATE: {
+        if (!has_read_passwords) {
+          if (ReadFromPasswordStore(&password_db_entries,

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

It seems expensive to read all passwords from the password store every time a
sync change arrives. Can we do better? Either by caching the metadata about
passwords, or adding support for only reading the individual password affected?

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

FWIW, my vote is against caching unless we really can't get the desired
performance without it -- I've found that it tends to introduce lots of
complexity and, consequently, buginess.  It might indeed make sense to only read
the individual passwords that are affected.

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

This implementation isn't slower than the existing one. I'm not sure that this
class is a proper place for the cache. Did you mean reading individual passwords
through GetLoginsImpl()? Should it be part of this CL?

+                                    &db_entries_map)) {
+            has_read_passwords = true;
+          } else {
+            return sync_error_factory_->CreateAndUploadError(
+                FROM_HERE,
+                "Failed to get passwords from store.");
+          }
+        }
+        syncer::SyncChangeList sync_changes;
+
+        // This item from sync will be compared to the data in passwords db and
+        // |new_sync_entries| or |updated_sync_entries| will be updated based on
+        // whether this sync item is an addition or update.

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

Hmm, why is this needed?  That is, why is it not safe to assume that the Sync
change type is correct at this point?

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

It's safe. I just reuse the existing method.

+        CreateOrUpdateEntry(it->sync_data(),
+                            &db_entries_map,
+                            &new_sync_entries,
+                            &updated_sync_entries,
+                            &sync_changes);
+        break;
+      }

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

nit: Leave a blank line between cases, IMO.

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

Done.

+      case syncer::SyncChange::ACTION_DELETE: {
+        const sync_pb::EntitySpecifics& specifics =
+            it->sync_data().GetSpecifics();
+        const sync_pb::PasswordSpecificsData& password_specifics(
+            specifics.password().client_only_encrypted_data());
+        autofill::PasswordForm* new_password = new autofill::PasswordForm;

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

nit: I prefer to immediately wrap all |new|s in scoped_ptr's or the like, and
then explicitly transfer ownership.  This is a nice habit to get into for quick
and easy correct memory management.

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

Done.

+        PasswordFromSpecifics(password_specifics, new_password);
+        deleted_entries.push_back(new_password);
+        break;
+      }
+      default: {

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

nit: Rather than a default case, please have an explicit case for
ACTION_INVALID.  That way, if the enum ever grows, the compiler will remind us
to think about updating this switch stmt.

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

Done.

+        return sync_error_factory_->CreateAndUploadError(
+            FROM_HERE,
+            "Failed to process sync changes for passwords datatype.");
+      }

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

nit: No need for curly braces.

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

Done.

+    }
+  }
+  WriteToPasswordStore(new_sync_entries.get(),
+                       updated_sync_entries.get(),
+                       deleted_entries.get());
+  return syncer::SyncError();
 }
 
 void PasswordSyncableService::ActOnPasswordStoreChanges(
@@ -220,9 +268,37 @@ void PasswordSyncableService::ActOnPasswordStoreChanges(
   sync_processor_->ProcessSyncChanges(FROM_HERE, sync_changes);
 }
 
+bool PasswordSyncableService::ReadFromPasswordStore(
+    ScopedVector<autofill::PasswordForm>* password_entries,
+    PasswordEntryMap* passwords_entry_map) const {
+  DCHECK(password_entries);

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

nit: No need for this, IMO, since you dereference the variable on the next line.

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

For better log readability?

+  if (!password_store_->FillAutofillableLogins(&password_entries->get()) ||
+      !password_store_->FillBlacklistLogins(&password_entries->get())) {

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

Have you added test coverage for blacklisted logins?

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

I modified PasswordSyncableServiceTest.ProcessSyncChanges and
PasswordSyncableServiceTest.GetAllSyncData

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

Lovely, thanks :)

+    // Password store often fails to load passwords. Track failures with UMA.
+    // (http://crbug.com/249000)
+    UMA_HISTOGRAM_ENUMERATION("Sync.LocalDataFailedToLoad",
+                              ModelTypeToHistogramInt(syncer::PASSWORDS),
+                              syncer::MODEL_TYPE_COUNT);
+    return false;
+  }
+
+  if (!passwords_entry_map)
+    return true;
+
+  for (PasswordForms::iterator it = password_entries->begin();
+       it != password_entries->end(); ++it) {
+     autofill::PasswordForm* password_form = *it;
+     passwords_entry_map->insert(
+         std::make_pair(MakePasswordSyncTag(*password_form), password_form));
+  }
+
+  return true;
+}
+
 void PasswordSyncableService::WriteToPasswordStore(
     const PasswordForms& new_entries,
-    const PasswordForms& updated_entries) {
+    const PasswordForms& updated_entries,
+    const PasswordForms& deleted_entries) {
   PasswordStoreChangeList changes;
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
            new_entries.begin();
@@ -238,6 +314,13 @@ void PasswordSyncableService::WriteToPasswordStore(
     AppendChanges(password_store_->UpdateLoginImpl(**it), &changes);
   }
 
+  for (std::vector<autofill::PasswordForm*>::const_iterator it =
+           deleted_entries.begin();
+       it != deleted_entries.end();
+       ++it) {
+    AppendChanges(password_store_->RemoveLoginImpl(**it), &changes);
+  }
+
   // We have to notify password store observers of the change by hand since
   // we use internal password store interfaces to make changes synchronously.
   NotifyPasswordStoreOfLoginChanges(changes);