autofill names - dont parse when calling SetRawInfo(FULL_NAME)

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 e786bb3305950e8cf6ae7ce3fa9c35fc909686fc..70c2e44c6ad986f4b47133e733d485cdd7ea4ec6 100644
--- a/components/autofill/core/browser/autofill_profile.cc
+++ b/components/autofill/core/browser/autofill_profile.cc
@@ -189,24 +189,20 @@ void CollapseCompoundFieldTypes(ServerFieldTypeSet* type_set) {
   std::swap(*type_set, collapsed_set);
 }
 
-class FindByPhone {
+class FindByPhone
+    : public std::unary_function<const base::string16&, bool> {

[Ilya Sherman, 2014/06/27 06:25:08]

FWIW, it looks like std::unary_function is deprecated in C++11, so it's not
clear to me whether there's much benefit to moving existing code over to use it.
 (I've never previously encountered it before, so I'm not familiar with the
details.)

[Evan Stade, 2014/06/28 01:01:35]

thanks for reminding me to remove this. It was only there as a part of a
slightly different approach that I originally attempted.

  public:
   FindByPhone(const base::string16& phone,
               const std::string& country_code,
               const std::string& app_locale)
       : phone_(phone),
         country_code_(country_code),
-        app_locale_(app_locale) {
-  }
+        app_locale_(app_locale) {}
 
   bool operator()(const base::string16& phone) {
     return i18n::PhoneNumbersMatch(phone, phone_, country_code_, app_locale_);
   }
 
-  bool operator()(const base::string16* phone) {
-    return i18n::PhoneNumbersMatch(*phone, phone_, country_code_, app_locale_);
-  }
-
  private:
   base::string16 phone_;
   std::string country_code_;
@@ -215,12 +211,18 @@ class FindByPhone {
 
 // Functor used to check for case-insensitive equality of two strings.
 struct CaseInsensitiveStringEquals
-    : public std::binary_function<base::string16, base::string16, bool>
-{
-  bool operator()(const base::string16& x, const base::string16& y) const {
-    return
-        x.size() == y.size() && StringToLowerASCII(x) == StringToLowerASCII(y);
+    : public std::unary_function<const base::string16&, bool> {
+ public:
+  CaseInsensitiveStringEquals(const base::string16& other)
+      : other_(other) {}
+
+  bool operator()(const base::string16& x) const {
+    return x.size() == other_.size() &&
+        StringToLowerASCII(x) == StringToLowerASCII(other_);
   }
+
+ private:
+  const base::string16& other_;
 };
 
 }  // namespace
@@ -434,7 +436,8 @@ int AutofillProfile::Compare(const AutofillProfile& profile) const {
       return comparison;
   }
 
-  const ServerFieldType multi_value_types[] = { NAME_FIRST,
+  const ServerFieldType multi_value_types[] = { NAME_FULL,
+                                                NAME_FIRST,
                                                 NAME_MIDDLE,
                                                 NAME_LAST,
                                                 EMAIL_ADDRESS,
@@ -518,7 +521,8 @@ bool AutofillProfile::IsSubsetOf(const AutofillProfile& profile,
 }
 
 void AutofillProfile::OverwriteOrAppendNames(
-    const std::vector<NameInfo>& names) {
+    const std::vector<NameInfo>& names,
+    const std::string& app_locale) {
   std::vector<NameInfo> results(name_);
   for (std::vector<NameInfo>::const_iterator it = names.begin();
        it != names.end();
@@ -528,14 +532,20 @@ void AutofillProfile::OverwriteOrAppendNames(
 
     for (size_t index = 0; index < name_.size(); ++index) {
       NameInfo current_name = name_[index];
-      if (current_name.EqualsIgnoreCase(imported_name)) {
+      if (current_name.ParsedBitsAreEqual(imported_name)) {
+        if (current_name.GetRawInfo(NAME_FULL).empty()) {
+          current_name.SetRawInfo(NAME_FULL,
+                                  imported_name.GetRawInfo(NAME_FULL));
+        }
+
         should_append_imported_name = false;
         break;
       }
 
-      base::string16 full_name = current_name.GetRawInfo(NAME_FULL);
+      AutofillType type = AutofillType(NAME_FULL);
+      base::string16 full_name = current_name.GetInfo(type, app_locale);
       if (StringToLowerASCII(full_name) ==
-          StringToLowerASCII(imported_name.GetRawInfo(NAME_FULL))) {
+          StringToLowerASCII(imported_name.GetInfo(type, app_locale))) {
         // The imported name has the same full name string as one of the
         // existing names for this profile.  Because full names are
         // _heuristically_ parsed into {first, middle, last} name components,
@@ -545,13 +555,13 @@ void AutofillProfile::OverwriteOrAppendNames(
         // parse, as this more likely represents the correct, user-input parse
         // of the name.
         NameInfo heuristically_parsed_name;
-        heuristically_parsed_name.SetRawInfo(NAME_FULL, full_name);
-        if (imported_name.EqualsIgnoreCase(heuristically_parsed_name)) {
+        heuristically_parsed_name.SetInfo(type, full_name, app_locale);
+        if (imported_name.ParsedBitsAreEqual(heuristically_parsed_name)) {
           should_append_imported_name = false;
           break;
         }
 
-        if (current_name.EqualsIgnoreCase(heuristically_parsed_name)) {
+        if (current_name.ParsedBitsAreEqual(heuristically_parsed_name)) {
           results[index] = imported_name;
           should_append_imported_name = false;
           break;
@@ -589,43 +599,57 @@ void AutofillProfile::OverwriteWithOrAddTo(const AutofillProfile& profile,
 
   for (ServerFieldTypeSet::const_iterator iter = field_types.begin();
        iter != field_types.end(); ++iter) {
-    if (AutofillProfile::SupportsMultiValue(*iter)) {
-      std::vector<base::string16> new_values;
-      profile.GetRawMultiInfo(*iter, &new_values);
-      std::vector<base::string16> existing_values;
-      GetRawMultiInfo(*iter, &existing_values);
-
-      // GetMultiInfo always returns at least one element, even if the profile
-      // has no data stored for this field type.
-      if (existing_values.size() == 1 && existing_values.front().empty())
-        existing_values.clear();
-
-      FieldTypeGroup group = AutofillType(*iter).group();
-      for (std::vector<base::string16>::iterator value_iter =
-               new_values.begin();
-           value_iter != new_values.end(); ++value_iter) {
-        // Don't add duplicates.
-        if (group == PHONE_HOME) {
-          AddPhoneIfUnique(*value_iter, app_locale, &existing_values);
-        } else {
-          std::vector<base::string16>::const_iterator existing_iter =
-              std::find_if(
-                  existing_values.begin(), existing_values.end(),
-                  std::bind1st(CaseInsensitiveStringEquals(), *value_iter));
-          if (existing_iter == existing_values.end())
-            existing_values.insert(existing_values.end(), *value_iter);
-        }
-      }
-      if (group == NAME)
-        OverwriteOrAppendNames(profile.name_);
-      else
-        SetRawMultiInfo(*iter, existing_values);
-    } else {
+    FieldTypeGroup group = AutofillType(*iter).group();
+    // Special case names.
+    if (group == NAME) {
+      OverwriteOrAppendNames(profile.name_, app_locale);
+      continue;

[Ilya Sherman, 2014/06/27 06:25:08]

Optional nit: I prefer to only use "continue" stmts when doing no work other
than skipping over an iteration of the loop.  In this case, I think an if/else
ladder would be clearer.

[Evan Stade, 2014/06/28 01:01:35]

I'll pass on this nit, as I prefer less indentation for the meat of the loop.

+    }
+
+    // Single value field --- overwrite.
+    if (!AutofillProfile::SupportsMultiValue(*iter)) {
       base::string16 new_value = profile.GetRawInfo(*iter);
       if (StringToLowerASCII(GetRawInfo(*iter)) !=
               StringToLowerASCII(new_value)) {
         SetRawInfo(*iter, new_value);
       }
+      continue;
+    }
+
+    // Multi value field --- overwrite/append.
+    std::vector<base::string16> new_values;
+    profile.GetRawMultiInfo(*iter, &new_values);
+    std::vector<base::string16> existing_values;
+    GetRawMultiInfo(*iter, &existing_values);
+
+    // GetMultiInfo always returns at least one element, even if the profile
+    // has no data stored for this field type.
+    if (existing_values.size() == 1 && existing_values.front().empty())
+      existing_values.clear();
+
+    for (std::vector<base::string16>::iterator value_iter =
+             new_values.begin();
+         value_iter != new_values.end(); ++value_iter) {
+      // Don't add duplicates. Most types get case insensitive matching.
+      std::vector<base::string16>::const_iterator existing_iter;
+
+      if (group == PHONE_HOME) {
+        // Phones allow "fuzzy" matching, so "1-800-FLOWERS", "18003569377",
+        // "(800)356-9377" and "356-9377" are considered the same.
+        std::string country_code =
+            base::UTF16ToASCII(GetRawInfo(ADDRESS_HOME_COUNTRY));
+        existing_iter =
+            std::find_if(existing_values.begin(), existing_values.end(),
+                         FindByPhone(*value_iter, country_code, app_locale));
+      } else {
+        existing_iter =
+            std::find_if(existing_values.begin(), existing_values.end(),
+                         CaseInsensitiveStringEquals(*value_iter));
+      }
+
+      if (existing_iter == existing_values.end())
+        existing_values.insert(existing_values.end(), *value_iter);
+      SetRawMultiInfo(*iter, existing_values);

[Ilya Sherman, 2014/06/27 06:25:08]

It seems like this ought to be outside the inner for loop.

[Evan Stade, 2014/06/28 01:01:35]

good catch. This is the unfortunate kind of bug a test probably could never
catch, because the end state of *iter is correct.

     }
   }
 }
@@ -643,10 +667,11 @@ bool AutofillProfile::SupportsMultiValue(ServerFieldType type) {
 // static
 void AutofillProfile::CreateDifferentiatingLabels(
     const std::vector<AutofillProfile*>& profiles,
+    const std::string& app_locale,
     std::vector<base::string16>* labels) {
   const size_t kMinimalFieldsShown = 2;
   CreateInferredLabels(profiles, NULL, UNKNOWN_TYPE, kMinimalFieldsShown,
-                       labels);
+                       app_locale, labels);
   DCHECK_EQ(profiles.size(), labels->size());
 }
 
@@ -656,6 +681,7 @@ void AutofillProfile::CreateInferredLabels(
     const std::vector<ServerFieldType>* suggested_fields,
     ServerFieldType excluded_field,
     size_t minimal_fields_shown,
+    const std::string& app_locale,
     std::vector<base::string16>* labels) {
   std::vector<ServerFieldType> fields_to_use;
   GetFieldsForDistinguishingProfiles(suggested_fields, excluded_field,
@@ -668,7 +694,8 @@ void AutofillProfile::CreateInferredLabels(
   for (size_t i = 0; i < profiles.size(); ++i) {
     base::string16 label =
         profiles[i]->ConstructInferredLabel(fields_to_use,
-                                            minimal_fields_shown);
+                                            minimal_fields_shown,
+                                            app_locale);
     labels_to_profiles[label].push_back(i);
   }
 
@@ -685,7 +712,7 @@ void AutofillProfile::CreateInferredLabels(
       // We have more than one profile with the same label, so add
       // differentiating fields.
       CreateInferredLabelsHelper(profiles, it->second, fields_to_use,
-                                 minimal_fields_shown, labels);
+                                 minimal_fields_shown, app_locale, labels);
     }
   }
 }
@@ -719,25 +746,10 @@ void AutofillProfile::GetMultiInfoImpl(
   }
 }
 
-void AutofillProfile::AddPhoneIfUnique(
-    const base::string16& phone,
-    const std::string& app_locale,
-    std::vector<base::string16>* existing_phones) {
-  DCHECK(existing_phones);
-  // Phones allow "fuzzy" matching, so "1-800-FLOWERS", "18003569377",
-  // "(800)356-9377" and "356-9377" are considered the same.
-  std::string country_code =
-      base::UTF16ToASCII(GetRawInfo(ADDRESS_HOME_COUNTRY));
-  if (std::find_if(existing_phones->begin(), existing_phones->end(),
-                   FindByPhone(phone, country_code, app_locale)) ==
-      existing_phones->end()) {
-    existing_phones->push_back(phone);
-  }
-}
-
 base::string16 AutofillProfile::ConstructInferredLabel(
     const std::vector<ServerFieldType>& included_fields,
-    size_t num_fields_to_use) const {
+    size_t num_fields_to_use,
+    const std::string& app_locale) const {
   const base::string16 separator =
       l10n_util::GetStringUTF16(IDS_AUTOFILL_ADDRESS_SUMMARY_SEPARATOR);
 
@@ -747,7 +759,7 @@ base::string16 AutofillProfile::ConstructInferredLabel(
            included_fields.begin();
        it != included_fields.end() && num_fields_used < num_fields_to_use;
        ++it) {
-    base::string16 field = GetRawInfo(*it);
+    base::string16 field = GetInfo(AutofillType(*it), app_locale);
     if (field.empty())
       continue;
 
@@ -772,6 +784,7 @@ void AutofillProfile::CreateInferredLabelsHelper(
     const std::list<size_t>& indices,
     const std::vector<ServerFieldType>& fields,
     size_t num_fields_to_include,
+    const std::string& app_locale,
     std::vector<base::string16>* labels) {
   // For efficiency, we first construct a map of fields to their text values and
   // each value's frequency.
@@ -785,7 +798,8 @@ void AutofillProfile::CreateInferredLabelsHelper(
     for (std::list<size_t>::const_iterator it = indices.begin();
          it != indices.end(); ++it) {
       const AutofillProfile* profile = profiles[*it];
-      base::string16 field_text = profile->GetRawInfo(*field);
+      base::string16 field_text =
+          profile->GetInfo(AutofillType(*field), app_locale);
 
       // If this label is not already in the map, add it with frequency 0.
       if (!field_text_frequencies.count(field_text))
@@ -812,7 +826,8 @@ void AutofillProfile::CreateInferredLabelsHelper(
     for (std::vector<ServerFieldType>::const_iterator field = fields.begin();
          field != fields.end(); ++field) {
       // Skip over empty fields.
-      base::string16 field_text = profile->GetRawInfo(*field);
+      base::string16 field_text =
+          profile->GetInfo(AutofillType(*field), app_locale);
       if (field_text.empty())
         continue;
 
@@ -837,8 +852,8 @@ void AutofillProfile::CreateInferredLabelsHelper(
         break;
     }
 
-    (*labels)[*it] =
-        profile->ConstructInferredLabel(label_fields, label_fields.size());
+    (*labels)[*it] = profile->ConstructInferredLabel(
+        label_fields, label_fields.size(), app_locale);
   }
 }