[Autofill] Transfer cards' billing address id when deduping profiles.

components/autofill/core/browser/personal_data_manager.cc

Index: components/autofill/core/browser/personal_data_manager.cc
diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc
index 73ba1d3517992601907c047fba18dcceae0aa623..39d2082ec25bfa41de7ea64f8cf5c5d15eabd041 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -1011,7 +1011,8 @@ std::string PersonalDataManager::MergeProfile(
   std::string guid = new_profile.guid();
 
   // If we have already saved this address, merge in any missing values.
-  // Only merge with the first match.
+  // Only merge with the first match. Merging the new profile into the existing
+  // one preserves the validity of credit card's billing address reference.
   AutofillProfileComparator comparator(app_locale);
   for (const auto& existing_profile : *existing_profiles) {
     if (!matching_profile_found &&
@@ -1703,9 +1704,13 @@ bool PersonalDataManager::ApplyDedupingRoutine() {
   std::unordered_set<AutofillProfile*> profiles_to_delete;
   profiles_to_delete.reserve(web_profiles_.size());
 
-  DedupeProfiles(&web_profiles_, &profiles_to_delete);
+  // Create the map used to update credit card's billing addresses after the
+  // dedupe.
+  std::unordered_map<std::string, std::string> guids_merge_map;
 
-  // Apply the changes to the database.
+  DedupeProfiles(&web_profiles_, &profiles_to_delete, &guids_merge_map);
+
+  // Apply the profile changes to the database.
   for (const auto& profile : web_profiles_) {
     // If the profile was set to be deleted, remove it from the database.
     if (profiles_to_delete.count(profile.get())) {
@@ -1716,6 +1721,8 @@ bool PersonalDataManager::ApplyDedupingRoutine() {
     }
   }
 
+  UpdateCardsBillingAddressReference(guids_merge_map);
+
   // Set the pref to the current major version.
   pref_service_->SetInteger(prefs::kAutofillLastVersionDeduped,
                             current_major_version);
@@ -1728,7 +1735,8 @@ bool PersonalDataManager::ApplyDedupingRoutine() {
 
 void PersonalDataManager::DedupeProfiles(
     std::vector<std::unique_ptr<AutofillProfile>>* existing_profiles,
-    std::unordered_set<AutofillProfile*>* profiles_to_delete) {
+    std::unordered_set<AutofillProfile*>* profiles_to_delete,
+    std::unordered_map<std::string, std::string>* guids_merge_map) {
   AutofillMetrics::LogNumberOfProfilesConsideredForDedupe(
       existing_profiles->size());
 
@@ -1782,6 +1790,10 @@ void PersonalDataManager::DedupeProfiles(
       // and will not accept updates from profile_to_merge.
       if (existing_profile->SaveAdditionalInfo(*profile_to_merge,
                                                app_locale_)) {
+        // Keep track of the merges to update cards billing address references.

[Mathieu, 2016/12/02 21:12:39]

Make the comment more specific to the reader: Keep track that
|profile_to_merge|'s GUID should now point to |existing_profile|'s GUID when
credit card billing address associations are updated.

[sebsg, 2016/12/02 21:46:34]

Done.

+        guids_merge_map->insert(std::pair<std::string, std::string>(
+            profile_to_merge->guid(), existing_profile->guid()));
+
         // Since |profile_to_merge| was a duplicate of |existing_profile|
         // and was merged successfully, it can now be deleted.
         profiles_to_delete->insert(profile_to_merge);
@@ -1801,4 +1813,25 @@ void PersonalDataManager::DedupeProfiles(
       profiles_to_delete->size());
 }
 
+void PersonalDataManager::UpdateCardsBillingAddressReference(
+    const std::unordered_map<std::string, std::string>& guids_merge_map) {
+  for (auto& credit_card : local_credit_cards_) {

[Mathieu, 2016/12/02 21:12:39]

The ASCII art that you have in the test would be useful here, too.

[sebsg, 2016/12/02 21:46:34]

Done.

+    bool was_modified = false;

[Mathieu, 2016/12/02 21:12:39]

nit: bring closer to the while loop

[sebsg, 2016/12/02 21:46:34]

Done.

+
+    // If the billing address profile associated with the card has been merged,
+    // replace it by the id of the profile in which it was merged. Repeat the
+    // process until the billing address has not been merged into another one.
+    while (guids_merge_map.count(credit_card->billing_address_id())) {

[Mathieu, 2016/12/02 21:12:39]

what if there is no billing_address_id? Should we just let this code handle it
implicitely or be more explicit about it? I vote explicit by adding a check in
the first lines of the for loop, above.

[Roger McFarlane (Chromium), 2016/12/02 21:23:05]

the count() and the at() both do a map lookup. Maybe do the lookup once with
find and reuse the resulting iterator?

while (true) {
  auto it = guids_merge_map.find(credit_card->billing_address_id());
  if (it == lguilds_merge_map.end()) break;
  was_modified = true;
  credit_card->set_billing_address_id(it->second);
}

[sebsg, 2016/12/02 21:46:34]

Sure, I thought the other way was more readable, but your suggestion is more
efficient.

[sebsg, 2016/12/02 21:46:34]

Done.

+      was_modified = true;
+      credit_card->set_billing_address_id(
+          guids_merge_map.at(credit_card->billing_address_id()));

[Mathieu, 2016/12/02 21:12:39]

what about guids_merge_map[credit_card->billing_address_id()]? It should be
equivalent to "at" here but I think we use [] mostly.

[sebsg, 2016/12/02 21:46:33]

In this use case they would be equivalent, but I chose "at" because it's const.

+    }
+
+    // If the card was modified, apply the changes to the database.
+    if (was_modified) {
+      database_->UpdateCreditCard(*credit_card);
+    }
+  }
+}
+
 }  // namespace autofill