Merge 40655 - AutoFill phone number parser changes.

Moved the PersonalDataManager::ParsePhoneNumber() method into the PhoneNumber class.  Made some bug fixes to the parser.  Extended its functionality to strip separator characters.  Added unit tests to cover the parser.

BUG=38240
TEST=PhoneNumberTest

Review URL: http://codereview.chromium.org/669026

TBR=dhollowa@chromium.org
Review URL: http://codereview.chromium.org/986003

git-svn-id: svn://svn.chromium.org/chrome/branches/342/src@41651 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 186 insertions, 52 deletions

diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index 2526109..1e9ca9a 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/autofill/autofill_manager.h"
 #include "chrome/browser/autofill/autofill_field.h"
 #include "chrome/browser/autofill/form_structure.h"
+#include "chrome/browser/autofill/phone_number.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/webdata/web_data_service.h"
 
@@ -18,12 +19,6 @@
 // before autofill will attempt to import the data into a profile.
 static const int kMinImportSize = 5;
 
-// The number of digits in a phone number.
-static const int kPhoneNumberLength = 7;
-
-// The number of digits in an area code.
-static const int kPhoneCityCodeLength = 3;
-
 PersonalDataManager::~PersonalDataManager() {
   CancelPendingQuery(&pending_profiles_query_);
   CancelPendingQuery(&pending_creditcards_query_);
@@ -149,12 +144,27 @@ bool PersonalDataManager::ImportFormData(
         // 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 (field_type.subgroup() == AutoFillType::PHONE_WHOLE_NUMBER) {
+          string16 number;
+          string16 city_code;
+          string16 country_code;
           if (group == AutoFillType::PHONE_HOME) {
-            ParsePhoneNumber(imported_profile_.get(), &value, PHONE_HOME_NUMBER,
-                             PHONE_HOME_CITY_CODE, PHONE_HOME_COUNTRY_CODE);
+            PhoneNumber::ParsePhoneNumber(
+                value, &number, &city_code, &country_code);
+            imported_profile_->SetInfo(
+                AutoFillType(PHONE_HOME_COUNTRY_CODE), country_code);
+            imported_profile_->SetInfo(
+                AutoFillType(PHONE_HOME_CITY_CODE), city_code);
+            imported_profile_->SetInfo(
+                AutoFillType(PHONE_HOME_NUMBER), number);
           } else if (group == AutoFillType::PHONE_FAX) {
-            ParsePhoneNumber(imported_profile_.get(), &value, PHONE_FAX_NUMBER,
-                             PHONE_FAX_CITY_CODE, PHONE_FAX_COUNTRY_CODE);
+            PhoneNumber::ParsePhoneNumber(
+                value, &number, &city_code, &country_code);
+            imported_profile_->SetInfo(
+                AutoFillType(PHONE_FAX_COUNTRY_CODE), country_code);
+            imported_profile_->SetInfo(
+                AutoFillType(PHONE_FAX_CITY_CODE), city_code);
+            imported_profile_->SetInfo(
+                AutoFillType(PHONE_FAX_NUMBER), number);
           }
           continue;
         }
@@ -373,32 +383,6 @@ int PersonalDataManager::CreateNextUniqueID(std::set<int>* unique_ids) {
   return id;
 }
 
-void PersonalDataManager::ParsePhoneNumber(
-    AutoFillProfile* profile,
-    string16* value,
-    AutoFillFieldType number,
-    AutoFillFieldType city_code,
-    AutoFillFieldType country_code) const {
-  // Treat the last 7 digits as the number.
-  string16 number_str = value->substr(kPhoneNumberLength,
-                                      value->size() - kPhoneNumberLength);
-  profile->SetInfo(AutoFillType(number), number_str);
-  value->resize(value->size() - kPhoneNumberLength);
-  if (value->empty())
-    return;
-
-  // Treat the next three digits as the city code.
-  string16 city_code_str = value->substr(kPhoneCityCodeLength,
-                                         value->size() - kPhoneCityCodeLength);
-  profile->SetInfo(AutoFillType(city_code), city_code_str);
-  value->resize(value->size() - kPhoneCityCodeLength);
-  if (value->empty())
-    return;
-
-  // Treat any remaining digits as the country code.
-  profile->SetInfo(AutoFillType(country_code), *value);
-}
-
 void PersonalDataManager::LoadProfiles() {
   WebDataService* web_data_service =
       profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
diff --git a/chrome/browser/autofill/personal_data_manager.h b/chrome/browser/autofill/personal_data_manager.h
index 1dfbd1f..403d871 100644
--- a/chrome/browser/autofill/personal_data_manager.h
+++ b/chrome/browser/autofill/personal_data_manager.h
@@ -110,16 +110,6 @@ class PersonalDataManager : public WebDataServiceConsumer,
   // This will create and reserve a new unique ID for a profile.
   int CreateNextUniqueID(std::set<int>* unique_ids);
 
-  // Parses value to extract the components of a phone number and adds them to
-  // profile.
-  //
-  // TODO(jhawkins): Investigate if this can be moved to PhoneField.
-  void ParsePhoneNumber(AutoFillProfile* profile,
-                        string16* value,
-                        AutoFillFieldType number,
-                        AutoFillFieldType city_code,
-                        AutoFillFieldType country_code) const;
-
   // Loads the saved profiles from the web database.
   void LoadProfiles();
 
diff --git a/chrome/browser/autofill/phone_number.cc b/chrome/browser/autofill/phone_number.cc
index 8c4de77..e8829aa 100644
--- a/chrome/browser/autofill/phone_number.cc
+++ b/chrome/browser/autofill/phone_number.cc
@@ -6,12 +6,21 @@
 
 #include "base/basictypes.h"
 #include "base/string_util.h"
+#include "chrome/browser/autofill/autofill_profile.h"
 #include "chrome/browser/autofill/autofill_type.h"
 #include "chrome/browser/autofill/field_types.h"
 
-static const string16 kPhoneNumberSeparators = ASCIIToUTF16(" .()-");
+namespace {
 
-static const AutoFillType::FieldTypeSubGroup kAutoFillPhoneTypes[] = {
+const char16 kPhoneNumberSeparators[] = { ' ', '.', '(', ')', '-', 0 };
+
+// The number of digits in a phone number.
+const size_t kPhoneNumberLength = 7;
+
+// The number of digits in an area code.
+const size_t kPhoneCityCodeLength = 3;
+
+const AutoFillType::FieldTypeSubGroup kAutoFillPhoneTypes[] = {
   AutoFillType::PHONE_NUMBER,
   AutoFillType::PHONE_CITY_CODE,
   AutoFillType::PHONE_COUNTRY_CODE,
@@ -19,7 +28,9 @@ static const AutoFillType::FieldTypeSubGroup kAutoFillPhoneTypes[] = {
   AutoFillType::PHONE_WHOLE_NUMBER,
 };
 
-static const int kAutoFillPhoneLength = arraysize(kAutoFillPhoneTypes);
+const int kAutoFillPhoneLength = arraysize(kAutoFillPhoneTypes);
+
+}  // namespace
 
 void PhoneNumber::GetPossibleFieldTypes(const string16& text,
                                         FieldTypeSet* possible_types) const {
@@ -109,6 +120,46 @@ void PhoneNumber::SetInfo(const AutoFillType& type, const string16& value) {
   //   set_extension(number);
 }
 
+// Static.
+void PhoneNumber::ParsePhoneNumber(const string16& value,
+                                   string16* number,
+                                   string16* city_code,
+                                   string16* country_code) {
+  DCHECK(number);
+  DCHECK(city_code);
+  DCHECK(country_code);
+
+  // Make a working copy of value.
+  string16 working = value;
+
+  *number = string16();
+  *city_code = string16();
+  *country_code = string16();
+
+  // First remove any punctuation.
+  StripPunctuation(&working);
+
+  if (working.size() < kPhoneNumberLength)
+    return;
+
+  // Treat the last 7 digits as the number.
+  *number = working.substr(working.size() - kPhoneNumberLength,
+                           kPhoneNumberLength);
+  working.resize(working.size() - kPhoneNumberLength);
+  if (working.size() < kPhoneCityCodeLength)
+    return;
+
+  // Treat the next three digits as the city code.
+  *city_code = working.substr(working.size() - kPhoneCityCodeLength,
+                              kPhoneCityCodeLength);
+  working.resize(working.size() - kPhoneCityCodeLength);
+  if (working.empty())
+    return;
+
+  // Treat any remaining digits as the country code.
+  *country_code = working;
+}
+
 string16 PhoneNumber::WholeNumber() const {
   string16 whole_number;
   if (!country_code_.empty())
@@ -212,6 +263,7 @@ bool PhoneNumber::Validate(const string16& number) const {
   return true;
 }
 
-void PhoneNumber::StripPunctuation(string16* number) const {
-  RemoveChars(*number, kPhoneNumberSeparators.c_str(), number);
+// Static.
+void PhoneNumber::StripPunctuation(string16* number) {
+  RemoveChars(*number, kPhoneNumberSeparators, number);
 }
diff --git a/chrome/browser/autofill/phone_number.h b/chrome/browser/autofill/phone_number.h
index c7cfc59..5e8affe 100644
--- a/chrome/browser/autofill/phone_number.h
+++ b/chrome/browser/autofill/phone_number.h
@@ -26,6 +26,15 @@ class PhoneNumber : public FormGroup {
   virtual string16 GetFieldText(const AutoFillType& type) const;
   virtual void SetInfo(const AutoFillType& type, const string16& value);
 
+  // Parses |value| to extract the components of a phone number.  |number|
+  // returns the trailing 7 digits, |city_code| returns the next 3 digits, and
+  // |country_code| returns any remaining digits.
+  // Separator characters are stripped before parsing the digits.
+  static void ParsePhoneNumber(const string16& value,
+                               string16* number,
+                               string16* city_code,
+                               string16* country_code);
+
  protected:
   explicit PhoneNumber(const PhoneNumber& phone_number);
 
@@ -74,7 +83,7 @@ class PhoneNumber : public FormGroup {
   bool Validate(const string16& number) const;
 
   // Removes any punctuation characters from |number|.
-  void StripPunctuation(string16* number) const;
+  static void StripPunctuation(string16* number);
 
   // The pieces of the phone number.
   string16 country_code_;
diff --git a/chrome/browser/autofill/phone_number_unittest.cc b/chrome/browser/autofill/phone_number_unittest.cc
new file mode 100644
index 0000000..878031f
--- /dev/null
+++ b/chrome/browser/autofill/phone_number_unittest.cc
@@ -0,0 +1,98 @@
+// Copyright (c) 2010 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/phone_number.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Tests the phone number parser.
+TEST(PhoneNumberTest, Parser) {
+  string16 number;
+  string16 city_code;
+  string16 country_code;
+
+  // Test for empty string.  Should give back empty strings.
+  string16 phone0;
+  PhoneNumber::ParsePhoneNumber(phone0, &number, &city_code, &country_code);
+  EXPECT_EQ(string16(), number);
+  EXPECT_EQ(string16(), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with less than 7 digits.  Should give back empty strings.
+  string16 phone1(ASCIIToUTF16("1234"));
+  PhoneNumber::ParsePhoneNumber(phone1, &number, &city_code, &country_code);
+  EXPECT_EQ(string16(), number);
+  EXPECT_EQ(string16(), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with exactly 7 digits.  Should give back only phone number.
+  string16 phone2(ASCIIToUTF16("1234567"));
+  PhoneNumber::ParsePhoneNumber(phone2, &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("1234567"), number);
+  EXPECT_EQ(string16(), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with exactly 7 digits and separators.  Should give back
+  // only phone number.
+  string16 phone_separator2(ASCIIToUTF16("123-4567"));
+  PhoneNumber::ParsePhoneNumber(phone_separator2,
+      &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("1234567"), number);
+  EXPECT_EQ(string16(), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with greater than 7 digits but less than 10 digits.
+  // Should give back only phone number.
+  string16 phone3(ASCIIToUTF16("123456789"));
+  PhoneNumber::ParsePhoneNumber(phone3, &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("3456789"), number);
+  EXPECT_EQ(string16(), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with greater than 7 digits but less than 10 digits and
+  // separators.
+  // Should give back only phone number.
+  string16 phone_separator3(ASCIIToUTF16("12.345-6789"));
+  PhoneNumber::ParsePhoneNumber(phone3, &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("3456789"), number);
+  EXPECT_EQ(string16(), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with exactly 10 digits.
+  // Should give back phone number and city code.
+  string16 phone4(ASCIIToUTF16("1234567890"));
+  PhoneNumber::ParsePhoneNumber(phone4, &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("4567890"), number);
+  EXPECT_EQ(ASCIIToUTF16("123"), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with exactly 10 digits and separators.
+  // Should give back phone number and city code.
+  string16 phone_separator4(ASCIIToUTF16("(123) 456-7890"));
+  PhoneNumber::ParsePhoneNumber(phone_separator4,
+      &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("4567890"), number);
+  EXPECT_EQ(ASCIIToUTF16("123"), city_code);
+  EXPECT_EQ(string16(), country_code);
+
+  // Test for string with over 10 digits.
+  // Should give back phone number, city code, and country code.
+  string16 phone5(ASCIIToUTF16("011234567890"));
+  PhoneNumber::ParsePhoneNumber(phone5, &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("4567890"), number);
+  EXPECT_EQ(ASCIIToUTF16("123"), city_code);
+  EXPECT_EQ(ASCIIToUTF16("01"), country_code);
+
+  // Test for string with over 10 digits with separator characters.
+  // Should give back phone number, city code, and country code.
+  string16 phone6(ASCIIToUTF16("(01) 123-456.7890"));
+  PhoneNumber::ParsePhoneNumber(phone6, &number, &city_code, &country_code);
+  EXPECT_EQ(ASCIIToUTF16("4567890"), number);
+  EXPECT_EQ(ASCIIToUTF16("123"), city_code);
+  EXPECT_EQ(ASCIIToUTF16("01"), country_code);
+}
+
+}  // namespace
+
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index f4553ec..b8f40b9 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -539,6 +539,7 @@
         'browser/autofill/credit_card_unittest.cc',
         'browser/autofill/form_structure_unittest.cc',
         'browser/autofill/personal_data_manager_unittest.cc',
+        'browser/autofill/phone_number_unittest.cc',
         'browser/automation/automation_provider_unittest.cc',
         'browser/back_forward_menu_model_unittest.cc',
         'browser/bookmarks/bookmark_codec_unittest.cc',