These changes *are* for review :)

chrome/browser/autofill/personal_data_manager.cc

Index: chrome/browser/autofill/personal_data_manager.cc
===================================================================
--- chrome/browser/autofill/personal_data_manager.cc	(revision 84722)
+++ chrome/browser/autofill/personal_data_manager.cc	(working copy)
@@ -15,6 +15,7 @@
 #include "chrome/browser/autofill/autofill_metrics.h"
 #include "chrome/browser/autofill/form_structure.h"
 #include "chrome/browser/autofill/phone_number.h"
+#include "chrome/browser/autofill/phone_number_i18n.h"
 #include "chrome/browser/autofill/select_control_handler.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -202,6 +203,15 @@
   // Detect and discard forms with multiple fields of the same type.
   std::set<AutofillFieldType> types_seen;
 
+  // We only set complete phone, so aggregate phone parts in these vars and set

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

The phone/fax specific logic in this function is getting overly large.  I'm
wondering if we can package up the intermediate components of the phone numbers
(home_number, home_city_code, etc...) into a utility class, and then move the
assembly of the final |constructed_number| below off into that utility class.

Thoughts?

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

simplified it (see code)

+  // complete at the end.
+  string16 home_number;
+  string16 home_city_code;
+  string16 home_country_code;
+  string16 fax_number;
+  string16 fax_city_code;
+  string16 fax_country_code;
+
   for (size_t i = 0; i < form.field_count(); ++i) {
     const AutofillField* field = form.field(i);
     string16 value = CollapseWhitespace(field->value, false);
@@ -245,52 +255,42 @@
         ++importable_credit_card_fields;
       }
     } else {
-      // In the case of a phone number, if the whole phone number was entered
-      // into a single field, then parse it and set the sub components.
-      if (AutofillType(field_type).subgroup() ==
-              AutofillType::PHONE_WHOLE_NUMBER) {
-        string16 number;
-        string16 city_code;
-        string16 country_code;
-        PhoneNumber::ParsePhoneNumber(value,
-                                      &number,
-                                      &city_code,
-                                      &country_code);
-        if (number.empty())
-          continue;
-
-        if (group == AutofillType::PHONE_HOME) {
-          imported_profile->SetInfo(PHONE_HOME_COUNTRY_CODE, country_code);
-          imported_profile->SetInfo(PHONE_HOME_CITY_CODE, city_code);
-          imported_profile->SetInfo(PHONE_HOME_NUMBER, number);
-        } else if (group == AutofillType::PHONE_FAX) {
-          imported_profile->SetInfo(PHONE_FAX_COUNTRY_CODE, country_code);
-          imported_profile->SetInfo(PHONE_FAX_CITY_CODE, city_code);
-          imported_profile->SetInfo(PHONE_FAX_NUMBER, number);
-        }
-
-        continue;
-      }
-
       // Phone and fax numbers can be split across multiple fields, so we
       // might have already stored the prefix, and now be at the suffix.
       // If so, combine them to form the full number.
-      if (group == AutofillType::PHONE_HOME ||
-          group == AutofillType::PHONE_FAX) {
-        AutofillFieldType number_type = PHONE_HOME_NUMBER;
-        if (group == AutofillType::PHONE_FAX)
-          number_type = PHONE_FAX_NUMBER;
-
-        string16 stored_number = imported_profile->GetInfo(number_type);
-        if (stored_number.size() ==
-                static_cast<size_t>(PhoneNumber::kPrefixLength) &&
-            value.size() == static_cast<size_t>(PhoneNumber::kSuffixLength)) {
-          value = stored_number + value;
-        }
+      switch (field_type) {
+        case PHONE_HOME_CITY_CODE:
+          home_city_code = value;
+          break;
+        case PHONE_HOME_COUNTRY_CODE:
+          home_country_code = value;
+          break;
+        case PHONE_HOME_CITY_AND_NUMBER:
+          home_number = value;
+          break;
+        // Phone and fax numbers can be split across multiple fields, so we
+        // might have already stored the prefix, and now be at the suffix.
+        // If so, combine them to form the full number.
+        case PHONE_HOME_NUMBER:
+          home_number.append(value);
+          break;
+        case PHONE_FAX_CITY_CODE:
+          fax_city_code = value;
+          break;
+        case PHONE_FAX_COUNTRY_CODE:
+          fax_country_code = value;
+          break;
+        case PHONE_FAX_NUMBER:
+          fax_number.append(value);
+          break;
+        case PHONE_FAX_CITY_AND_NUMBER:
+          fax_number = value;
+          break;
+        default:
+          imported_profile->SetInfo(field_type, value);
+          break;
       }
 
-      imported_profile->SetInfo(field_type, value);
-
       // Reject profiles with invalid country information.
       if (field_type == ADDRESS_HOME_COUNTRY &&
           !value.empty() && imported_profile->CountryCode().empty()) {
@@ -300,6 +300,42 @@
     }
   }
 
+  // Build phone numbers if they are from parts.
+  if (imported_profile.get()) {
+    if (!home_number.empty()) {
+      string16 constructed_number;
+      if (!autofill_i18n::ConstructPhoneNumber(
+              home_country_code, home_city_code, home_number,
+              imported_profile->CountryCode(),
+              (home_country_code.empty() ?
+                  autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
+              &constructed_number)) {
+        imported_profile.reset();
+      } else {
+        imported_profile->SetInfo(PHONE_HOME_WHOLE_NUMBER, constructed_number);
+      }
+    }
+    if (!fax_number.empty()) {
+      string16 constructed_number;
+      if (!autofill_i18n::ConstructPhoneNumber(
+              fax_country_code, fax_city_code, fax_number,
+              imported_profile->CountryCode(),
+              (fax_country_code.empty() ?
+                  autofill_i18n::NATIONAL : autofill_i18n::INTERNATIONAL),
+              &constructed_number)) {
+        imported_profile.reset();
+      } else {
+        imported_profile->SetInfo(PHONE_FAX_WHOLE_NUMBER, constructed_number);
+      }
+    }
+  }
+  // Normalize phone numbers.
+  if (imported_profile.get()) {
+    // Reject profile if even one of the phones is invalid.
+    if (!imported_profile->NormalizePhones())

[Ilya Sherman, 2011/05/14 04:38:51]

It looks like this is the only place where NormalizePhones() is used -- perhaps
that logic should be local to the PersonalDataManager implementation?

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

home_number_, fax_number_ are encapsulated in profile and are local to that

+      imported_profile.reset();
+  }
+
   // Reject the profile if minimum address and validation requirements are not
   // met.
   if (imported_profile.get() && !IsValidLearnableProfile(*imported_profile))