[Autofill] Ensure that clients pass the correct region info when parsing phone numbers.

BUG=100845


Review URL: https://chromiumcodereview.appspot.com/11783045

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175975 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autofill/autofill_ie_toolbar_import_win.cc

@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/string16.h"
 #include "base/win/registry.h"
+#include "chrome/browser/autofill/autofill_country.h"
 #include "chrome/browser/autofill/autofill_profile.h"
 #include "chrome/browser/autofill/credit_card.h"
 #include "chrome/browser/autofill/crypto/rc4_decryptor.h"
@@ -65,15 +66,15 @@ bool IsEmptySalt(std::wstring const& salt) {
   return true;
 }
 
-string16 ReadAndDecryptValue(RegKey* key, const wchar_t* value_name) {
+string16 ReadAndDecryptValue(const RegKey& key, const wchar_t* value_name) {
   DWORD data_type = REG_BINARY;
   DWORD data_size = 0;
-  LONG result = key->ReadValue(value_name, NULL, &data_size, &data_type);
+  LONG result = key.ReadValue(value_name, NULL, &data_size, &data_type);
   if ((result != ERROR_SUCCESS) || !data_size || data_type != REG_BINARY)
     return string16();
   std::vector<uint8> data;
   data.resize(data_size);
-  result = key->ReadValue(value_name, &(data[0]), &data_size, &data_type);
+  result = key.ReadValue(value_name, &(data[0]), &data_size, &data_type);
   if (result == ERROR_SUCCESS) {
     std::string out_data;
     if (syncer::DecryptData(data, &out_data)) {
@@ -123,42 +124,61 @@ struct {
 
 typedef std::map<std::wstring, AutofillFieldType> RegToFieldMap;
 
-bool ImportSingleProfile(FormGroup* profile,
-                         RegKey* key,
-                         const RegToFieldMap& reg_to_field ) {
-  DCHECK(profile != NULL);
-  if (!key->Valid())
+// Imports address or credit card data from the given registry |key| into the
+// given |form_group|, with the help of |reg_to_field|.  When importing address
+// data, writes the phone data into |phone|; otherwise, |phone| should be null.
+// Returns true if any fields were set, false otherwise.
+bool ImportSingleFormGroup(const RegKey& key,
+                           const RegToFieldMap& reg_to_field,
+                           FormGroup* form_group,
+                           PhoneNumber::PhoneCombineHelper* phone) {
+  if (!key.Valid())
     return false;
 
   bool has_non_empty_fields = false;
 
-  // Phones need to be rebuilt.
-  PhoneNumber::PhoneCombineHelper phone;
-
-  for (uint32 i = 0; i < key->GetValueCount(); ++i) {
+  for (uint32 i = 0; i < key.GetValueCount(); ++i) {
     std::wstring value_name;
-    if (key->GetValueNameAt(i, &value_name) != ERROR_SUCCESS)
+    if (key.GetValueNameAt(i, &value_name) != ERROR_SUCCESS)
       continue;
+
     RegToFieldMap::const_iterator it = reg_to_field.find(value_name);
     if (it == reg_to_field.end())
       continue;  // This field is not imported.
+
     string16 field_value = ReadAndDecryptValue(key, value_name.c_str());
     if (!field_value.empty()) {
-      has_non_empty_fields = true;
       if (it->second == CREDIT_CARD_NUMBER)
         field_value = DecryptCCNumber(field_value);
 
-      // We need to store phone data in |phone| before building the whole number
-      // at the end. The rest of the fields are set "as is".
+      // Phone numbers are stored piece-by-piece, and then reconstructed from
+      // the pieces.  The rest of the fields are set "as is".
       // TODO(isherman): Call SetInfo(), rather than SetRawInfo().
-      if (!phone.SetInfo(it->second, field_value))
-        profile->SetRawInfo(it->second, field_value);
+      if (!phone || !phone->SetInfo(it->second, field_value)) {
+        has_non_empty_fields = true;
+        form_group->SetRawInfo(it->second, field_value);
+      }
     }
   }
+
+  return has_non_empty_fields;
+}
+
+// Imports address data from the given registry |key| into the given |profile|,
+// with the help of |reg_to_field|.  Returns true if any fields were set, false
+// otherwise.
+bool ImportSingleProfile(const RegKey& key,
+                         const RegToFieldMap& reg_to_field,
+                         AutofillProfile* profile) {
+  PhoneNumber::PhoneCombineHelper phone;
+  bool has_non_empty_fields =
+      ImportSingleFormGroup(key, reg_to_field, profile, &phone);
+
   // Now re-construct the phones if needed.
   string16 constructed_number;
-  if (!phone.IsEmpty() &&
-      phone.ParseNumber(std::string("US"), &constructed_number)) {
+  const std::string app_locale = AutofillCountry::ApplicationLocale();
+  if (phone.ParseNumber(*profile, app_locale, &constructed_number)) {
+    has_non_empty_fields = true;
     profile->SetRawInfo(PHONE_HOME_WHOLE_NUMBER, constructed_number);
   }
 
@@ -232,7 +252,7 @@ bool ImportCurrentUserProfiles(std::vector<AutofillProfile>* profiles,
     key_name.append(iterator_profiles.Name());
     RegKey key(HKEY_CURRENT_USER, key_name.c_str(), KEY_READ);
     AutofillProfile profile;
-    if (ImportSingleProfile(&profile, &key, reg_to_field)) {
+    if (ImportSingleProfile(key, reg_to_field, &profile)) {
       // Combine phones into whole phone #.
       profiles->push_back(profile);
     }
@@ -241,8 +261,8 @@ bool ImportCurrentUserProfiles(std::vector<AutofillProfile>* profiles,
   string16 salt;
   RegKey cc_key(HKEY_CURRENT_USER, kCreditCardKey, KEY_READ);
   if (cc_key.Valid()) {
-    password_hash = ReadAndDecryptValue(&cc_key, kPasswordHashValue);
-    salt = ReadAndDecryptValue(&cc_key, kSaltValue);
+    password_hash = ReadAndDecryptValue(cc_key, kPasswordHashValue);
+    salt = ReadAndDecryptValue(cc_key, kSaltValue);
   }
 
   // We import CC profiles only if they are not password protected.
@@ -255,7 +275,7 @@ bool ImportCurrentUserProfiles(std::vector<AutofillProfile>* profiles,
       key_name.append(iterator_cc.Name());
       RegKey key(HKEY_CURRENT_USER, key_name.c_str(), KEY_READ);
       CreditCard credit_card;
-      if (ImportSingleProfile(&credit_card, &key, reg_to_field)) {
+      if (ImportSingleFormGroup(key, reg_to_field, &credit_card, NULL)) {
         // TODO(isherman): Call into GetInfo() below, rather than GetRawInfo().
         string16 cc_number = credit_card.GetRawInfo(CREDIT_CARD_NUMBER);
         if (!cc_number.empty())

chrome/browser/autofill/personal_data_manager.cc

@@ -294,8 +294,7 @@ bool PersonalDataManager::ImportFormData(
   // Construct the phone number. Reject the profile if the number is invalid.
   if (imported_profile.get() && !home.IsEmpty()) {
     string16 constructed_number;
-    if (!home.ParseNumber(imported_profile->CountryCode(),
-                          &constructed_number) ||
+    if (!home.ParseNumber(*imported_profile, app_locale, &constructed_number) ||
         !imported_profile->SetInfo(PHONE_HOME_WHOLE_NUMBER, constructed_number,
                                    app_locale)) {
       imported_profile.reset();

chrome/browser/autofill/phone_number.cc

@@ -28,6 +28,19 @@ void StripPunctuation(string16* number) {
   RemoveChars(*number, kPhoneNumberSeparators, number);
 }
 
+// Returns the region code for this phone number, which is an ISO 3166 2-letter
+// country code.  The returned value is based on the |profile|; if the |profile|
+// does not have a country code associated with it, falls back to the country
+// code corresponding to the |app_locale|.
+std::string GetRegion(const AutofillProfile& profile,
+                      const std::string& app_locale) {
+  std::string country_code = profile.CountryCode();
+  if (!country_code.empty())
+    return country_code;
+
+  return AutofillCountry::CountryCodeForLocale(app_locale);
+}
+
 }  // namespace
 
 PhoneNumber::PhoneNumber(AutofillProfile* profile)
@@ -97,7 +110,8 @@ string16 PhoneNumber::GetInfo(AutofillFieldType type,
 
     // TODO(isherman): Can/should this use the cached_parsed_phone_?
     string16 normalized_phone =
-        autofill_i18n::NormalizePhoneNumber(phone, GetRegion(app_locale));
+        autofill_i18n::NormalizePhoneNumber(phone,
+                                            GetRegion(*profile_, app_locale));
     return !normalized_phone.empty() ? normalized_phone : phone;
   }
 
@@ -156,7 +170,7 @@ void PhoneNumber::GetMatchingTypes(const string16& text,
   // For US numbers, also compare to the three-digit prefix and the four-digit
   // suffix, since web sites often split numbers into these two fields.
   string16 number = GetInfo(PHONE_HOME_NUMBER, app_locale);
-  if (GetRegion(app_locale) == "US" &&
+  if (GetRegion(*profile_, app_locale) == "US" &&
       number.size() == (kPrefixLength + kSuffixLength)) {
     string16 prefix = number.substr(kPrefixOffset, kPrefixLength);
     string16 suffix = number.substr(kSuffixOffset, kSuffixLength);
@@ -167,22 +181,15 @@ void PhoneNumber::GetMatchingTypes(const string16& text,
   string16 whole_number = GetInfo(PHONE_HOME_WHOLE_NUMBER, app_locale);
   if (!whole_number.empty()) {
     string16 normalized_number =
-        autofill_i18n::NormalizePhoneNumber(text, GetRegion(app_locale));
+        autofill_i18n::NormalizePhoneNumber(text,
+                                            GetRegion(*profile_, app_locale));
     if (normalized_number == whole_number)
       matching_types->insert(PHONE_HOME_WHOLE_NUMBER);
   }
 }
 
-std::string PhoneNumber::GetRegion(const std::string& app_locale) const {
-  const std::string country_code = profile_->CountryCode();
-  if (country_code.empty())
-    return AutofillCountry::CountryCodeForLocale(app_locale);
-
-  return country_code;
-}
-
 void PhoneNumber::UpdateCacheIfNeeded(const std::string& app_locale) const {
-  std::string region = GetRegion(app_locale);
+  std::string region = GetRegion(*profile_, app_locale);
   if (!number_.empty() && cached_parsed_phone_.GetRegion() != region)
     cached_parsed_phone_ = autofill_i18n::PhoneObject(number_, region);
 }
@@ -223,15 +230,20 @@ bool PhoneNumber::PhoneCombineHelper::SetInfo(AutofillFieldType field_type,
   return false;
 }
 
-bool PhoneNumber::PhoneCombineHelper::ParseNumber(const std::string& region,
-                                                  string16* value) {
+bool PhoneNumber::PhoneCombineHelper::ParseNumber(
+    const AutofillProfile& profile,
+    const std::string& app_locale,
+    string16* value) {
+  if (IsEmpty())
+    return false;
+
   if (!whole_number_.empty()) {
     *value = whole_number_;
     return true;
   }
 
   return autofill_i18n::ConstructPhoneNumber(
-      country_, city_, phone_, region,
+      country_, city_, phone_, GetRegion(profile, app_locale),
       (country_.empty() ?
           autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
       value);

chrome/browser/autofill/phone_number.h

@@ -56,8 +56,13 @@ class PhoneNumber : public FormGroup {
     // returns true.  For all other field types returs false.
     bool SetInfo(AutofillFieldType type, const string16& value);
 
-    // Returns true if parsing was successful, false otherwise.
-    bool ParseNumber(const std::string& region, string16* value);
+    // Parses the number built up from pieces stored via SetInfo() according to
+    // the specified |profile|'s country code, falling back to the given
+    // |app_locale| if the |profile| has no associated country code.  Returns
+    // true if parsing was successful, false otherwise.
+    bool ParseNumber(const AutofillProfile& profile,
+                     const std::string& app_locale,
+                     string16* value);
 
     // Returns true if both |phone_| and |whole_number_| are empty.
     bool IsEmpty() const;
@@ -73,13 +78,6 @@ class PhoneNumber : public FormGroup {
   // FormGroup:
   virtual void GetSupportedTypes(FieldTypeSet* supported_types) const OVERRIDE;
 
-  // Returns the region code for this phone number, which is an ISO 3166
-  // 2-letter country code.  The name "region" is chosen since "country code"
-  // already refers to part of a phone number.  The returned value is based on
-  // the |profile_|; if the |profile_| does not have a country code associated
-  // with it, falls back to the country code corresponding to the |app_locale|.
-  std::string GetRegion(const std::string& app_locale) const;
-
   // Updates the cached parsed number if the profile's region has changed
   // since the last time the cache was updated.
   void UpdateCacheIfNeeded(const std::string& app_locale) const;

chrome/browser/autofill/phone_number_unittest.cc

@@ -81,6 +81,9 @@ TEST(PhoneNumberTest, Matcher) {
 }
 
 TEST(PhoneNumberTest, PhoneCombineHelper) {
+  AutofillProfile profile;
+  profile.SetCountryCode("US");
+
   PhoneNumber::PhoneCombineHelper number1;
   EXPECT_FALSE(number1.SetInfo(ADDRESS_BILLING_CITY,
                                ASCIIToUTF16("1")));
@@ -91,7 +94,7 @@ TEST(PhoneNumberTest, PhoneCombineHelper) {
   EXPECT_TRUE(number1.SetInfo(PHONE_HOME_NUMBER,
                               ASCIIToUTF16("2345678")));
   string16 parsed_phone;
-  EXPECT_TRUE(number1.ParseNumber("US", &parsed_phone));
+  EXPECT_TRUE(number1.ParseNumber(profile, "en-US", &parsed_phone));
   // International format as it has a country code.
   EXPECT_EQ(ASCIIToUTF16("+1 650-234-5678"), parsed_phone);
 
@@ -100,7 +103,7 @@ TEST(PhoneNumberTest, PhoneCombineHelper) {
                               ASCIIToUTF16("650")));
   EXPECT_TRUE(number3.SetInfo(PHONE_HOME_NUMBER,
                               ASCIIToUTF16("2345680")));
-  EXPECT_TRUE(number3.ParseNumber("US", &parsed_phone));
+  EXPECT_TRUE(number3.ParseNumber(profile, "en-US", &parsed_phone));
   // National format as it does not have a country code.
   EXPECT_EQ(ASCIIToUTF16("(650) 234-5680"), parsed_phone);
 
@@ -109,13 +112,13 @@ TEST(PhoneNumberTest, PhoneCombineHelper) {
                               ASCIIToUTF16("123")));  // Incorrect city code.
   EXPECT_TRUE(number4.SetInfo(PHONE_HOME_NUMBER,
                               ASCIIToUTF16("2345680")));
-  EXPECT_FALSE(number4.ParseNumber("US", &parsed_phone));
+  EXPECT_FALSE(number4.ParseNumber(profile, "en-US", &parsed_phone));
   EXPECT_EQ(string16(), parsed_phone);
 
   PhoneNumber::PhoneCombineHelper number5;
   EXPECT_TRUE(number5.SetInfo(PHONE_HOME_CITY_AND_NUMBER,
                               ASCIIToUTF16("6502345681")));
-  EXPECT_TRUE(number5.ParseNumber("US", &parsed_phone));
+  EXPECT_TRUE(number5.ParseNumber(profile, "en-US", &parsed_phone));
   EXPECT_EQ(ASCIIToUTF16("(650) 234-5681"), parsed_phone);
 
   PhoneNumber::PhoneCombineHelper number6;
@@ -125,6 +128,18 @@ TEST(PhoneNumberTest, PhoneCombineHelper) {
                               ASCIIToUTF16("234")));
   EXPECT_TRUE(number6.SetInfo(PHONE_HOME_NUMBER,
                               ASCIIToUTF16("5682")));
-  EXPECT_TRUE(number6.ParseNumber("US", &parsed_phone));
+  EXPECT_TRUE(number6.ParseNumber(profile, "en-US", &parsed_phone));
+  EXPECT_EQ(ASCIIToUTF16("(650) 234-5682"), parsed_phone);
+
+  // Ensure parsing is possible when falling back to detecting the country code
+  // based on the app locale.
+  PhoneNumber::PhoneCombineHelper number7;
+  EXPECT_TRUE(number7.SetInfo(PHONE_HOME_CITY_CODE,
+                              ASCIIToUTF16("650")));
+  EXPECT_TRUE(number7.SetInfo(PHONE_HOME_NUMBER,
+                              ASCIIToUTF16("234")));
+  EXPECT_TRUE(number7.SetInfo(PHONE_HOME_NUMBER,
+                              ASCIIToUTF16("5682")));
+  EXPECT_TRUE(number7.ParseNumber(AutofillProfile(), "en-US", &parsed_phone));
   EXPECT_EQ(ASCIIToUTF16("(650) 234-5682"), parsed_phone);
 }