Changed parsing code for the phonenumbers fields to incorporate different com...

chrome/browser/autofill/phone_field.h

 // Copyright (c) 2009 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.
 
 #ifndef CHROME_BROWSER_AUTOFILL_PHONE_FIELD_H_
 #define CHROME_BROWSER_AUTOFILL_PHONE_FIELD_H_
 #pragma once
 
 #include <vector>
 
@@ skipping 13 matching lines @@
   static PhoneField* Parse(std::vector<AutoFillField*>::const_iterator* iter,
                            bool is_ecml);
   static PhoneField* ParseECML(
       std::vector<AutoFillField*>::const_iterator* iter);
 
   virtual bool GetFieldInfo(FieldTypeMap* field_type_map) const;
 
  private:
   PhoneField();
 
   enum PHONE_TYPE {

[dhollowa, 2011/02/16 00:25:37]

Types should be mixed case, not all-caps.  So: s/PHONE_TYPE/PhoneType/.

[GeorgeY, 2011/02/16 20:53:34]

Done.

     PHONE_TYPE_FIRST = 0,
     HOME_PHONE = PHONE_TYPE_FIRST,
     FAX_PHONE,
 
     // Must be last.
     PHONE_TYPE_MAX,
   };
 
   // Some field names are different for phone and fax.
+  string16 GetCountryRegex() const;
+  // This string includes all area code separators, including NoText.
   string16 GetAreaRegex() const;
+  // Separator of the area code in the case fields are formatted without
+  // any text indicating what fields are (e.g. field1 "(" field2 ")" field3 "-"
+  // field4 means Country Code, Area Code, Preffix, Suffix)

[Ilya Sherman, 2011/02/16 09:33:22]

nit: "Prefix" (one 'f')

[GeorgeY, 2011/02/16 20:53:34]

Done.

+  string16 GetAreaNoTextRegex() const;
   string16 GetPhoneRegex() const;
+  string16 GetPrefixSeparatorRegex() const;
   string16 GetPrefixRegex() const;
+  string16 GetSuffixSeparatorRegex() const;
   string16 GetSuffixRegex() const;
   string16 GetExtensionRegex() const;
 
+  // This is for easy description of the possible parsing paths of the phone
+  // fields.
+  enum REGEX_MATCH {

[dhollowa, 2011/02/16 00:25:37]

Types should be mixed case, not all-caps.  So: s/REGEX_MATCH/RegexMatch/ or
maybe "RegexType"?

[GeorgeY, 2011/02/16 20:53:34]

Done. (RegexType)

+    REGEX_COUNTRY,
+    REGEX_AREA,
+    REGEX_AREA_NOTEXT,
+    REGEX_PHONE,
+    REGEX_PREFIX_SEPARATOR,
+    REGEX_PREFIX,
+    REGEX_SUFFIX_SEPARATOR,
+    REGEX_SUFFIX,
+    REGEX_EXTENSION,
+
+    // Separates regexps in grammar.
+    REGEX_SEPARATOR,
+  };
+
+  string16 GetRegExp(REGEX_MATCH regex_id) const;
+
   // |field| - field to fill up on successful parsing.
   // |iter| - in/out. Form field iterator, points to the first field that is
   //   attempted to be parsed. If parsing successful, points to the first field
   //   after parsed fields.
   // |regular_phone| - true if the parsed phone is a HOME phone, false
   //   otherwise.
   static bool ParseInternal(PhoneField* field,
                             std::vector<AutoFillField*>::const_iterator* iter,
                             bool regular_phone);
 
   void SetPhoneType(PHONE_TYPE phone_type);
 
   // Field types are different as well, so we create a temporary phone number,
   // to get relevant field types.
   scoped_ptr<PhoneNumber> number_;
   PHONE_TYPE phone_type_;
 
-  // Always present; holds suffix if prefix is present.
-  AutoFillField* phone_;
 
-  AutoFillField* area_code_;  // optional
-  AutoFillField* prefix_;     // optional
-  AutoFillField* extension_;  // optional
+  // Parsed fields.
+  enum {

[dhollowa, 2011/02/16 00:25:37]

Let's add a type to this enum, "PhonePart" say (?), and then use that this type
definition on line 112 to type the |field_id|.  And maybe change that from
|field_id| to |phone_part|.

[GeorgeY, 2011/02/16 20:53:34]

Done.

+    FIELD_COUNTRY_CODE,

[dhollowa, 2011/02/16 00:25:37]

I notice the |phone_field_grammars_| member is initialized with 0 for the
PhoneField::REGEX_SEPARATOR.  It would be preferable to have an explicit
FIELD_NULL = -1, at the start to avoid value overloading of 0 ==
FIELD_COUNTRY_CODE.

[GeorgeY, 2011/02/16 20:53:34]

Done.

+    FIELD_AREA_CODE,
+    FIELD_PHONE,
+    FIELD_SUFFIX,
+    FIELD_EXTENSION,
+
+    FIELD_MAX,
+  };
+
+  // FIELD_PHONE is always present; holds suffix if prefix is present.
+  // The rest could be NULL.
+  AutoFillField* parsed_phone_fields_[FIELD_MAX];
+
+  static struct Parser {
+    REGEX_MATCH regex;  // Field matching reg-ex.
+    size_t field_id;    // Index of the field.

[dhollowa, 2011/02/16 00:25:37]

As mentioned above, size_t -> PhonePart, and field_id -> phone_part.

[GeorgeY, 2011/02/16 20:53:34]

Done.

+    int max_size;       // Max size of the field to match. 0 means any.
+  } phone_field_grammars_[];
 
   DISALLOW_COPY_AND_ASSIGN(PhoneField);
 };
 
 #endif  // CHROME_BROWSER_AUTOFILL_PHONE_FIELD_H_