Autofill refactor form_field.h/cc.

Autofill refactor form_field.h/cc.

1. Moves FormField methods into proper public/protected/private visibility.
2. Eliminates redundant "Parse" methods.
3. Introduces |MatchType| bit field to simplify variant matching.
4. Consolidates |Match| method.
5. Renames |Add| and |GetFieldInfo| to |AddClassification| and |ClassifyField| respectively.
6. Move ECML logic into separate file.

BUG=none
TEST=Unit tests in autofill/*_field_unittest.cc.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86016

[Ilya Sherman, 2011-05-19 05:44:16]

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.cc (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.cc:304: &address_field->zip4_);
nit: indentation (probably other places with ParseText->ParseField as well)

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.h:31: bool IsFullAddress();
This is used only in the unit tests -- should we perhaps get rid of it?

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.cc (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.cc:102: bool ok =
form_field->ClassifyField(map);
This line parses really oddly, since form_field can actually represent a
collection of fields, and ClassifyField() actually adds classifications for
these multiple fields.  I'm not sure how to make this much cleaner though :/

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:27: // The association is stored into
|map|.  Each field has a |unique_name| that
nit: I don't see "unique_name" used as a variable below, so I would omit the
pipes.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:51: // is non-NULL and the pattern matches,
the matched field is returned.
nit: When does this return true/false?  We should probably mention that in the
comment.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:52: static bool
ParseFieldSpecifics(AutofillScanner* scanner,
I don't really get much out of the term "Specifics" -- other than that this is
mostly like |ParseField|, but with something to be wary of.  I don't have any
better suggestions right now, but perhaps you had some alternate names in mind?

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:73: virtual bool
ClassifyField(FieldTypeMap* map) const = 0;
nit: It seems odd to me to give a virtual method |private| visibility, even if
it's pure-virtual.  If subclasses are expected to implement this method, I'd
kinda expect to see it in the |protected| section of the class.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
File chrome/browser/autofill/name_field.cc (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:32: }
nit: Please move this to be below Parse()

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:38: bool username = ParseField(
nit: Perhaps "is_username"?

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:49: &field))
nit: indentation

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:69: &v->first_name_) &&
nit: indentation

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
File chrome/browser/autofill/name_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.h:45: class FullNameField : public NameField
{
Should this class and also FirstLastNameField be moved into an anonymous
namespace in the implementation file?

[dhollowa, 2011-05-19 17:53:08]

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.cc (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.cc:304: &address_field->zip4_);
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: indentation (probably other places with ParseText->ParseField as well)

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.h:31: bool IsFullAddress();
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> This is used only in the unit tests -- should we perhaps get rid of it?

Done.  Yes, this wasn't providing any meaningful coverage in the tests either. 
So I've removed it.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.cc (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.cc:102: bool ok =
form_field->ClassifyField(map);
Yes, it requires the reader to understand that a "field" can represent more than
one entry in the map potentially.  But this is a systemic issue, encoded in the
overall structure of the FormField and its subclasses.  It's not specific to the
code here.  I'll add a comment to this effect, for the casual reader.

On 2011/05/19 05:44:16, Ilya Sherman wrote:
> This line parses really oddly, since form_field can actually represent a
> collection of fields, and ClassifyField() actually adds classifications for
> these multiple fields.  I'm not sure how to make this much cleaner though :/

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:27: // The association is stored into
|map|.  Each field has a |unique_name| that
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: I don't see "unique_name" used as a variable below, so I would omit the
> pipes.

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:51: // is non-NULL and the pattern matches,
the matched field is returned.
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: When does this return true/false?  We should probably mention that in the
> comment.

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:52: static bool
ParseFieldSpecifics(AutofillScanner* scanner,
I had a few: ParseFieldVariant, ParseFieldWithMatch... But ParseFieldSpecifics
seems most accurate and descriptive to me.

On 2011/05/19 05:44:16, Ilya Sherman wrote:
> I don't really get much out of the term "Specifics" -- other than that this is
> mostly like |ParseField|, but with something to be wary of.  I don't have any
> better suggestions right now, but perhaps you had some alternate names in
mind?

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:73: virtual bool
ClassifyField(FieldTypeMap* map) const = 0;
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: It seems odd to me to give a virtual method |private| visibility, even if
> it's pure-virtual.  If subclasses are expected to implement this method, I'd
> kinda expect to see it in the |protected| section of the class.

I was following a principle of least visibility, where the visibility is
determined by the needs of the caller, not the subclass.  And since the only
caller is |FormField| then "private" is most correct.

However, I see your point.  It does seem a little strange from the derivation
point of view.  I've pulled up to "protected".

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
File chrome/browser/autofill/name_field.cc (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:32: }
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: Please move this to be below Parse()

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:38: bool username = ParseField(
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: Perhaps "is_username"?

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:49: &field))
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.cc:69: &v->first_name_) &&
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
File chrome/browser/autofill/name_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/name_fi...
chrome/browser/autofill/name_field.h:45: class FullNameField : public NameField
{
On 2011/05/19 05:44:16, Ilya Sherman wrote:
> Should this class and also FirstLastNameField be moved into an anonymous
> namespace in the implementation file?

Yes.  Done.

[Ilya Sherman, 2011-05-19 20:24:09]

LGTM, thanks

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:52: static bool
ParseFieldSpecifics(AutofillScanner* scanner,
Ok.  "Specifics" still doesn't evoke "label and/or name" for me; but as I don't
have any better suggestions, let's keep this for now.

On 2011/05/19 17:53:08, dhollowa wrote:
> I had a few: ParseFieldVariant, ParseFieldWithMatch... But ParseFieldSpecifics
> seems most accurate and descriptive to me.
> 
> On 2011/05/19 05:44:16, Ilya Sherman wrote:
> > I don't really get much out of the term "Specifics" -- other than that this
is
> > mostly like |ParseField|, but with something to be wary of.  I don't have
any
> > better suggestions right now, but perhaps you had some alternate names in
> mind?
>

[honten.org, 2011-05-20 02:50:19]

Are you done for refactoring?

Or do you need more?

On 2011/05/19 20:24:09, Ilya Sherman wrote:
> LGTM, thanks
> 
>
http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
> File chrome/browser/autofill/form_field.h (right):
> 
>
http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
> chrome/browser/autofill/form_field.h:52: static bool
> ParseFieldSpecifics(AutofillScanner* scanner,
> Ok.  "Specifics" still doesn't evoke "label and/or name" for me; but as I
don't
> have any better suggestions, let's keep this for now.
> 
> On 2011/05/19 17:53:08, dhollowa wrote:
> > I had a few: ParseFieldVariant, ParseFieldWithMatch... But
ParseFieldSpecifics
> > seems most accurate and descriptive to me.
> > 
> > On 2011/05/19 05:44:16, Ilya Sherman wrote:
> > > I don't really get much out of the term "Specifics" -- other than that
this
> is
> > > mostly like |ParseField|, but with something to be wary of.  I don't have
> any
> > > better suggestions right now, but perhaps you had some alternate names in
> > mind?
> >

[dhollowa, 2011-05-20 15:55:15]

Still some more coming, yes.

On 2011/05/20 02:50:19, honten wrote:
> Are you done for refactoring?
> 
> Or do you need more?
> 
> On 2011/05/19 20:24:09, Ilya Sherman wrote:
> > LGTM, thanks
> > 
> >
>
http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
> > File chrome/browser/autofill/form_field.h (right):
> > 
> >
>
http://codereview.chromium.org/7043027/diff/1/chrome/browser/autofill/form_fi...
> > chrome/browser/autofill/form_field.h:52: static bool
> > ParseFieldSpecifics(AutofillScanner* scanner,
> > Ok.  "Specifics" still doesn't evoke "label and/or name" for me; but as I
> don't
> > have any better suggestions, let's keep this for now.
> > 
> > On 2011/05/19 17:53:08, dhollowa wrote:
> > > I had a few: ParseFieldVariant, ParseFieldWithMatch... But
> ParseFieldSpecifics
> > > seems most accurate and descriptive to me.
> > > 
> > > On 2011/05/19 05:44:16, Ilya Sherman wrote:
> > > > I don't really get much out of the term "Specifics" -- other than that
> this
> > is
> > > > mostly like |ParseField|, but with something to be wary of.  I don't
have
> > any
> > > > better suggestions right now, but perhaps you had some alternate names
in
> > > mind?
> > >