These changes *are* for review :)

chrome/browser/autofill/autofill_ie_toolbar_import_win.cc

 // Copyright (c) 2011 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 "chrome/browser/autofill/autofill_ie_toolbar_import_win.h"
 
 #include <stddef.h>
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/string16.h"
 #include "base/win/registry.h"
 #include "chrome/browser/autofill/autofill_profile.h"
 #include "chrome/browser/autofill/credit_card.h"
 #include "chrome/browser/autofill/crypto/rc4_decryptor.h"
 #include "chrome/browser/autofill/field_types.h"
 #include "chrome/browser/autofill/form_group.h"
 #include "chrome/browser/autofill/personal_data_manager.h"
+#include "chrome/browser/autofill/phone_number_i18n.h"
 #include "chrome/browser/sync/util/data_encryption.h"
 
 using base::win::RegKey;
 
 // Forward declaration. This function is not in unnamed namespace as it
 // is referenced in the unittest.
 bool ImportCurrentUserProfiles(std::vector<AutofillProfile>* profiles,
                                std::vector<CreditCard>* credit_cards);
 namespace {
 
@@ skipping 92 matching lines @@
 
 bool ImportSingleProfile(FormGroup* profile,
                          RegKey* key,
                          const RegToFieldMap& reg_to_field ) {
   DCHECK(profile != NULL);
   if (!key->Valid())
     return false;
 
   bool has_non_empty_fields = false;
 
+  // Phones need to be rebuild.

[dhollowa, 2011/05/13 18:55:35]

nit: s/rebuild/rebuilt/

[GeorgeY, 2011/05/18 17:41:45]

Done.

+  string16 home, home_city, home_country;
+  string16 fax, fax_city, fax_country;
+
   for (uint32 value_index = 0; value_index < key->ValueCount(); ++value_index) {
     std::wstring value_name;
     if (key->ReadName(value_index, &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);
       }
-      profile->SetInfo(it->second, field_value);
+      switch (it->second) {

[dhollowa, 2011/05/13 18:55:35]

This is getting pretty hard to read with |SetInfo| at line 174 and then below at
185 and 192.  It is not immediately clear how to make this more readable though.
 Do you have any ideas on how we might split out a helper function or something
to deal with phone specific logic?

[GeorgeY, 2011/05/18 17:41:45]

Simplified it

+        case PHONE_HOME_CITY_CODE:
+          home_city = field_value;
+          break;
+        case PHONE_HOME_COUNTRY_CODE:
+          home_country = field_value;
+          break;
+        case PHONE_HOME_CITY_AND_NUMBER:
+        case PHONE_HOME_NUMBER:
+          home = field_value;
+          break;
+        case PHONE_FAX_CITY_CODE:
+          fax_city = field_value;
+          break;
+        case PHONE_FAX_COUNTRY_CODE:
+          fax_country = field_value;
+          break;
+        case PHONE_FAX_NUMBER:
+        case PHONE_FAX_CITY_AND_NUMBER:
+          fax = field_value;
+          break;
+        default:
+          profile->SetInfo(it->second, field_value);
+          break;
+      }
     }
   }
+  string16 constructed_number;
+  if (!home.empty() && autofill_i18n::ConstructPhoneNumber(
+          home_country, home_city, home, std::string("US"),
+          (home_country.empty() ?
+              autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
+          &constructed_number)) {
+    profile->SetInfo(PHONE_HOME_WHOLE_NUMBER, constructed_number);
+  }
+  if (!fax.empty() && autofill_i18n::ConstructPhoneNumber(
+          fax_country, fax_city, fax, std::string("US"),
+          (fax_country.empty() ?
+              autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
+          &constructed_number)) {
+    profile->SetInfo(PHONE_FAX_WHOLE_NUMBER, constructed_number);
+  }
+
   return has_non_empty_fields;
 }
 
 // Imports profiles from the IE toolbar and stores them. Asynchronous
 // if PersonalDataManager has not been loaded yet. Deletes itself on completion.
 class AutofillImporter : public PersonalDataManager::Observer {
  public:
   explicit AutofillImporter(PersonalDataManager* personal_data_manager)
     : personal_data_manager_(personal_data_manager) {
       personal_data_manager_->SetObserver(this);
@@ skipping 48 matching lines @@
   base::win::RegistryKeyIterator iterator_profiles(HKEY_CURRENT_USER,
                                                    kProfileKey);
   for (; iterator_profiles.Valid(); ++iterator_profiles) {
     std::wstring key_name(kProfileKey);
     key_name.append(L"\\");
     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)) {
       // Combine phones into whole phone #.
-      string16 phone;
-      phone = profile.GetInfo(PHONE_HOME_COUNTRY_CODE);
-      phone.append(profile.GetInfo(PHONE_HOME_CITY_CODE));
-      phone.append(profile.GetInfo(PHONE_HOME_NUMBER));
-      profile.SetInfo(PHONE_HOME_WHOLE_NUMBER, phone);
-      phone = profile.GetInfo(PHONE_FAX_COUNTRY_CODE);
-      phone.append(profile.GetInfo(PHONE_FAX_CITY_CODE));
-      phone.append(profile.GetInfo(PHONE_FAX_NUMBER));
-      profile.SetInfo(PHONE_FAX_WHOLE_NUMBER, phone);
       profiles->push_back(profile);
     }
   }
   string16 password_hash;
   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);
   }
@@ skipping 20 matching lines @@
 
 bool ImportAutofillDataWin(PersonalDataManager* pdm) {
   // In incognito mode we do not have PDM - and we should not import anything.
   if (!pdm)
     return false;
   AutofillImporter *importer = new AutofillImporter(pdm);
   // importer will self delete.
   return importer->ImportProfiles();
 }