[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 67355e2a9668629a1af517a96612ca85160b350c..510b799cf6d247c7fea6a4f3da2abcf8bd18786e 100644
--- a/chrome/browser/password_manager/password_syncable_service.cc
+++ b/chrome/browser/password_manager/password_syncable_service.cc
@@ -5,6 +5,9 @@
 #include "chrome/browser/password_manager/password_syncable_service.h"
 
 #include "base/location.h"
+#include "base/memory/scoped_vector.h"
+#include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_store.h"
 #include "components/autofill/core/common/password_form.h"
@@ -13,32 +16,6 @@
 
 namespace {
 
-// Converts the |PasswordSpecifics| obtained from sync to an
-// object of type |PasswordForm|.
-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();
-}
-
 // Merges the sync password (obtained from the password specifics) and
 // local password and stores the output in the |new_password_form| pointer.
 bool MergeLocalAndSyncPasswords(
@@ -71,7 +48,9 @@ bool MergeLocalAndSyncPasswords(
           password_form.date_created) {
     *new_password_form = password_form;
   } else {
-    ExtractPasswordFromSpecifics(password_specifics, new_password_form);
+    PasswordSyncableService::ExtractPasswordFromSpecifics(
+        password_specifics,
+        new_password_form);
   }
 
   return true;
@@ -86,6 +65,50 @@ PasswordSyncableService::PasswordSyncableService(
 
 PasswordSyncableService::~PasswordSyncableService() {}
 
+void PasswordSyncableService::CreateOrUpdateEntry(
+    const syncer::SyncData& data,
+    PasswordEntryMap* loaded_data,
+    ScopedVector<autofill::PasswordForm>* new_entries,
+    ScopedVector<autofill::PasswordForm>* updated_entries,
+    syncer::SyncChangeList* updated_db_entries) {
+  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
+    // password store. Add the entry to the new_entries list.
+    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),
+                                   new_password.get())) {
+      updated_db_entries->push_back(
+          syncer::SyncChange(FROM_HERE,
+                             syncer::SyncChange::ACTION_UPDATE,
+                             CreateSyncData(*(new_password.get()))));
+
+      updated_entries->push_back(new_password.release());
+    }
+    // Remove the entry from the entry map to indicate a match has been found.
+    // Entries that remain in the map at the end of associating all sync entries
+    // will be treated as additions that needs to be propagated to sync.
+    loaded_data->erase(it);
+  }
+}
+
 syncer::SyncMergeResult
 PasswordSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
@@ -96,9 +119,63 @@ PasswordSyncableService::MergeDataAndStartSyncing(
   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()))) {
+    // 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;
+  for (std::vector<autofill::PasswordForm*>::iterator
+           it = password_entries.begin();
+       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)] = it;
+  }
+
+  // List that contains the entries that are known only to sync.
+  ScopedVector<autofill::PasswordForm> new_synced_entries;

[Nicolas Zea, 2013/10/25 20:20:13]

nit: I think the name here would better represent whether or not the data is new
to the db. In other words, because the data is known only to sync, it should be
added to a list of "new_db_entries". Similarly, the variable below would be
"updated_db_entries".

[lipalani1, 2013/10/29 20:16:36]

May be I am missing something here:
The names refer to the state of the item in the respective stores(sync or
local). new_sync_entries are items that are new in sync sync db. 

there is already a variable above called new_db_entries(which are items new in
passwords db!)

I suppose you mean to make them refer to what action we will do on those
items.(i.e., switch these names) I am OK either way, let me know.

My assumption is with the comment above each variable it should be easy to
follow.
On 2013/10/25 20:20:13, Nicolas Zea wrote:

+
+  // 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;
+
+  // Changes from password db that needs to be propagated to sync.
+  syncer::SyncChangeList db_changes;
+  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,
+                        &updated_sync_entries,
+                        &db_changes);
+  }
+
+  WriteToPasswordStore(new_synced_entries.get(),
+                       updated_sync_entries.get());
+
+  for (PasswordEntryMap::iterator it = new_db_entries.begin();
+       it != new_db_entries.end();
+       ++it) {
+    db_changes.push_back(syncer::SyncChange(FROM_HERE,
+                              syncer::SyncChange::ACTION_ADD,
+                              CreateSyncData(**(it->second))));
+  }
+
+  merge_result.set_error(

[Nicolas Zea, 2013/10/25 20:20:13]

could you also fill calculate the other variables in merge result? (items
before, after, added, etc.)

[lipalani1, 2013/10/29 20:16:36]

Done.

+      sync_processor_->ProcessSyncChanges(FROM_HERE, db_changes));
   return merge_result;
 }
 
@@ -122,29 +199,33 @@ syncer::SyncError PasswordSyncableService::ProcessSyncChanges(
 }
 
 void PasswordSyncableService::WriteToPasswordStore(
-    PasswordForms* new_entries,
-    PasswordForms* updated_entries) {
+    const PasswordForms& new_entries,
+    const PasswordForms& updated_entries) {
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           new_entries->begin();
-       it != new_entries->end();
+           new_entries.begin();
+       it != new_entries.end();
        ++it) {
     password_store_->AddLoginImpl(**it);
   }
 
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           updated_entries->begin();
-       it != updated_entries->end();
+           updated_entries.begin();
+       it != updated_entries.end();
        ++it) {
     password_store_->UpdateLoginImpl(**it);
   }
 
-  if (!new_entries->empty() || !updated_entries->empty()) {
+  if (!new_entries.empty() || !updated_entries.empty()) {
     // 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();
   }
 }
 
+void PasswordSyncableService::NotifyPasswordStore() {
+  password_store_->PostNotifyLoginsChanged();
+}
+
 syncer::SyncData PasswordSyncableService::CreateSyncData(
     const autofill::PasswordForm& password_form) {
   sync_pb::EntitySpecifics password_data;
@@ -206,3 +287,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();
+}
+