Use AutofillProfileComparator in place of ad-hoc merge logic.

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>
 #include <string>
 #include <utility>
 
 #include "base/i18n/case_conversion.h"
 #include "base/i18n/timezone.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "components/autofill/core/browser/address_i18n.h"
 #include "components/autofill/core/browser/autofill-inl.h"
 #include "components/autofill/core/browser/autofill_country.h"
 #include "components/autofill/core/browser/autofill_experiments.h"
 #include "components/autofill/core/browser/autofill_field.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
+#include "components/autofill/core/browser/autofill_profile_comparator.h"
 #include "components/autofill/core/browser/country_data.h"
 #include "components/autofill/core/browser/country_names.h"
 #include "components/autofill/core/browser/form_structure.h"
 #include "components/autofill/core/browser/personal_data_manager_observer.h"
 #include "components/autofill/core/browser/phone_number.h"
 #include "components/autofill/core/browser/phone_number_i18n.h"
 #include "components/autofill/core/browser/validation.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/autofill/core/common/autofill_switches.h"
 #include "components/autofill/core/common/autofill_util.h"
@@ skipping 680 matching lines @@
 }
 
 std::vector<Suggestion> PersonalDataManager::GetProfileSuggestions(
     const AutofillType& type,
     const base::string16& field_contents,
     bool field_is_autofilled,
     const std::vector<ServerFieldType>& other_field_types) {
   if (IsInAutofillSuggestionsDisabledExperiment())
     return std::vector<Suggestion>();
 
+  AutofillProfileComparator comparator(app_locale_);
   base::string16 field_contents_canon =
-      AutofillProfile::CanonicalizeProfileString(field_contents);
+      comparator.NormalizeForComparison(field_contents);
 
   // Get the profiles to suggest, which are already sorted.
   std::vector<AutofillProfile*> profiles = GetProfilesToSuggest();
 
   std::vector<Suggestion> suggestions;
   // Match based on a prefix search.
   std::vector<AutofillProfile*> matched_profiles;
   for (AutofillProfile* profile : profiles) {
     base::string16 value = GetInfoInOneLine(profile, type, app_locale_);
     if (value.empty())
       continue;
 
     bool prefix_matched_suggestion;
-    base::string16 suggestion_canon =
-        AutofillProfile::CanonicalizeProfileString(value);
+    base::string16 suggestion_canon = comparator.NormalizeForComparison(value);
     if (IsValidSuggestionForFieldContents(
             suggestion_canon, field_contents_canon, type,
             /* is_masked_server_card= */ false, &prefix_matched_suggestion)) {
       matched_profiles.push_back(profile);
       suggestions.push_back(Suggestion(value));
       suggestions.back().backend_id = profile->guid();
       suggestions.back().match = prefix_matched_suggestion
                                      ? Suggestion::PREFIX_MATCH
                                      : Suggestion::SUBSTRING_MATCH;
     }
@@ skipping 167 matching lines @@
                 return !a->IsVerified();
               return a->CompareFrecency(b, 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.
+  AutofillProfileComparator comparator(app_locale);
   for (AutofillProfile* existing_profile : existing_profiles) {
     if (!matching_profile_found &&
-        !new_profile.PrimaryValue(app_locale_).empty() &&
+        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;
       guid = existing_profile->guid();
 
       // We set the modification date so that immediate requests for profiles
       // will properly reflect the fact that this profile has been modified
@@ skipping 685 matching lines @@
   // similar verified profile will be discarded.
   base::Time comparison_time = base::Time::Now();
   std::sort(existing_profiles->begin(), existing_profiles->end(),
             [comparison_time](const AutofillDataModel* a,
                               const AutofillDataModel* b) {
               if (a->IsVerified() != b->IsVerified())
                 return !a->IsVerified();
               return a->CompareFrecency(b, comparison_time);
             });
 
+  AutofillProfileComparator comparator(app_locale_);
+
   for (size_t i = 0; i < existing_profiles->size(); ++i) {
     AutofillProfile* profile_to_merge = (*existing_profiles)[i];
 
     // If the profile was set to be deleted, skip it. It has already been
     // merged into another profile.
     if (profiles_to_delete->count(profile_to_merge))
       continue;
 
     // If we have reached the verified profiles, stop trying to merge. Verified
     // profiles do not get merged.
     if (profile_to_merge->IsVerified())
       break;
 
     // If we have not reached the last profile, try to merge |profile_to_merge|
     // with all the less relevant |existing_profiles|.
     for (size_t j = i + 1; j < existing_profiles->size(); ++j) {
       // Don't try to merge a profile that was already set for deletion. Also
       // don't try to merge with profiles with a different PrimaryValue
       // because they are not similar.
       AutofillProfile* existing_profile = (*existing_profiles)[j];
       if (!profiles_to_delete->count(existing_profile) &&
-          existing_profile->PrimaryValue(app_locale_) ==
-              profile_to_merge->PrimaryValue(app_locale_)) {
+          comparator.AreMergeable(*existing_profile, *profile_to_merge)) {

[sebsg, 2016/06/29 08:57:04]

If you made the change to SaveAdditionnalInfo (dcheck to return false) this line
can be removed.

[Roger McFarlane (Chromium), 2016/06/29 18:21:37]

That's true, but I think that would make this code less clear.

Maybe if we rename SaveAdditionalInfo to AttemptMerge or something, then it
would make sense?

---- Start Chat ----

rogerm: SaveAdditionalInfo seems like it could use a rename (or a deletion)

rogerm: it's called in two places MergeProfiles and DedupProfiles and really
only serves to abort if the IsVerified() status is not right and/or to
degenerate to "successful" NOP.

sebsg: Yes exactly, it used to do what AreMeargeable now does with a few more
things like you mentioned. So I'm thinking about what the best course of action
is, renaming and keeping it super small or move the code into OverwriteWith or
something else

rogerm: So, the issue to me is that the IsVerified logic isn't accounted for in
AreMergeable and OverwriteWith (which I'm renaming to MergeDataFrom, btw)

sebsg: yeah, what I'd do for now is leave it as is but with the return false
instead. also line 541-543 can probably be removed
That way the behavior of SaveAdditionalInfo is pretty much the same as before

rogerm: Ok, I'm going to do a quick layout and comment update at the
SaveAdditionalInfo call sites, but leave the logic as it is in this CL and we
can revisit this later.

[sebsg, 2016/06/29 19:04:50]

Yes, AttemptMerge would be a better name for sure. But let's do that in another
CL as refactoring. This works well for the moment.

         // If the profiles are found to be similar, |profile_to_merge|'s non
         // empty data will overwrite the |existing_profile|'s data.
         // |existing_profile| will have added the complementary data from
         // |profile_to_merge| and will keep the most relevant syntax for data
         // present in both profiles.
         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.
           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());
 }
 
 }  // namespace autofill