[Autofill] Dedupe similar profiles on insertion.

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>
 #include <map>
@@ skipping 845 matching lines @@
   if (profile.IsPresentButInvalid(ADDRESS_HOME_STATE))
     return false;
 
   // Reject profiles with invalid US zip information.
   if (profile.IsPresentButInvalid(ADDRESS_HOME_ZIP))
     return false;
 
   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,
     const std::string& app_locale,
     std::vector<AutofillProfile>* merged_profiles) {
   merged_profiles->clear();
 
   // Sort the existing profiles in decreasing order of frecency, so the "best"
   // profiles are checked first.
   base::Time comparison_time = base::Time::Now();
   std::sort(existing_profiles.begin(), existing_profiles.end(),
@@ skipping 11 matching lines @@
   for (AutofillProfile* existing_profile : existing_profiles) {
     if (!matching_profile_found && !new_profile.PrimaryValue().empty() &&
         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;
       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
       // the profile will have a very slightly newer time reflecting what's
       // actually stored in the database.
       existing_profile->set_modification_date(base::Time::Now());
     }
     merged_profiles->push_back(*existing_profile);
   }
 
@@ skipping 596 matching lines @@
   if (IsFeatureSubstringMatchEnabled()) {
     std::stable_sort(suggestions.begin(), suggestions.end(),
                      [](const Suggestion& a, const Suggestion& b) {
                        return a.match < b.match;
                      });
   }
 
   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