Use AutofillProfileComparator in place of ad-hoc merge logic.

components/autofill/core/browser/autofill_profile.cc

Index: components/autofill/core/browser/autofill_profile.cc
diff --git a/components/autofill/core/browser/autofill_profile.cc b/components/autofill/core/browser/autofill_profile.cc
index ee5111b1d0ca28fcf736ae562e6f753217683a92..ff37996a1c0afa9f8236281f85306a584c96320a 100644
--- a/components/autofill/core/browser/autofill_profile.cc
+++ b/components/autofill/core/browser/autofill_profile.cc
@@ -189,36 +189,6 @@ void GetFieldsForDistinguishingProfiles(
   }
 }
 
-// Collapse compound field types to their "full" type.  I.e. First name
-// collapses to full name, area code collapses to full phone, etc.
-void CollapseCompoundFieldTypes(ServerFieldTypeSet* type_set) {
-  ServerFieldTypeSet collapsed_set;
-  for (const auto& it : *type_set) {
-    switch (it) {
-      case NAME_FIRST:
-      case NAME_MIDDLE:
-      case NAME_LAST:
-      case NAME_MIDDLE_INITIAL:
-      case NAME_FULL:
-      case NAME_SUFFIX:
-        collapsed_set.insert(NAME_FULL);
-        break;
-
-      case PHONE_HOME_NUMBER:
-      case PHONE_HOME_CITY_CODE:
-      case PHONE_HOME_COUNTRY_CODE:
-      case PHONE_HOME_CITY_AND_NUMBER:
-      case PHONE_HOME_WHOLE_NUMBER:
-        collapsed_set.insert(PHONE_HOME_WHOLE_NUMBER);
-        break;
-
-      default:
-        collapsed_set.insert(it);
-    }
-  }
-  std::swap(*type_set, collapsed_set);
-}
-
 }  // namespace
 
 AutofillProfile::AutofillProfile(const std::string& guid,
@@ -415,17 +385,6 @@ bool AutofillProfile::operator!=(const AutofillProfile& profile) const {
   return !operator==(profile);
 }
 
-const base::string16 AutofillProfile::PrimaryValue(
-    const std::string& app_locale) const {
-  std::vector<base::string16> primary_values{
-      GetInfo(AutofillType(NAME_FIRST), app_locale),
-      GetInfo(AutofillType(NAME_LAST), app_locale),
-      GetInfo(AutofillType(ADDRESS_HOME_LINE1), app_locale),
-      GetInfo(AutofillType(ADDRESS_HOME_CITY), app_locale)};
-  return CanonicalizeProfileString(
-      base::JoinString(primary_values, base::UTF8ToUTF16(" ")));
-}
-
 bool AutofillProfile::IsSubsetOf(const AutofillProfile& profile,
                                  const std::string& app_locale) const {
   ServerFieldTypeSet types;
@@ -514,62 +473,63 @@ bool AutofillProfile::OverwriteWith(const AutofillProfile& profile,
                                     const std::string& app_locale) {
   // Verified profiles should never be overwritten with unverified data.
   DCHECK(!IsVerified() || profile.IsVerified());
+  AutofillProfileComparator comparator(app_locale);
+  DCHECK(comparator.AreMergeable(*this, profile));
+
+  NameInfo name;
+  EmailInfo email;
+  CompanyInfo company;
+  PhoneNumber phone_number(this);
+  Address address;
+
+  // The comparator's merge operations are biased to prefer the data in the
+  // first profile parameter when the data is the same modulo case. We pass the
+  // other profile in this position to prefer accepting updates instead of

[Mathieu, 2016/06/29 15:50:57]

possibly change "other" -> "incoming"

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

Done.

+  // preserving the original data. I.e., passing the other profile first accepts
+  // case changes, the other ways does not.
+  if (!comparator.MergeNames(profile, *this, &name) ||
+      !comparator.MergeEmailAddresses(profile, *this, &email) ||
+      !comparator.MergeCompanyNames(profile, *this, &company) ||
+      !comparator.MergePhoneNumbers(profile, *this, &phone_number) ||
+      !comparator.MergeAddresses(profile, *this, &address)) {
+    NOTREACHED();
+    return false;
+  }
+
   set_origin(profile.origin());
   set_language_code(profile.language_code());
   set_use_count(profile.use_count() + use_count());
   if (profile.use_date() > use_date())
     set_use_date(profile.use_date());
 
-  // |types_to_overwrite| is initially populated with all types that have
-  //  non-empty data in the incoming |profile|. After adjustment, all data from
-  // |profile| corresponding to types in |types_to_overwrite| is overwritten in
-  // |this| profile.
-  ServerFieldTypeSet types_to_overwrite;
-  profile.GetNonEmptyTypes(app_locale, &types_to_overwrite);
-
-  // Only transfer "full" types (e.g. full name) and not fragments (e.g.
-  // first name, last name).
-  CollapseCompoundFieldTypes(&types_to_overwrite);
+  bool modified = false;

[Mathieu, 2016/06/29 15:50:57]

Please add an obvious comment above this block. Something like "now that the new
values have been obtained, overwrite them if applicable". 

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

Done.

 
-  // Remove ADDRESS_HOME_STREET_ADDRESS to ensure a merge of the address line by
-  // line. See comment below.
-  types_to_overwrite.erase(ADDRESS_HOME_STREET_ADDRESS);
-
-  l10n::CaseInsensitiveCompare compare;
+  if (name_ != name) {
+    name_ = name;
+    modified = true;
+  }
 
-  // Special case for addresses. With the whole address comparison, it is now
-  // necessary to make sure to keep the best address format: both lines used.
-  // This is because some sites might not have an address line 2 and the
-  // previous value should not be replaced with an empty string in that case.
-  if (compare.StringsEqual(
-          CanonicalizeProfileString(
-              profile.GetRawInfo(ADDRESS_HOME_STREET_ADDRESS)),
-          CanonicalizeProfileString(GetRawInfo(ADDRESS_HOME_STREET_ADDRESS))) &&
-      !GetRawInfo(ADDRESS_HOME_LINE2).empty() &&
-      profile.GetRawInfo(ADDRESS_HOME_LINE2).empty()) {
-    types_to_overwrite.erase(ADDRESS_HOME_LINE1);
-    types_to_overwrite.erase(ADDRESS_HOME_LINE2);
+  if (email_ != email) {
+    email_ = email;
+    modified = true;
   }
 
-  bool did_overwrite = false;
+  if (company_ != company) {
+    company_ = company;
+    modified = true;
+  }
 
-  for (const ServerFieldType field_type : types_to_overwrite) {
-    // Special case for names.
-    if (AutofillType(field_type).group() == NAME) {
-      did_overwrite |= OverwriteName(profile.name_, app_locale);
-      continue;
-    }
+  if (phone_number_ != phone_number) {
+    phone_number_ = phone_number;
+    modified = true;
+  }
 
-    base::string16 new_value = profile.GetRawInfo(field_type);
-    // Overwrite the data in |this| profile for the field type and set
-    // |did_overwrite| if the previous data was different than the |new_value|.
-    if (GetRawInfo(field_type) != new_value) {
-      SetRawInfo(field_type, new_value);
-      did_overwrite = true;
-    }
+  if (address_ != address) {
+    address_ = address;
+    modified = true;
   }
 
-  return did_overwrite;
+  return modified;
 }
 
 bool AutofillProfile::SaveAdditionalInfo(const AutofillProfile& profile,
@@ -582,107 +542,15 @@ bool AutofillProfile::SaveAdditionalInfo(const AutofillProfile& profile,
   GetNonEmptyTypes(app_locale, &field_types);

[sebsg, 2016/06/29 15:35:24]

We don't need those 3 lines either now :)

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

Removed.

Also removed OverwriteName() which is also no longer used.

   profile.GetNonEmptyTypes(app_locale, &other_field_types);
 
-  // The address needs to be compared line by line to take into account the
-  // logic for empty fields implemented in the loop.
-  field_types.erase(ADDRESS_HOME_STREET_ADDRESS);
-  l10n::CaseInsensitiveCompare compare;
-  for (ServerFieldType field_type : field_types) {
-    if (other_field_types.count(field_type)) {
-      AutofillType type = AutofillType(field_type);
-      // Special cases for name and phone. If the whole/full value matches, skip
-      // the individual fields comparison.
-      if (type.group() == NAME &&
-          compare.StringsEqual(
-              profile.GetInfo(AutofillType(NAME_FULL), app_locale),
-              GetInfo(AutofillType(NAME_FULL), app_locale))) {
-        continue;
-      }
-      if (type.group() == PHONE_HOME &&
-          i18n::PhoneNumbersMatch(
-              GetRawInfo(PHONE_HOME_WHOLE_NUMBER),
-              profile.GetRawInfo(PHONE_HOME_WHOLE_NUMBER),
-              base::UTF16ToASCII(GetRawInfo(ADDRESS_HOME_COUNTRY)),
-              app_locale)) {
-        continue;
-      }
-
-      // Special case for postal codes, where postal codes with/without spaces
-      // in them are considered equivalent.
-      if (field_type == ADDRESS_HOME_ZIP) {
-        base::string16 profile_zip;
-        base::string16 current_zip;
-        base::RemoveChars(profile.GetRawInfo(field_type), ASCIIToUTF16(" "),
-                          &profile_zip);
-        base::RemoveChars(GetRawInfo(field_type), ASCIIToUTF16(" "),
-                          &current_zip);
-        if (!compare.StringsEqual(profile_zip, current_zip))
-          return false;
-        continue;
-      }
-
-      // Special case for the address because the comparison uses canonicalized
-      // values. Start by comparing the address line by line. If it fails, make
-      // sure that the address as a whole is different before returning false.
-      // It is possible that the user put the info from line 2 on line 1 because
-      // of a certain form for example.
-      if (field_type == ADDRESS_HOME_LINE1 ||
-          field_type == ADDRESS_HOME_LINE2) {
-        if (!compare.StringsEqual(
-                CanonicalizeProfileString(profile.GetRawInfo(field_type)),
-                CanonicalizeProfileString(GetRawInfo(field_type))) &&
-            !compare.StringsEqual(CanonicalizeProfileString(profile.GetRawInfo(
-                                      ADDRESS_HOME_STREET_ADDRESS)),
-                                  CanonicalizeProfileString(GetRawInfo(
-                                      ADDRESS_HOME_STREET_ADDRESS)))) {
-          return false;
-        }
-        continue;
-      }
+  AutofillProfileComparator comparator(app_locale);
 
-      // Special case for the state to support abbreviations. Currently only the
-      // US states are supported.
-      if (field_type == ADDRESS_HOME_STATE) {
-        base::string16 full;
-        base::string16 abbreviation;
-        state_names::GetNameAndAbbreviation(GetRawInfo(ADDRESS_HOME_STATE),
-                                            &full, &abbreviation);
-        if (compare.StringsEqual(profile.GetRawInfo(ADDRESS_HOME_STATE),
-                                 full) ||
-            compare.StringsEqual(profile.GetRawInfo(ADDRESS_HOME_STATE),
-                                 abbreviation))
-          continue;
-      }
-
-      // Special case for company names to support cannonicalized variations.
-      if (field_type == COMPANY_NAME) {
-        if (compare.StringsEqual(
-                CanonicalizeProfileString(profile.GetRawInfo(field_type)),
-                CanonicalizeProfileString(GetRawInfo(field_type)))) {
-          continue;
-        }
-      }
-
-      // Special case for middle name to support initials.
-      if (field_type == NAME_MIDDLE) {
-        base::string16 middle_name = GetRawInfo(NAME_MIDDLE);
-        base::string16 profile_middle_name = profile.GetRawInfo(NAME_MIDDLE);
-        DCHECK(!middle_name.empty());
-        DCHECK(!profile_middle_name.empty());
-        // If one of the two middle names is an initial that matches the first
-        // letter of the other middle name, they are considered equivalent.
-        if ((middle_name.size() == 1 || profile_middle_name.size() == 1) &&
-            middle_name[0] == profile_middle_name[0]) {
-          continue;
-        }
-      }
-
-      if (!compare.StringsEqual(profile.GetRawInfo(field_type),
-                                GetRawInfo(field_type))) {
-        return false;
-      }
-    }
-  }
+  // SaveAdditionalInfo should not have been called if the profiles were not
+  // already deemed to be mergeable.
+  DCHECK(comparator.AreMergeable(*this, profile));

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

Would it be better to return false here instead? It would be more in line with
the previous behaviour where we returned false if we did not find the profiles
to be mergeable. What do you think?

I'm also thinking this method should be renamed but I can do that in another CL.
There are a bunch a method names changes and small refactoring enabled by your
CL.

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

This dcheck firing indicates a programming error, not an expected possible
condition. The relevant test that would return false from this method is the
IsVerified !! profile.IsVerified test at the top. Otherwise the do
something/nothing logic is the same modulo the bail out in the middle initial
handling of the old version.

I'd actually refactor this a bit to simplify the extra layer that
SaveAdditionalInfo adds. I'm just not sure yet what I want.

I don't really like the logic of "yes they're mergeable, so merge them and maybe
it'll be a NOP but return true anyway".

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

Yes, let's discuss it and fix it in a subsequent CL. This looks good.

 
+  // We don't replace verified profile data with unverified profile data. But,
+  // we can merge two verified profiles or merge verified profile data into an
+  // unverified profile.
   if (!IsVerified() || profile.IsVerified()) {
     if (OverwriteWith(profile, app_locale)) {
       AutofillMetrics::LogProfileActionOnFormSubmitted(
@@ -782,14 +650,6 @@ void AutofillProfile::RecordAndLogUse() {
   RecordUse();
 }
 
-// static
-base::string16 AutofillProfile::CanonicalizeProfileString(
-    const base::string16& str) {
-  // The locale doesn't matter for general string canonicalization.
-  AutofillProfileComparator comparator("en-US");
-  return comparator.NormalizeForComparison(str);
-}
-
 void AutofillProfile::GetSupportedTypes(
     ServerFieldTypeSet* supported_types) const {
   FormGroupList info = FormGroups();