Use AutofillProfileComparator in place of ad-hoc merge logic.

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 781ddd080fb2b4c50257e84120e601ea2eadb5df..90e221dd9cbebcd3c4725953da474a5c87d388aa 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -24,6 +24,7 @@
 #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"
@@ -724,8 +725,9 @@ std::vector<Suggestion> PersonalDataManager::GetProfileSuggestions(
   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();
@@ -739,8 +741,7 @@ std::vector<Suggestion> PersonalDataManager::GetProfileSuggestions(
       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)) {
@@ -928,9 +929,10 @@ std::string PersonalDataManager::MergeProfile(
 
   // 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
@@ -1636,6 +1638,8 @@ void PersonalDataManager::DedupeProfiles(
               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];
 
@@ -1657,8 +1661,7 @@ void PersonalDataManager::DedupeProfiles(
       // 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