autofill names - dont parse when calling SetRawInfo(FULL_NAME)

components/autofill/core/browser/autofill_profile.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/autofill_profile.h"
 
 #include <algorithm>
 #include <functional>
 #include <map>
 #include <ostream>
@@ skipping 171 matching lines @@
         collapsed_set.insert(PHONE_HOME_WHOLE_NUMBER);
         break;
 
       default:
         collapsed_set.insert(*it);
     }
   }
   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_;
   std::string app_locale_;
 };
 
 // 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
 
 AutofillProfile::AutofillProfile(const std::string& guid,
                                  const std::string& origin)
     : AutofillDataModel(guid, origin),
       name_(1),
       email_(1),
       phone_number_(1, PhoneNumber(this)) {
@@ skipping 193 matching lines @@
     ADDRESS_HOME_COUNTRY,
   };
 
   for (size_t i = 0; i < arraysize(single_value_types); ++i) {
     int comparison = GetRawInfo(single_value_types[i]).compare(
         profile.GetRawInfo(single_value_types[i]));
     if (comparison != 0)
       return comparison;
   }
 
-  const ServerFieldType multi_value_types[] = { NAME_FIRST,
+  const ServerFieldType multi_value_types[] = { NAME_FULL,
+                                                NAME_FIRST,
                                                 NAME_MIDDLE,
                                                 NAME_LAST,
                                                 EMAIL_ADDRESS,
                                                 PHONE_HOME_WHOLE_NUMBER };
 
   for (size_t i = 0; i < arraysize(multi_value_types); ++i) {
     std::vector<base::string16> values_a;
     std::vector<base::string16> values_b;
     GetRawMultiInfo(multi_value_types[i], &values_a);
     profile.GetRawMultiInfo(multi_value_types[i], &values_b);
@@ skipping 63 matching lines @@
     } else if (StringToLowerASCII(GetRawInfo(*it)) !=
                    StringToLowerASCII(profile.GetRawInfo(*it))) {
       return false;
     }
   }
 
   return true;
 }
 
 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();
        ++it) {
     NameInfo imported_name = *it;
     bool should_append_imported_name = true;
 
     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,
         // it's possible that either the existing name or the imported name
         // was misparsed.  Prefer to keep the name whose {first, middle,
         // last} components do not match those computed by the heuristic
         // 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;
         }
       }
     }
 
     // Append unique names to the list.
     if (should_append_imported_name)
       results.push_back(imported_name);
@@ skipping 17 matching lines @@
   CollapseCompoundFieldTypes(&field_types);
 
   // TODO(isherman): Revisit this decision in the context of i18n and storing
   // full addresses rather than storing 1-to-2 lines of an address.
   // For addresses, do the opposite: transfer individual address lines, rather
   // than full addresses.
   field_types.erase(ADDRESS_HOME_STREET_ADDRESS);
 
   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);
+    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.

+    }
 
-      // 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 {
+    // 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.

     }
   }
 }
 
 // static
 bool AutofillProfile::SupportsMultiValue(ServerFieldType type) {
   FieldTypeGroup group = AutofillType(type).group();
   return group == NAME ||
          group == NAME_BILLING ||
          group == EMAIL ||
          group == PHONE_HOME ||
          group == PHONE_BILLING;
 }
 
 // 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());
 }
 
 // static
 void AutofillProfile::CreateInferredLabels(
     const std::vector<AutofillProfile*>& profiles,
     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,
                                      &fields_to_use);
 
   // Construct the default label for each profile. Also construct a map that
   // associates each label with the profiles that have this label. This map is
   // then used to detect which labels need further differentiating fields.
   std::map<base::string16, std::list<size_t> > labels_to_profiles;
   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);
   }
 
   labels->resize(profiles.size());
   for (std::map<base::string16, std::list<size_t> >::const_iterator it =
            labels_to_profiles.begin();
        it != labels_to_profiles.end(); ++it) {
     if (it->second.size() == 1) {
       // This label is unique, so use it without any further ado.
       base::string16 label = it->first;
       size_t profile_index = it->second.front();
       (*labels)[profile_index] = label;
     } else {
       // 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);
     }
   }
 }
 
 void AutofillProfile::GetSupportedTypes(
     ServerFieldTypeSet* supported_types) const {
   FormGroupList info = FormGroups();
   for (FormGroupList::const_iterator it = info.begin(); it != info.end(); ++it)
     (*it)->GetSupportedTypes(supported_types);
 }
