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

components/autofill/core/browser/personal_data_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/core/browser/personal_data_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <list>
@@ skipping 993 matching lines @@
               if (a->IsVerified() != b->IsVerified())
                 return !a->IsVerified();
               return a->CompareFrecency(b.get(), comparison_time);
             });
 
   // Set to true if |existing_profiles| already contains an equivalent profile.
   bool matching_profile_found = false;
   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 &&
         comparator.AreMergeable(new_profile, *existing_profile) &&
         existing_profile->SaveAdditionalInfo(new_profile, app_locale)) {
       // Unverified profiles should always be updated with the newer data,
       // whereas verified profiles should only ever be overwritten by verified
       // data.  If an automatically aggregated profile would overwrite a
       // verified profile, just drop it.
       matching_profile_found = true;
@@ skipping 671 matching lines @@
       current_major_version) {
     DVLOG(1)
         << "Autofill profile de-duplication already performed for this version";
     return false;
   }
 
   DVLOG(1) << "Starting autofill profile de-duplication.";
   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())) {
       database_->RemoveAutofillProfile(profile->guid());
     } else {
       // Otherwise, update the profile in the database.
       database_->UpdateAutofillProfile(*profile);
     }
   }
 
+  UpdateCardsBillingAddressReference(guids_merge_map);
+
   // Set the pref to the current major version.
   pref_service_->SetInteger(prefs::kAutofillLastVersionDeduped,
                             current_major_version);
 
   // Refresh the local cache and send notifications to observers.
   Refresh();
 
   return true;
 }
 
 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());
 
   // Sort the profiles by frecency with all the verified profiles at the end.
   // That way the most relevant profiles will get merged into the less relevant
   // profiles, which keeps the syntax of the most relevant profiles data.
   // Verified profiles are put at the end because they do not merge into other
   // profiles, so the loop can be stopped when we reach those. However they need
   // to be in the vector because an unverified profile trying to merge into a
   // similar verified profile will be discarded.
@@ skipping 33 matching lines @@
       // Move on if the profiles are not mergeable.
       if (!comparator.AreMergeable(*existing_profile, *profile_to_merge))
         continue;
 
       // The profiles are found to be mergeable. Attempt to update the existing
       // profile. This returns true if the merge was successful, or if the
       // merge would have been successful but the existing profile IsVerified()
       // 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);
 
         // Now try to merge the new resulting profile with the rest of the
         // existing profiles.
         profile_to_merge = existing_profile;
 
         // Verified profiles do not get merged. Save some time by not
         // trying.
         if (profile_to_merge->IsVerified())
           break;
       }
     }
   }
   AutofillMetrics::LogNumberOfProfilesRemovedDuringDedupe(
       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