Autofill: Better recognize credit card fields.

components/autofill/core/browser/credit_card_field.cc

Index: components/autofill/core/browser/credit_card_field.cc
diff --git a/components/autofill/core/browser/credit_card_field.cc b/components/autofill/core/browser/credit_card_field.cc
index ba9c001022dc69ced0d5207c70f5d64b26d4550b..2129751245c14d986346ddccd34fd0edd93c6d0a 100644
--- a/components/autofill/core/browser/credit_card_field.cc
+++ b/components/autofill/core/browser/credit_card_field.cc
@@ -12,6 +12,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autofill_field.h"
 #include "components/autofill/core/browser/autofill_regex_constants.h"
+#include "components/autofill/core/browser/autofill_regexes.h"
 #include "components/autofill/core/browser/autofill_scanner.h"
 #include "components/autofill/core/browser/field_types.h"
 
@@ -22,6 +23,14 @@ namespace autofill {
 static const size_t kMaxValidCardNumberSize = 19;
 
 // static
+const int CreditCardField::kMatchNumAndTel =
+    MATCH_DEFAULT | MATCH_NUMBER | MATCH_TELEPHONE;
+
+// static
+const int CreditCardField::kMatchNumTelAndPass =
+    kMatchNumAndTel | MATCH_PASSWORD;
+
+// static
 scoped_ptr<FormField> CreditCardField::Parse(AutofillScanner* scanner) {
   if (scanner->IsEnd())
     return nullptr;
@@ -31,13 +40,15 @@ scoped_ptr<FormField> CreditCardField::Parse(AutofillScanner* scanner) {
 
   // Credit card fields can appear in many different orders.
   // We loop until no more credit card related fields are found, see |break| at
-  // bottom of the loop.
+  // the bottom of the loop.
   for (int fields = 0; !scanner->IsEnd(); ++fields) {
     // Ignore gift card fields.
     if (ParseField(scanner, base::UTF8ToUTF16(kGiftCardRe), nullptr))
       break;
 
-    if (!credit_card_field->cardholder_) {
+    if (!credit_card_field->cardholder_ &&
+        !LikelyCardNumberField(scanner) &&

[Evan Stade, 2015/03/13 20:57:13]

Why are you checking !LikelyCardNumberField in several places, as opposed to,
say, moving the card number matcher earlier in the loop?

[Lei Zhang, 2015/03/13 23:11:00]

Changing the order may creates more of the same problem. Instead of incorrectly
recognizing a name as a number, we may end up incorrectly recgonzing a number as
a name. The order we have seem to work well for most cases, so I don't want to
change it. I just want to make it more accurate. The !LikelyFoo() calls act as
filters to help decrease false positives.

+        !MayBeCardCVCField(scanner)) {
       if (ParseField(scanner,
                      base::UTF8ToUTF16(kNameOnCardRe),
                      &credit_card_field->cardholder_)) {
@@ -60,11 +71,11 @@ scoped_ptr<FormField> CreditCardField::Parse(AutofillScanner* scanner) {
     }
 
     // Check for a credit card type (Visa, MasterCard, etc.) field.
-    if (!credit_card_field->type_ &&
-        ParseFieldSpecifics(scanner,
-                            base::UTF8ToUTF16(kCardTypeRe),
-                            MATCH_DEFAULT | MATCH_SELECT,
-                            &credit_card_field->type_)) {
+    // This used to match text fields, and had a regex for the field name/label,
+    // but LikelyCardType() works as well and produces fewer incorrect matches.

[Evan Stade, 2015/03/13 20:57:13]

this extra documentation is perhaps relevant to this code review but I'd not
leave it in the code

[Lei Zhang, 2015/03/13 23:11:00]

Removed.

+    if (!credit_card_field->type_ && LikelyCardType(scanner)) {
+      credit_card_field->type_ = scanner->Cursor();
+      scanner->Advance();
       continue;
     }
 
@@ -74,11 +85,11 @@ scoped_ptr<FormField> CreditCardField::Parse(AutofillScanner* scanner) {
     // identification number", and others listed in the regex pattern used
     // below.
     // Note: Some sites use type="tel" or type="number" for numerical inputs.
-    const int kMatchNumAndTel = MATCH_DEFAULT | MATCH_NUMBER | MATCH_TELEPHONE;
     if (!credit_card_field->verification_ &&
+        !LikelyCardNumberField(scanner) &&
         ParseFieldSpecifics(scanner,
                             base::UTF8ToUTF16(kCardCvcRe),
-                            kMatchNumAndTel | MATCH_PASSWORD,
+                            kMatchNumTelAndPass,
                             &credit_card_field->verification_)) {
       continue;
     }
@@ -121,14 +132,6 @@ scoped_ptr<FormField> CreditCardField::Parse(AutofillScanner* scanner) {
       return nullptr;
     }
 
-    // Some pages (e.g. ExpediaBilling.html) have a "card description"

[Lei Zhang, 2015/03/13 08:15:11]

With the other changes, this reached the start of a new CC block and start
eating those fields. If we ever find another example where a CC block gets
broken in two, we can try putting this back, but with something more restrictive
than just ^card.

I wonder what happened to ExpediaBilling.html and friends.

-    // field; we parse this field but ignore it.
-    // We also ignore any other fields within a credit card block that
-    // start with "card", under the assumption that they are related to
-    // the credit card section being processed but are uninteresting to us.
-    if (ParseField(scanner, base::UTF8ToUTF16(kCardIgnoredRe), nullptr))
-      continue;
-
     break;
   }
 
@@ -157,6 +160,57 @@ scoped_ptr<FormField> CreditCardField::Parse(AutofillScanner* scanner) {
   return nullptr;
 }
 
+// static
+bool CreditCardField::LikelyCardNumberField(AutofillScanner* scanner) {
+  if (scanner->IsEnd())
+    return false;
+
+  // Look for a form of the right type with a 16 to 19 digit maxlength.

[Evan Stade, 2015/03/13 20:57:13]

the minimum card has less than 16 digits

[Lei Zhang, 2015/03/13 23:11:00]

Yes, but if a website made it less than 16, then they can't accept a
VISA/Mastercard CC number.

+  AutofillField* field = scanner->Cursor();
+  return (MatchesFormControlType(field->form_control_type, kMatchNumAndTel) &&
+          field->max_length >= 16 &&
+          field->max_length <= kMaxValidCardNumberSize);
+}
+
+// static
+bool CreditCardField::LikelyCardType(AutofillScanner* scanner) {
+  if (scanner->IsEnd())
+    return false;
+
+  AutofillField* field = scanner->Cursor();
+  if (MatchesFormControlType(field->form_control_type, MATCH_SELECT)) {

[Evan Stade, 2015/03/13 20:57:13]

I feel like this code should go somewhere we can reuse it for address pieces

Also you should reuse/share with the filling code, i.e.
FillCreditCardTypeSelectControl

[Lei Zhang, 2015/03/13 23:11:00]

Done.

+    // VISA and Mastercard are oftentimes branded as is in other countries, so
+    // translations are probably not needed.
+    const char* const kCardBrands[] = {
+      "visa",
+      "mastercard",
+    };
+    for (const char* brand : kCardBrands) {
+      for (const auto& value : field->option_values) {
+        if (MatchesPattern(value, base::ASCIIToUTF16(brand)))
+          return true;
+      }
+      for (const auto& content : field->option_contents) {
+        if (MatchesPattern(content, base::ASCIIToUTF16(brand)))
+          return true;
+      }
+    }
+  }
+  return false;
+}
+
+// static
+bool CreditCardField::MayBeCardCVCField(AutofillScanner* scanner) {
+  if (scanner->IsEnd())
+    return false;
+
+  // Look for a form of the right type with a 3 or 4 digit maxlength.
+  AutofillField* field = scanner->Cursor();
+  return (MatchesFormControlType(field->form_control_type,
+                                 kMatchNumTelAndPass) &&
+          (field->max_length == 3 || field->max_length == 4));
+}
+
 CreditCardField::CreditCardField()
     : cardholder_(nullptr),
       cardholder_last_(nullptr),