[Sync] Implementation of model association for passwords using sync API

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 fd6ceae2073f62b18646e4bc0ccd69541bee95c5..203fe0bc2d15375ccf1890cf2fabbcfc3785d6b1 100644
--- a/chrome/browser/password_manager/password_syncable_service.cc
+++ b/chrome/browser/password_manager/password_syncable_service.cc
@@ -20,31 +20,6 @@
 #include "sync/api/sync_error_factory.h"
 
 namespace {
-
-void ExtractPasswordFromSpecifics(
-     const sync_pb::PasswordSpecificsData& password,
-     autofill::PasswordForm* new_password) {
-  new_password->scheme =
-      static_cast<autofill::PasswordForm::Scheme>(password.scheme());
-  new_password->signon_realm = password.signon_realm();
-  new_password->origin = GURL(password.origin());
-  new_password->action = GURL(password.action());
-  new_password->username_element =
-      UTF8ToUTF16(password.username_element());
-  new_password->password_element =
-      UTF8ToUTF16(password.password_element());
-  new_password->username_value =
-      UTF8ToUTF16(password.username_value());
-  new_password->password_value =
-      UTF8ToUTF16(password.password_value());
-  new_password->ssl_valid = password.ssl_valid();
-  new_password->preferred = password.preferred();
-  new_password->date_created =
-      base::Time::FromInternalValue(password.date_created());
-  new_password->blacklisted_by_user =
-      password.blacklisted();
-}
-
 bool MergeLocalAndSyncPasswords(
     const sync_pb::PasswordSpecificsData& password,
     const autofill::PasswordForm& password_form,
@@ -75,7 +50,9 @@ bool MergeLocalAndSyncPasswords(
           password_form.date_created) {
     *new_password = password_form;
   } else {
-    ExtractPasswordFromSpecifics(password, new_password);
+    PasswordSyncableService::ExtractPasswordFromSpecifics(
+        password,
+        new_password);
   }
 
   return true;
@@ -89,6 +66,51 @@ PasswordSyncableService::PasswordSyncableService(
 
 PasswordSyncableService::~PasswordSyncableService() {}
 
+void PasswordSyncableService::CreateOrUpdateEntry(
+    const syncer::SyncData& data,
+    PasswordEntryMap* loaded_data,
+    std::vector<autofill::PasswordForm*>* new_entries,

[Nicolas Zea, 2013/10/15 22:04:10]

pass ScopedVectors instead?

[lipalani1, 2013/10/18 23:33:03]

Done.

+    std::vector<autofill::PasswordForm*>* udpated_entries) {

[Nicolas Zea, 2013/10/15 22:04:10]

udpated -> updated

[lipalani1, 2013/10/18 23:33:03]

Done.

+  const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
+  const sync_pb::PasswordSpecificsData& password_specifics(
+      specifics.password().client_only_encrypted_data());
+
+  std::string tag = MakeTag(password_specifics);
+
+  // Find if the data from sync is already in password store.
+  PasswordEntryMap::iterator it = loaded_data->find(tag);
+
+  if (it == loaded_data->end()) {
+    // The sync data is not in password store, so we need to create it in
+    // passowrd store. Add the entry to the new_entries list.

[Nicolas Zea, 2013/10/15 22:04:10]

passowrd -> password

[lipalani1, 2013/10/18 23:33:03]

Done.

+    autofill::PasswordForm* new_password = new autofill::PasswordForm();
+    ExtractPasswordFromSpecifics(password_specifics, new_password);
+    // The new_password pointer is owned by the caller.
+    new_entries->push_back(new_password);
+  } else {
+    // The entry is in password store. If the entries are not identical the
+    // entries need to be merged.
+    scoped_ptr<autofill::PasswordForm> new_password(
+        new autofill::PasswordForm());
+    if (MergeLocalAndSyncPasswords(password_specifics,
+                                   **(it->second.second),
+                       new_password.get())) {

[Nicolas Zea, 2013/10/15 22:04:10]

fix indent

[lipalani1, 2013/10/18 23:33:03]

Done.

+      delete *(it->second.second);
+      *(it->second.second) = new_password.release();
+      it->second.first = syncer::SyncChange::ACTION_UPDATE;
+
+      // Make a copy for updated list.
+      autofill::PasswordForm* updated_password = new autofill::PasswordForm();
+      *updated_password = **(it->second.second);
+      udpated_entries->push_back(updated_password);
+    } else {
+      // The entries are identical. Remove it from the |loaded_data| list to
+      // prevent it from being treated as an unassociated entry.
+      loaded_data->erase(it);
+    }
+  }
+}
+
 syncer::SyncMergeResult
 PasswordSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
@@ -98,9 +120,64 @@ PasswordSyncableService::MergeDataAndStartSyncing(
   syncer::SyncMergeResult merge_result(type);
   sync_error_factory_ = sync_error_factory.Pass();
   sync_processor_ = sync_processor.Pass();
-  merge_result.set_error(sync_error_factory->CreateAndUploadError(
-      FROM_HERE,
-      "Password Syncable Service Not Implemented."));
+
+  ScopedVector<autofill::PasswordForm> password_entries;
+  if (!password_store_->FillAutofillableLogins(&(password_entries.get())) ||
+      !password_store_->FillBlacklistLogins(&(password_entries.get()))) {
+    STLDeleteElements(&password_entries);
+
+    // 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);
+    merge_result.set_error(sync_error_factory->CreateAndUploadError(
+        FROM_HERE,
+        "Failed to get passwords from store."));
+    return merge_result;;
+  }
+
+  PasswordEntryMap new_db_entries;

[Nicolas Zea, 2013/10/15 22:04:10]

I think it's cleaner to track the entries separately from the changes. For
example, use an std::set to track the local entries that have been matched with
sync, and use a SyncChangeList to track the changes to sync. See for example
pref_model_associator.cc.

CreateOrUpdateEntry no longer has to have confusing lines like "delete
*(it->second.second)"

[lipalani1, 2013/10/18 23:33:03]

Possible I am missing something here:
delete *(it->second.second) - I have done this slightly differently now.(but it
is only marginally better :) **(it->second.second) = foo )

The logic to compute the un-synced db entries is by having all the db entries in
a set and removing the entries that are identical to sync entries and modifying
the entries with sync updates. Then this set is converted to a SyncChangeList.

To compute sync entries that are not in passwords db: Compare each entry in sync
db with the entries in password db and when not present add them to a vector,
when they are present but different add them to another vector. Then these lists
are used to populate the passwordstore.

The it->second.second is present because we use a single list to track both
updates and additions. Is your point to seperate them to 2 lists. Even if we did
that we still have to write it->second. and when creating the syncchangelist
from these we will have to have 2 ways, one for converting addtions to
syncchangelist and another for converting updates, as additions will be stored
in set and updates will be stored in vector. Refer to line 170 on how it is done
now. I am not opposed to this change though, let me know.
On 2013/10/15 22:04:10, Nicolas Zea wrote:

[Nicolas Zea, 2013/10/21 23:55:14]

I'm not really suggesting to have two lists for tracking changes, but rather
have a single SyncChangeList that gets passed to CreateOrUpdateEntry, which will
append an Update as necessary. Then, in the section later where you iterate
through new_db_entries, you can create ADDs for all the remaining entries. At
that point you can just pass new_changes directly to the ProcessSyncChanges
method.

You could keep the password entry map around if you like; it just would now be a
map of string to password form pointer

+  for (std::vector<autofill::PasswordForm*>::iterator
+       it = password_entries.begin();

[Nicolas Zea, 2013/10/15 22:04:10]

indent four more spaces

[lipalani1, 2013/10/18 23:33:03]

Done.

+       it != password_entries.end();
+       ++it) {
+     // We add all the db entries as |new_db_entries| initially. While
+     // association entries with matching sync entries will be removed and this
+     // list will only contain entries that are not in sync.
+     new_db_entries[MakeTag(**it)] =
+         std::make_pair(syncer::SyncChange::ACTION_ADD, it);
+  }
+
+  // List that contains the entries that are known only to sync.
+  ScopedVector<autofill::PasswordForm> new_synced_entries;
+
+  // List that contains the entries that are known to both sync and db but
+  // have to be updated in sync.
+  ScopedVector<autofill::PasswordForm> updated_sync_entries;
+  for (syncer::SyncDataList::const_iterator sync_iter =
+           initial_sync_data.begin();
+       sync_iter != initial_sync_data.end(); ++sync_iter) {
+    CreateOrUpdateEntry(*sync_iter,
+                        &new_db_entries,
+                        &(new_synced_entries.get()),
+                        &(updated_sync_entries.get()));
+  }
+
+  WriteToPasswordStore(&(new_synced_entries.get()),
+                       &(updated_sync_entries.get()));
+
+  syncer::SyncChangeList new_changes;
+  for (PasswordEntryMap::iterator it = new_db_entries.begin();
+       it != new_db_entries.end();
+       ++it) {
+    new_changes.push_back(syncer::SyncChange(FROM_HERE,
+                              it->second.first,
+                              CreateSyncData(**(it->second.second))));
+  }
+
+  merge_result.set_error(
+      sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes));
   return merge_result;
 }
 
@@ -145,11 +222,15 @@ bool PasswordSyncableService::WriteToPasswordStore(
       updated_entries->size() > 0) {
     // We have to notify password store observers of the change by hand since
     // we use internal password store interfaces to make changes synchronously.
-    password_store_->PostNotifyLoginsChanged();
+    NotifyPasswordStore();
   }
   return true;
 }
 
