Address readability comments in PasswordSyncableService.

components/password_manager/core/browser/password_syncable_service.cc

Index: components/password_manager/core/browser/password_syncable_service.cc
diff --git a/components/password_manager/core/browser/password_syncable_service.cc b/components/password_manager/core/browser/password_syncable_service.cc
index 8cf38174c4a4eb9901884adcdcc1602c828ce28a..d2c9390f4705de48776accb084617acfef09a3e5 100644
--- a/components/password_manager/core/browser/password_syncable_service.cc
+++ b/components/password_manager/core/browser/password_syncable_service.cc
@@ -14,25 +14,13 @@
 #include "net/base/escape.h"
 #include "sync/api/sync_error_factory.h"
 
-namespace password_manager {
-
 namespace {
 
-// Describes the result of merging sync and local passwords.
-enum MergeResult {
-  IDENTICAL,
-  SYNC,
-  LOCAL,
-};
-
-// Merges the local and sync passwords and outputs the entry into
-// |new_password_form|. Returns MergeResult value describing the content of
-// |new_password_form|.
-MergeResult MergeLocalAndSyncPasswords(
+// Checks if |password_specifics| and |password_specifics| are equal
+// memberwise.
+bool AreLocalAndSyncPasswordsEqual(
     const sync_pb::PasswordSpecificsData& password_specifics,
-    const autofill::PasswordForm& password_form,
-    autofill::PasswordForm* new_password_form) {
-  DCHECK(new_password_form);
+    const autofill::PasswordForm& password_form) {
   if (password_form.scheme == password_specifics.scheme() &&
       password_form.signon_realm == password_specifics.signon_realm() &&
       password_form.origin.spec() == password_specifics.origin() &&
@@ -52,63 +40,38 @@ MergeResult MergeLocalAndSyncPasswords(
       password_form.blacklisted_by_user == password_specifics.blacklisted() &&
       password_form.type == password_specifics.type() &&
       password_form.times_used == password_specifics.times_used()) {
-    return IDENTICAL;
-  }
-
-  // If the passwords differ, take the one that was created more recently.
-  if (base::Time::FromInternalValue(password_specifics.date_created()) <
-          password_form.date_created) {
-    *new_password_form = password_form;
-    return LOCAL;
+    return true;
   }
-
-  PasswordFromSpecifics(password_specifics, new_password_form);
-  return SYNC;
-}
-
-std::string MakePasswordSyncTag(const std::string& origin_url,
-                                const std::string& username_element,
-                                const std::string& username_value,
-                                const std::string& password_element,
-                                const std::string& signon_realm) {
-  return net::EscapePath(origin_url) + "|" +
-         net::EscapePath(username_element) + "|" +
-         net::EscapePath(username_value) + "|" +
-         net::EscapePath(password_element) + "|" +
-         net::EscapePath(signon_realm);
+  return false;
 }
 
+// Internal MakePasswordSyncTag version for PasswordForm.
 std::string MakePasswordSyncTag(const autofill::PasswordForm& password) {
-  return MakePasswordSyncTag(password.origin.spec(),
-                             base::UTF16ToUTF8(password.username_element),
-                             base::UTF16ToUTF8(password.username_value),
-                             base::UTF16ToUTF8(password.password_element),
-                             password.signon_realm);
+  return net::EscapePath(password.origin.spec()) + "|" +
+         net::EscapePath(base::UTF16ToUTF8(password.username_element)) + "|" +
+         net::EscapePath(base::UTF16ToUTF8(password.username_value)) + "|" +
+         net::EscapePath(base::UTF16ToUTF8(password.password_element)) + "|" +
+         net::EscapePath(password.signon_realm);
 }
 
 syncer::SyncChange::SyncChangeType GetSyncChangeType(
-    PasswordStoreChange::Type type) {
+    password_manager::PasswordStoreChange::Type type) {
   switch (type) {
-    case PasswordStoreChange::ADD:
+    case password_manager::PasswordStoreChange::ADD:
       return syncer::SyncChange::ACTION_ADD;
-    case PasswordStoreChange::UPDATE:
+    case password_manager::PasswordStoreChange::UPDATE:
       return syncer::SyncChange::ACTION_UPDATE;
-    case PasswordStoreChange::REMOVE:
+    case password_manager::PasswordStoreChange::REMOVE:
       return syncer::SyncChange::ACTION_DELETE;
   }
   NOTREACHED();
   return syncer::SyncChange::ACTION_INVALID;
 }
 
-void AppendChanges(const PasswordStoreChangeList& new_changes,
-                   PasswordStoreChangeList* all_changes) {
-  all_changes->insert(all_changes->end(),
-                      new_changes.begin(),
-                      new_changes.end());
-}
-
 }  // namespace
 
+namespace password_manager {
+
 PasswordSyncableService::PasswordSyncableService(
     PasswordStoreSync* password_store)
     : password_store_(password_store),
@@ -175,8 +138,7 @@ syncer::SyncMergeResult PasswordSyncableService::MergeDataAndStartSyncing(
   merge_result.set_num_items_modified(updated_sync_entries.size());
 
   for (PasswordEntryMap::iterator it = new_local_entries.begin();
-       it != new_local_entries.end();
-       ++it) {
+       it != new_local_entries.end(); ++it) {
     updated_db_entries.push_back(
         syncer::SyncChange(FROM_HERE,
                            syncer::SyncChange::ACTION_ADD,
@@ -279,7 +241,7 @@ void PasswordSyncableService::ActOnPasswordStoreChanges(
        it != local_changes.end();
        ++it) {
     syncer::SyncData data = (it->type() == PasswordStoreChange::REMOVE ?
-        syncer::SyncData::CreateLocalDelete(MakePasswordSyncTag(it->form()),
+        syncer::SyncData::CreateLocalDelete(::MakePasswordSyncTag(it->form()),
                                             syncer::PASSWORDS) :
         SyncDataFromPassword(it->form()));
     sync_changes.push_back(
@@ -294,6 +256,75 @@ void PasswordSyncableService::InjectStartSyncFlare(
   flare_ = flare;
 }
 
+// static
+syncer::SyncData PasswordSyncableService::SyncDataFromPassword(
+    const autofill::PasswordForm& password_form) {
+  sync_pb::EntitySpecifics password_data;
+  sync_pb::PasswordSpecificsData* password_specifics =
+      password_data.mutable_password()->mutable_client_only_encrypted_data();
+#define CopyField(field) password_specifics->set_ ## field(password_form.field)
+#define CopyStringField(field) \
+    password_specifics->set_ ## field(base::UTF16ToUTF8(password_form.field))
+  CopyField(scheme);
+  CopyField(signon_realm);
+  password_specifics->set_origin(password_form.origin.spec());
+  password_specifics->set_action(password_form.action.spec());
+  CopyStringField(username_element);
+  CopyStringField(password_element);
+  CopyStringField(username_value);
+  CopyStringField(password_value);
+  CopyField(ssl_valid);
+  CopyField(preferred);
+  password_specifics->set_date_created(
+      password_form.date_created.ToInternalValue());
+  password_specifics->set_blacklisted(password_form.blacklisted_by_user);
+  CopyField(type);
+  CopyField(times_used);
+#undef CopyStringField
+#undef CopyField
+
+  std::string tag = MakePasswordSyncTag(*password_specifics);
+  return syncer::SyncData::CreateLocalData(tag, tag, password_data);
+}
+
+// static
+void PasswordSyncableService::PasswordFromSpecifics(
+    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 =
+      base::UTF8ToUTF16(password.username_element());
+  new_password->password_element =
+      base::UTF8ToUTF16(password.password_element());
+  new_password->username_value = base::UTF8ToUTF16(password.username_value());
+  new_password->password_value = base::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();
+  new_password->type =
+      static_cast<autofill::PasswordForm::Type>(password.type());
+  new_password->times_used = password.times_used();
+}
+
+// static
+std::string PasswordSyncableService::MakePasswordSyncTag(
+    const sync_pb::PasswordSpecificsData& password) {
+  autofill::PasswordForm password_form;
+  PasswordFromSpecifics(password, &password_form);
+  return ::MakePasswordSyncTag(password_form);
+}
+
+void PasswordSyncableService::NotifyPasswordStoreOfLoginChanges(
+    const PasswordStoreChangeList& changes) {
+  password_store_->NotifyLoginsChanged(changes);
+}
+
 bool PasswordSyncableService::ReadFromPasswordStore(
     ScopedVector<autofill::PasswordForm>* password_entries,
     PasswordEntryMap* passwords_entry_map) const {
@@ -314,8 +345,8 @@ bool PasswordSyncableService::ReadFromPasswordStore(
   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));
+     (*passwords_entry_map)[::MakePasswordSyncTag(*password_form)] =
+         password_form;
   }
 
   return true;
@@ -326,37 +357,15 @@ void PasswordSyncableService::WriteToPasswordStore(
     const PasswordForms& updated_entries,
     const PasswordForms& deleted_entries) {
   PasswordStoreChangeList changes;
-  for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           new_entries.begin();
-       it != new_entries.end();
-       ++it) {
-    AppendChanges(password_store_->AddLoginImpl(**it), &changes);
-  }
-
-  for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           updated_entries.begin();
-       it != updated_entries.end();
-       ++it) {
-    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);
-  }
+  AppendChanges(&PasswordStoreSync::AddLoginImpl, new_entries, &changes);
+  AppendChanges(&PasswordStoreSync::UpdateLoginImpl, updated_entries, &changes);
+  AppendChanges(&PasswordStoreSync::RemoveLoginImpl, deleted_entries, &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);
 }
 
-void PasswordSyncableService::NotifyPasswordStoreOfLoginChanges(
-    const PasswordStoreChangeList& changes) {
-  password_store_->NotifyLoginsChanged(changes);
-}
-
 void PasswordSyncableService::CreateOrUpdateEntry(
     const syncer::SyncData& data,
     PasswordEntryMap* umatched_data_from_password_db,
@@ -382,22 +391,21 @@ void PasswordSyncableService::CreateOrUpdateEntry(
   } else {
     // The entry is in password store. If the entries are not identical, then
     // the entries need to be merged.
-    scoped_ptr<autofill::PasswordForm> new_password(new autofill::PasswordForm);
-    switch (MergeLocalAndSyncPasswords(password_specifics,
-                                       *existing_local_entry_iter->second,
-                                       new_password.get())) {
-      case IDENTICAL:
-        break;
-      case SYNC:
-        new_password->date_synced = time_now;
-        updated_sync_entries->push_back(new_password.release());
-        break;
-      case LOCAL:
+    // If the passwords differ, take the one that was created more recently.
+    const autofill::PasswordForm& password_form =
+        *existing_local_entry_iter->second;
+    if (!AreLocalAndSyncPasswordsEqual(password_specifics, password_form)) {
+      if (base::Time::FromInternalValue(password_specifics.date_created()) <
+          password_form.date_created) {
         updated_db_entries->push_back(
             syncer::SyncChange(FROM_HERE,
                                syncer::SyncChange::ACTION_UPDATE,
-                               SyncDataFromPassword(*new_password)));
-        break;
+                               SyncDataFromPassword(password_form)));
+      } else {
+        updated_sync_entries->push_back(new autofill::PasswordForm);
+        PasswordFromSpecifics(password_specifics, updated_sync_entries->back());
+        updated_sync_entries->back()->date_synced = time_now;
+      }
     }
     // 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
@@ -406,65 +414,18 @@ void PasswordSyncableService::CreateOrUpdateEntry(
   }
 }
 
-syncer::SyncData SyncDataFromPassword(
-    const autofill::PasswordForm& password_form) {
-  sync_pb::EntitySpecifics password_data;
-  sync_pb::PasswordSpecificsData* password_specifics =
-      password_data.mutable_password()->mutable_client_only_encrypted_data();
-  password_specifics->set_scheme(password_form.scheme);
-  password_specifics->set_signon_realm(password_form.signon_realm);
-  password_specifics->set_origin(password_form.origin.spec());
-  password_specifics->set_action(password_form.action.spec());
-  password_specifics->set_username_element(
-      base::UTF16ToUTF8(password_form.username_element));
-  password_specifics->set_password_element(
-      base::UTF16ToUTF8(password_form.password_element));
-  password_specifics->set_username_value(
-      base::UTF16ToUTF8(password_form.username_value));
-  password_specifics->set_password_value(
-      base::UTF16ToUTF8(password_form.password_value));
-  password_specifics->set_ssl_valid(password_form.ssl_valid);
-  password_specifics->set_preferred(password_form.preferred);
-  password_specifics->set_date_created(
-      password_form.date_created.ToInternalValue());
-  password_specifics->set_blacklisted(password_form.blacklisted_by_user);
-  password_specifics->set_type(password_form.type);
-  password_specifics->set_times_used(password_form.times_used);
-
-  std::string tag = MakePasswordSyncTag(*password_specifics);
-  return syncer::SyncData::CreateLocalData(tag, tag, password_data);
-}
-
-void PasswordFromSpecifics(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 =
-      base::UTF8ToUTF16(password.username_element());
-  new_password->password_element =
-      base::UTF8ToUTF16(password.password_element());
-  new_password->username_value = base::UTF8ToUTF16(password.username_value());
-  new_password->password_value = base::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();
-  new_password->type =
-      static_cast<autofill::PasswordForm::Type>(password.type());
-  new_password->times_used = password.times_used();
-}
-
-std::string MakePasswordSyncTag(
-    const sync_pb::PasswordSpecificsData& password) {
-  return MakePasswordSyncTag(password.origin(),
-                             password.username_element(),
-                             password.username_value(),
-                             password.password_element(),
-                             password.signon_realm());
+void PasswordSyncableService::AppendChanges(
+    PasswordStoreChangeList (password_manager::PasswordStoreSync::*modify)(
+        const autofill::PasswordForm& form),
+    const PasswordForms& entries,
+    PasswordStoreChangeList* all_changes) {
+  for (PasswordForms::const_iterator it = entries.begin();
+       it != entries.end(); ++it) {
+    PasswordStoreChangeList new_changes = (password_store_->*modify)(**it);
+    all_changes->insert(all_changes->end(),
+                        new_changes.begin(),
+                        new_changes.end());
+  }
 }
 
 }  // namespace password_manager