[Autofill] Dedupe similar profiles on insertion.

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 e2b09f90ad65be4e5dab01af33d45e8352c233c5..dc1300d765a788fac47c760bb97142475e42f5a2 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -863,7 +863,6 @@ bool PersonalDataManager::IsValidLearnableProfile(
   return true;
 }
 
-// static
 std::string PersonalDataManager::MergeProfile(

[Mathieu, 2016/06/08 18:32:24]

It feels weird to remove the static property here. Can we make
profile_guids_to_delete be an argument, eventually populated by MergeProfile?
The caller of MergeProfile (also in PDM) could be responsible for deleting those
guids. Then all your functions could be static!

Would also rename MergeProfile to something else if it does a little more than
merge.

[sebsg, 2016/06/08 21:58:46]

I like the idea. Would you mind if I did it in another CL? I would probably put
the delete the code near or inside SetProfiles() and this method looks like it
needs a good refactoring.

I would also change a bunch of method names. For example
AutofillProfile::OverwriteWith should probably be call MergeWith.
AutofillProfile::SaveAdditionalInfo should be renamed with somehting like
AreMergeable and make use of Roger's new API. Finally, I would not call the
Merging logic inside of SaveAdditionalInfo/AreMergeable. This would allow for
better testing and also to make a difference between profiles that we thought
were not mergeable and profiles that failed merging.

     const AutofillProfile& new_profile,
     std::vector<AutofillProfile*> existing_profiles,
@@ -896,6 +895,10 @@ std::string PersonalDataManager::MergeProfile(
       matching_profile_found = true;
       guid = existing_profile->guid();
 
+      // Look for duplicates of |existing_profile| to merge into.
+      FindMergeAndDeleteDuplicateProfiles(existing_profiles, existing_profile,
+                                          app_locale);
+
       // We set the modification date so that immediate requests for profiles
       // will properly reflect the fact that this profile has been modified
       // recently. After writing to the database and refreshing the local copies
@@ -1512,4 +1515,45 @@ std::vector<Suggestion> PersonalDataManager::GetSuggestionsForCards(
   return suggestions;
 }
 
+void PersonalDataManager::FindMergeAndDeleteDuplicateProfiles(
+    const std::vector<AutofillProfile*>& existing_profiles,
+    AutofillProfile* profile_to_merge,
+    const std::string& app_locale) {
+  std::vector<std::string> profile_guids_to_delete;
+
+  FindAndMergeDuplicateProfiles(existing_profiles, profile_to_merge,
+                                &profile_guids_to_delete, app_locale);
+
+  // Delete the duplicate profiles.
+  for (std::string profile_guid_to_delete : profile_guids_to_delete) {
+    RemoveByGUID(profile_guid_to_delete);
+  }
+}
+
+void PersonalDataManager::FindAndMergeDuplicateProfiles(
+    const std::vector<AutofillProfile*>& existing_profiles,
+    AutofillProfile* profile_to_merge,
+    std::vector<std::string>* profile_guids_to_delete,
+    const std::string& app_locale) {
+  for (AutofillProfile* existing_profile : existing_profiles) {
+    // Don't try to merge a profile with itself or with any profile with a
+    // different PrimaryValue.
+    if (existing_profile->guid() != profile_to_merge->guid() &&
+        AutofillProfile::CanonicalizeProfileString(

[Mathieu, 2016/06/08 18:32:24]

Using the expanded PrimaryValue feels like a hack (with effects on other users
of PrimaryValue). Have a look (and review) Roger's CL which introduces
IsMergeable, which is a better API.

[sebsg, 2016/06/08 21:58:46]

The reason why I changed PrimaryValue was to fix a very subtle bug where the
canonicalized value of the similar profile with different syntax would not be
considered similar. I don't think this should affect other users of
PrimaryValue, since I'm only adding spaces between the fields.

The reason I use PrimaryValue here is to not call SaveAdditionalInfo on profiles
that are clearly not similar (eg different names or addresses). When Roger's CL
lands, I want to actually remove SaveAdditionalInfo and use his API instead.
Since it's super efficient at returning false for different profiles, I would
also remove the call to PrimaryValue here). 

+            existing_profile->PrimaryValue()) ==
+            AutofillProfile::CanonicalizeProfileString(
+                profile_to_merge->PrimaryValue())) {
+      if (existing_profile->SaveAdditionalInfo(*profile_to_merge, app_locale)) {
+        // Since |profile_to_merge| was a duplicate of |existing_profile| and
+        // was merged sucessfully, it can now be deleted.
+        profile_guids_to_delete->push_back(profile_to_merge->guid());
+
+        // Now try to merge the new resulting profile with the rest of the
+        // existing profiles.
+        profile_to_merge = existing_profile;
+      }
+    }
+  }
+}
+
 }  // namespace autofill