+void PasswordSyncableService::NotifyPasswordStore() {
+  password_store_->PostNotifyLoginsChanged();
+}
+
 syncer::SyncData PasswordSyncableService::CreateSyncData(
     const autofill::PasswordForm& password_form) {
   sync_pb::EntitySpecifics password_data;
@@ -211,3 +292,28 @@ std::string PasswordSyncableService::MakeTag(
                  password.signon_realm());
 }
 
+// static
+void PasswordSyncableService::ExtractPasswordFromSpecifics(
+     const sync_pb::PasswordSpecificsData& password,
+     autofill::PasswordForm* new_password) {
+  new_password->scheme =
+      static_cast<autofill::PasswordForm::Scheme>(password.scheme());
+  new_password->signon_realm = password.signon_realm();
+  new_password->origin = GURL(password.origin());
+  new_password->action = GURL(password.action());
+  new_password->username_element =
+      UTF8ToUTF16(password.username_element());
+  new_password->password_element =
+      UTF8ToUTF16(password.password_element());
+  new_password->username_value =
+      UTF8ToUTF16(password.username_value());
+  new_password->password_value =
+      UTF8ToUTF16(password.password_value());
+  new_password->ssl_valid = password.ssl_valid();
+  new_password->preferred = password.preferred();
+  new_password->date_created =
+      base::Time::FromInternalValue(password.date_created());
+  new_password->blacklisted_by_user =
+      password.blacklisted();
+}
+