@@ skipping 13 matching lines @@
     case PHONE_HOME:
     case PHONE_BILLING:
       CopyItemsToValues(type, phone_number_, app_locale, values);
       break;
     default:
       values->resize(1);
       (*values)[0] = GetFormGroupInfo(*this, type, app_locale);
   }
 }
 
-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);
 
   base::string16 label;
   size_t num_fields_used = 0;
   for (std::vector<ServerFieldType>::const_iterator it =
            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;
 
     if (!label.empty())
       label.append(separator);
 
     label.append(field);
     ++num_fields_used;
   }
 
   // Flatten the label if need be.
   const base::string16& line_separator =
       l10n_util::GetStringUTF16(IDS_AUTOFILL_ADDRESS_LINE_SEPARATOR);
   base::ReplaceChars(label, base::ASCIIToUTF16("\n"), line_separator, &label);
 
   return label;
 }
 
 // static
 void AutofillProfile::CreateInferredLabelsHelper(
     const std::vector<AutofillProfile*>& profiles,
     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.
   std::map<ServerFieldType,
            std::map<base::string16, size_t> > field_text_frequencies_by_field;
   for (std::vector<ServerFieldType>::const_iterator field = fields.begin();
        field != fields.end(); ++field) {
     std::map<base::string16, size_t>& field_text_frequencies =
         field_text_frequencies_by_field[*field];
 
     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))
         field_text_frequencies[field_text] = 0;
 
       // Now, increment the frequency for this label.
       ++field_text_frequencies[field_text];
     }
   }
 
   // Now comes the meat of the algorithm. For each profile, we scan the list of
   // fields to use, looking for two things:
   //  1. A (non-empty) field that differentiates the profile from all others
   //  2. At least |num_fields_to_include| non-empty fields
   // Before we've satisfied condition (2), we include all fields, even ones that
   // are identical across all the profiles. Once we've satisfied condition (2),
   // we only include fields that that have at last two distinct values.
   for (std::list<size_t>::const_iterator it = indices.begin();
        it != indices.end(); ++it) {
     const AutofillProfile* profile = profiles[*it];
 
     std::vector<ServerFieldType> label_fields;
     bool found_differentiating_field = false;
     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;
 
       std::map<base::string16, size_t>& field_text_frequencies =
           field_text_frequencies_by_field[*field];
       found_differentiating_field |=
           !field_text_frequencies.count(base::string16()) &&
           (field_text_frequencies[field_text] == 1);
 
       // Once we've found enough non-empty fields, skip over any remaining
       // fields that are identical across all the profiles.
       if (label_fields.size() >= num_fields_to_include &&
           (field_text_frequencies.size() == 1))
         continue;
 
       label_fields.push_back(*field);
 
       // If we've (1) found a differentiating field and (2) found at least
       // |num_fields_to_include| non-empty fields, we're done!
       if (found_differentiating_field &&
           label_fields.size() >= num_fields_to_include)
         break;
     }
 
-    (*labels)[*it] =
-        profile->ConstructInferredLabel(label_fields, label_fields.size());
+    (*labels)[*it] = profile->ConstructInferredLabel(
+        label_fields, label_fields.size(), app_locale);
   }
 }
 
 AutofillProfile::FormGroupList AutofillProfile::FormGroups() const {
   FormGroupList v(5);
   v[0] = &name_[0];
   v[1] = &email_[0];
   v[2] = &company_;
   v[3] = &phone_number_[0];
   v[4] = &address_;
@@ skipping 68 matching lines @@
       << UTF16ToUTF8(profile.GetRawInfo(ADDRESS_HOME_SORTING_CODE))
       << " "
       << UTF16ToUTF8(profile.GetRawInfo(ADDRESS_HOME_COUNTRY))
       << " "
       << profile.language_code()
       << " "
       << UTF16ToUTF8(MultiString(profile, PHONE_HOME_WHOLE_NUMBER));
 }
 
 }  // namespace autofill