These changes *are* for review :)

chrome/browser/autofill/personal_data_manager.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.

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

personal_data_manager_mac.cc needs to be fixed.  It is not currently compiling.

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

Commented it out for now, could you, please, fix it the same way it is changed
in .cc?

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autofill/personal_data_manager.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill-inl.h"
 #include "chrome/browser/autofill/autofill_field.h"
 #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"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/pref_names.h"
 #include "content/browser/browser_thread.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebRegularExpression.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
 
@@ skipping 167 matching lines @@
   scoped_ptr<CreditCard> local_imported_credit_card(new CreditCard);
 
   // Parse the form and construct a profile based on the information that is
   // possible to import.
   int importable_credit_card_fields = 0;
   std::vector<const FormStructure*>::const_iterator iter;
 
   // 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);
 
     // If we don't know the type of the field, or the user hasn't entered any
     // information into the field, then skip it.
     if (!field->IsFieldFillable() || value.empty())
       continue;
 
     AutofillFieldType field_type = field->type();
@@ skipping 23 matching lines @@
         } else {
           if (field_type == CREDIT_CARD_NUMBER) {
             // Clean up any imported credit card numbers.
             value = CreditCard::StripSeparators(value);
           }
           local_imported_credit_card->SetInfo(field_type, value);
         }
         ++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()) {
         imported_profile.reset();
         break;
       }
     }
   }
 
+  // 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))
     imported_profile.reset();
 
   // Reject the credit card if we did not detect enough filled credit card
   // fields or if the credit card number does not seem to be valid.
   if (local_imported_credit_card.get() &&
       (importable_credit_card_fields < kMinCreditCardImportSize ||
        !CreditCard::IsValidCreditCardNumber(
@@ skipping 624 matching lines @@
 }
 
 const AutofillMetrics* PersonalDataManager::metric_logger() const {
   return metric_logger_.get();
 }
 
 void PersonalDataManager::set_metric_logger(
     const AutofillMetrics* metric_logger) {
   metric_logger_.reset(metric_logger);
 }