Autofill: Apply the same field match mask to all phone field subtypes.

- Modify unit tests to test relevant field types.
- Cleanup code.
- Update heuristics test expectations.

Review URL: https://codereview.chromium.org/1010743007

Cr-Commit-Position: refs/heads/master@{#322312}

chrome/test/data/autofill/heuristics/output/24_checkout_harryanddavid.com.out

@@ -24,7 +24,7 @@ ADDRESS_HOME_ZIP | zipCode | ZIP* |  | typeOfAddress_1-default
 UNKNOWN_TYPE |  | Phone number | Χ  | typeOfAddress_1-default
 PHONE_HOME_WHOLE_NUMBER | phone1 | Phone number |  | typeOfAddress_1-default
 UNKNOWN_TYPE |  | Ext | Χ  | typeOfAddress_1-default
-PHONE_HOME_WHOLE_NUMBER | phone2 | Ext |  | typeOfAddress_1-default
+UNKNOWN_TYPE | phone2 | Ext |  | typeOfAddress_1-default
 UNKNOWN_TYPE | phone1Type | Cell phone | C | typeOfAddress_1-default
 UNKNOWN_TYPE |  | Email address* | Χ  | typeOfAddress_1-default
 EMAIL_ADDRESS | email1 | Email address* |  | typeOfAddress_1-default

components/autofill/core/browser/phone_field.cc

@@ -17,10 +17,10 @@ namespace autofill {
 namespace {
 
 // This string includes all area code separators, including NoText.
-base::string16 GetAreaRegex() {
-  base::string16 area_code = base::UTF8ToUTF16(kAreaCodeRe);
-  area_code.append(base::ASCIIToUTF16("|"));  // Regexp separator.
-  area_code.append(base::UTF8ToUTF16(kAreaCodeNotextRe));
+std::string GetAreaRegex() {
+  std::string area_code = kAreaCodeRe;
+  area_code.append("|");  // Regexp separator.
+  area_code.append(kAreaCodeNotextRe);
   return area_code;
 }
 
@@ -119,24 +119,23 @@ const PhoneField::Parser PhoneField::kPhoneFieldGrammars[] = {
 // static
 scoped_ptr<FormField> PhoneField::Parse(AutofillScanner* scanner) {
   if (scanner->IsEnd())
-    return NULL;
+    return nullptr;
 
-  scanner->SaveCursor();
+  size_t start_cursor = scanner->SaveCursor();
 
   // The form owns the following variables, so they should not be deleted.
   AutofillField* parsed_fields[FIELD_MAX];
 
   for (size_t i = 0; i < arraysize(kPhoneFieldGrammars); ++i) {
     memset(parsed_fields, 0, sizeof(parsed_fields));
-    scanner->SaveCursor();
+    size_t saved_cursor = scanner->SaveCursor();
 
     // Attempt to parse according to the next grammar.
     for (; i < arraysize(kPhoneFieldGrammars) &&
          kPhoneFieldGrammars[i].regex != REGEX_SEPARATOR; ++i) {
-      if (!ParseFieldSpecifics(
+      if (!ParsePhoneField(
               scanner,
               GetRegExp(kPhoneFieldGrammars[i].regex),
-              MATCH_DEFAULT | MATCH_TELEPHONE,
               &parsed_fields[kPhoneFieldGrammars[i].phone_part]))
         break;
       if (kPhoneFieldGrammars[i].max_size &&
@@ -148,8 +147,8 @@ scoped_ptr<FormField> PhoneField::Parse(AutofillScanner* scanner) {
     }
 
     if (i >= arraysize(kPhoneFieldGrammars)) {
-      scanner->Rewind();
-      return NULL;  // Parsing failed.
+      scanner->RewindTo(saved_cursor);
+      return nullptr;  // Parsing failed.
     }
     if (kPhoneFieldGrammars[i].regex == REGEX_SEPARATOR)
       break;  // Parsing succeeded.
@@ -160,17 +159,15 @@ scoped_ptr<FormField> PhoneField::Parse(AutofillScanner* scanner) {
     } while (i < arraysize(kPhoneFieldGrammars) &&
              kPhoneFieldGrammars[i].regex != REGEX_SEPARATOR);
 
+    scanner->RewindTo(saved_cursor);
     if (i + 1 == arraysize(kPhoneFieldGrammars)) {
-      scanner->Rewind();
-      return NULL;  // Tried through all the possibilities - did not match.
+      return nullptr;  // Tried through all the possibilities - did not match.
     }
-
-    scanner->Rewind();
   }
 
   if (!parsed_fields[FIELD_PHONE]) {
-    scanner->Rewind();
-    return NULL;
+    scanner->RewindTo(start_cursor);
+    return nullptr;
   }
 
   scoped_ptr<PhoneField> phone_field(new PhoneField);
@@ -181,15 +178,18 @@ scoped_ptr<FormField> PhoneField::Parse(AutofillScanner* scanner) {
 
   // Look for a third text box.
   if (!phone_field->parsed_phone_fields_[FIELD_SUFFIX]) {
-    if (!ParseField(scanner, base::UTF8ToUTF16(kPhoneSuffixRe),
+    if (!ParsePhoneField(scanner, kPhoneSuffixRe,
                          &phone_field->parsed_phone_fields_[FIELD_SUFFIX])) {
-      ParseField(scanner, base::UTF8ToUTF16(kPhoneSuffixSeparatorRe),
+      ParsePhoneField(scanner, kPhoneSuffixSeparatorRe,
                       &phone_field->parsed_phone_fields_[FIELD_SUFFIX]);
     }
   }
 
   // Now look for an extension.
-  ParseField(scanner, base::UTF8ToUTF16(kPhoneExtensionRe),
+  // The extension is not actually used, so this just eats the field so other
+  // parsers do not mistaken it for something else.
+  ParsePhoneField(scanner,
+                  kPhoneExtensionRe,
                   &phone_field->parsed_phone_fields_[FIELD_EXTENSION]);
 
   return phone_field.Pass();
@@ -200,21 +200,21 @@ bool PhoneField::ClassifyField(ServerFieldTypeMap* map) const {
 
   DCHECK(parsed_phone_fields_[FIELD_PHONE]);  // Phone was correctly parsed.
 
-  if ((parsed_phone_fields_[FIELD_COUNTRY_CODE] != NULL) ||
-      (parsed_phone_fields_[FIELD_AREA_CODE] != NULL) ||
-      (parsed_phone_fields_[FIELD_SUFFIX] != NULL)) {
-    if (parsed_phone_fields_[FIELD_COUNTRY_CODE] != NULL) {
+  if ((parsed_phone_fields_[FIELD_COUNTRY_CODE]) ||
+      (parsed_phone_fields_[FIELD_AREA_CODE]) ||
+      (parsed_phone_fields_[FIELD_SUFFIX])) {
+    if (parsed_phone_fields_[FIELD_COUNTRY_CODE]) {
       ok = ok && AddClassification(parsed_phone_fields_[FIELD_COUNTRY_CODE],
                                    PHONE_HOME_COUNTRY_CODE,
                                    map);
     }
 
     ServerFieldType field_number_type = PHONE_HOME_NUMBER;
-    if (parsed_phone_fields_[FIELD_AREA_CODE] != NULL) {
+    if (parsed_phone_fields_[FIELD_AREA_CODE]) {
       ok = ok && AddClassification(parsed_phone_fields_[FIELD_AREA_CODE],
                                    PHONE_HOME_CITY_CODE,
                                    map);
-    } else if (parsed_phone_fields_[FIELD_COUNTRY_CODE] != NULL) {
+    } else if (parsed_phone_fields_[FIELD_COUNTRY_CODE]) {
       // Only if we can find country code without city code, it means the phone
       // number include city code.
       field_number_type = PHONE_HOME_CITY_AND_NUMBER;
@@ -226,7 +226,7 @@ bool PhoneField::ClassifyField(ServerFieldTypeMap* map) const {
                                  map);
     // We tag the suffix as PHONE_HOME_NUMBER, then when filling the form
     // we fill only the suffix depending on the size of the input field.
-    if (parsed_phone_fields_[FIELD_SUFFIX] != NULL) {
+    if (parsed_phone_fields_[FIELD_SUFFIX]) {
       ok = ok && AddClassification(parsed_phone_fields_[FIELD_SUFFIX],
                                    PHONE_HOME_NUMBER,
                                    map);
@@ -245,31 +245,41 @@ PhoneField::PhoneField() {
 }
 
 // static
-base::string16 PhoneField::GetRegExp(RegexType regex_id) {
+std::string PhoneField::GetRegExp(RegexType regex_id) {
   switch (regex_id) {
     case REGEX_COUNTRY:
-      return base::UTF8ToUTF16(kCountryCodeRe);
+      return kCountryCodeRe;
     case REGEX_AREA:
       return GetAreaRegex();
     case REGEX_AREA_NOTEXT:
-      return base::UTF8ToUTF16(kAreaCodeNotextRe);
+      return kAreaCodeNotextRe;
     case REGEX_PHONE:
-      return base::UTF8ToUTF16(kPhoneRe);
+      return kPhoneRe;
     case REGEX_PREFIX_SEPARATOR:
-      return base::UTF8ToUTF16(kPhonePrefixSeparatorRe);
+      return kPhonePrefixSeparatorRe;
     case REGEX_PREFIX:
-      return base::UTF8ToUTF16(kPhonePrefixRe);
+      return kPhonePrefixRe;
     case REGEX_SUFFIX_SEPARATOR:
-      return base::UTF8ToUTF16(kPhoneSuffixSeparatorRe);
+      return kPhoneSuffixSeparatorRe;
     case REGEX_SUFFIX:
-      return base::UTF8ToUTF16(kPhoneSuffixRe);
+      return kPhoneSuffixRe;
     case REGEX_EXTENSION:
-      return base::UTF8ToUTF16(kPhoneExtensionRe);
+      return kPhoneExtensionRe;
     default:
       NOTREACHED();
       break;
   }
-  return base::string16();
+  return std::string();
+}
+
+// static
+bool PhoneField::ParsePhoneField(AutofillScanner* scanner,
+                                 const std::string& regex,
+                                 AutofillField** field) {
+  return ParseFieldSpecifics(scanner,
+                             base::UTF8ToUTF16(regex),
+                             MATCH_DEFAULT | MATCH_TELEPHONE,
+                             field);
 }
 
 }  // namespace autofill

components/autofill/core/browser/phone_field.h

@@ -79,8 +79,13 @@ class PhoneField : public FormField {
 
   PhoneField();
 
-  // Returns the regular expression string correspoding to |regex_id|
-  static base::string16 GetRegExp(RegexType regex_id);
+  // Returns the regular expression string corresponding to |regex_id|
+  static std::string GetRegExp(RegexType regex_id);
+
+  // Convenient wrapper for ParseFieldSpecifics().
+  static bool ParsePhoneField(AutofillScanner* scanner,
+                              const std::string& regex,
+                              AutofillField** field);
 
   // FIELD_PHONE is always present; holds suffix if prefix is present.
   // The rest could be NULL.

components/autofill/core/browser/phone_field_unittest.cc

@@ -15,21 +15,43 @@ using base::ASCIIToUTF16;
 
 namespace autofill {
 
+namespace {
+
+const char* const kFieldTypes[] = {
+  "text",
+  "tel",
+};
+
+}  // namespace
+
 class PhoneFieldTest : public testing::Test {
  public:
   PhoneFieldTest() {}
 
  protected:
-  ScopedVector<AutofillField> list_;
-  scoped_ptr<PhoneField> field_;
-  ServerFieldTypeMap field_type_map_;
-
   // Downcast for tests.
   static scoped_ptr<PhoneField> Parse(AutofillScanner* scanner) {
     scoped_ptr<FormField> field = PhoneField::Parse(scanner);
     return make_scoped_ptr(static_cast<PhoneField*>(field.release()));
   }
 
+  void Clear() {
+    list_.clear();
+    field_.reset();
+    field_type_map_.clear();
+  }
+
+  void CheckField(const std::string& name,
+                  ServerFieldType expected_type) const {
+    auto it = field_type_map_.find(ASCIIToUTF16(name));
+    ASSERT_TRUE(it != field_type_map_.end()) << name;
+    EXPECT_EQ(expected_type, it->second) << name;
+  }
+
+  ScopedVector<AutofillField> list_;
+  scoped_ptr<PhoneField> field_;
+  ServerFieldTypeMap field_type_map_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(PhoneFieldTest);
 };
@@ -49,8 +71,11 @@ TEST_F(PhoneFieldTest, NonParse) {
 
 TEST_F(PhoneFieldTest, ParseOneLinePhone) {
   FormFieldData field;
-  field.form_control_type = "text";
 
+  for (size_t i = 0; i < arraysize(kFieldTypes); ++i) {
+    Clear();
+
+    field.form_control_type = kFieldTypes[i];
     field.label = ASCIIToUTF16("Phone");
     field.name = ASCIIToUTF16("phone");
     list_.push_back(new AutofillField(field, ASCIIToUTF16("phone1")));
@@ -59,15 +84,17 @@ TEST_F(PhoneFieldTest, ParseOneLinePhone) {
     field_ = Parse(&scanner);
     ASSERT_NE(nullptr, field_.get());
     ASSERT_TRUE(field_->ClassifyField(&field_type_map_));
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("phone1")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_WHOLE_NUMBER, field_type_map_[ASCIIToUTF16("phone1")]);
+    CheckField("phone1", PHONE_HOME_WHOLE_NUMBER);
+  }
 }
 
 TEST_F(PhoneFieldTest, ParseTwoLinePhone) {
   FormFieldData field;
-  field.form_control_type = "text";
 
+  for (size_t i = 0; i < arraysize(kFieldTypes); ++i) {
+    Clear();
+
+    field.form_control_type = kFieldTypes[i];
     field.label = ASCIIToUTF16("Area Code");
     field.name = ASCIIToUTF16("area code");
     list_.push_back(new AutofillField(field, ASCIIToUTF16("areacode1")));
@@ -80,12 +107,9 @@ TEST_F(PhoneFieldTest, ParseTwoLinePhone) {
     field_ = Parse(&scanner);
     ASSERT_NE(nullptr, field_.get());
     ASSERT_TRUE(field_->ClassifyField(&field_type_map_));
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("areacode1")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_CITY_CODE, field_type_map_[ASCIIToUTF16("areacode1")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("phone2")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("phone2")]);
+    CheckField("areacode1", PHONE_HOME_CITY_CODE);
+    CheckField("phone2", PHONE_HOME_NUMBER);
+  }
 }
 
 TEST_F(PhoneFieldTest, ThreePartPhoneNumber) {
@@ -95,8 +119,11 @@ TEST_F(PhoneFieldTest, ThreePartPhoneNumber) {
   // size: <prefix> is no bigger than 3 characters, and <suffix> is no bigger
   // than 4.
   FormFieldData field;
-  field.form_control_type = "text";
 
+  for (size_t i = 0; i < arraysize(kFieldTypes); ++i) {
+    Clear();
+
+    field.form_control_type = kFieldTypes[i];
     field.label = ASCIIToUTF16("Phone:");
     field.name = ASCIIToUTF16("dayphone1");
     field.max_length = 0;
@@ -121,17 +148,11 @@ TEST_F(PhoneFieldTest, ThreePartPhoneNumber) {
     field_ = Parse(&scanner);
     ASSERT_NE(nullptr, field_.get());
     ASSERT_TRUE(field_->ClassifyField(&field_type_map_));
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("areacode1")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_CITY_CODE, field_type_map_[ASCIIToUTF16("areacode1")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("prefix2")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("prefix2")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("suffix3")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("suffix3")]);
-  EXPECT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("ext4")) == field_type_map_.end());
+    CheckField("areacode1", PHONE_HOME_CITY_CODE);
+    CheckField("prefix2", PHONE_HOME_NUMBER);
+    CheckField("suffix3", PHONE_HOME_NUMBER);
+    EXPECT_FALSE(ContainsKey(field_type_map_, ASCIIToUTF16("ext4")));
+  }
 }
 
 // This scenario of explicitly labeled "prefix" and "suffix" phone numbers
@@ -139,8 +160,11 @@ TEST_F(PhoneFieldTest, ThreePartPhoneNumber) {
 // https://www.wrapables.com/jsp/Signup.jsp.
 TEST_F(PhoneFieldTest, ThreePartPhoneNumberPrefixSuffix) {
   FormFieldData field;
-  field.form_control_type = "text";
 
+  for (size_t i = 0; i < arraysize(kFieldTypes); ++i) {
+    Clear();
+
+    field.form_control_type = kFieldTypes[i];
     field.label = ASCIIToUTF16("Phone:");
     field.name = ASCIIToUTF16("area");
     list_.push_back(new AutofillField(field, ASCIIToUTF16("areacode1")));
@@ -157,21 +181,19 @@ TEST_F(PhoneFieldTest, ThreePartPhoneNumberPrefixSuffix) {
     field_ = Parse(&scanner);
     ASSERT_NE(nullptr, field_.get());
     ASSERT_TRUE(field_->ClassifyField(&field_type_map_));
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("areacode1")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_CITY_CODE, field_type_map_[ASCIIToUTF16("areacode1")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("prefix2")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("prefix2")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("suffix3")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("suffix3")]);
+    CheckField("areacode1", PHONE_HOME_CITY_CODE);
+    CheckField("prefix2", PHONE_HOME_NUMBER);
+    CheckField("suffix3", PHONE_HOME_NUMBER);
+  }
 }
 
 TEST_F(PhoneFieldTest, ThreePartPhoneNumberPrefixSuffix2) {
   FormFieldData field;
-  field.form_control_type = "text";
 
+  for (size_t i = 0; i < arraysize(kFieldTypes); ++i) {
+    Clear();
+
+    field.form_control_type = kFieldTypes[i];
     field.label = ASCIIToUTF16("(");
     field.name = ASCIIToUTF16("phone1");
     field.max_length = 3;
@@ -191,23 +213,21 @@ TEST_F(PhoneFieldTest, ThreePartPhoneNumberPrefixSuffix2) {
     field_ = Parse(&scanner);
     ASSERT_NE(nullptr, field_.get());
     ASSERT_TRUE(field_->ClassifyField(&field_type_map_));
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("phone1")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_CITY_CODE, field_type_map_[ASCIIToUTF16("phone1")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("phone2")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("phone2")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("phone3")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_NUMBER, field_type_map_[ASCIIToUTF16("phone3")]);
+    CheckField("phone1", PHONE_HOME_CITY_CODE);
+    CheckField("phone2", PHONE_HOME_NUMBER);
+    CheckField("phone3", PHONE_HOME_NUMBER);
+  }
 }
 
 TEST_F(PhoneFieldTest, CountryAndCityAndPhoneNumber) {
   // Phone in format <country code>:3 - <city and number>:10
   // The |maxlength| is considered, otherwise it's too broad.
   FormFieldData field;
-  field.form_control_type = "text";
 
+  for (size_t i = 0; i < arraysize(kFieldTypes); ++i) {
+    Clear();
+
+    field.form_control_type = kFieldTypes[i];
     field.label = ASCIIToUTF16("Phone Number");
     field.name = ASCIIToUTF16("CountryCode");
     field.max_length = 3;
@@ -222,12 +242,9 @@ TEST_F(PhoneFieldTest, CountryAndCityAndPhoneNumber) {
     field_ = Parse(&scanner);
     ASSERT_NE(nullptr, field_.get());
     ASSERT_TRUE(field_->ClassifyField(&field_type_map_));
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("country")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_COUNTRY_CODE, field_type_map_[ASCIIToUTF16("country")]);
-  ASSERT_TRUE(
-      field_type_map_.find(ASCIIToUTF16("phone")) != field_type_map_.end());
-  EXPECT_EQ(PHONE_HOME_CITY_AND_NUMBER, field_type_map_[ASCIIToUTF16("phone")]);
+    CheckField("country", PHONE_HOME_COUNTRY_CODE);
+    CheckField("phone", PHONE_HOME_CITY_AND_NUMBER);
+  }
 }
 
 }  // namespace autofill