[Autofill] Add a separate enumeration for HTML field type hints.

[Autofill] Add a separate enumeration for HTML field type hints.

The Autofill code currently uses a single enum AutofillFieldType to describe all
field types. However, several of the field type hints provided in the HTML
autocomplete specification do not map nicely onto the native (server-supported)
field types.

This CL adds two new enumerations corresponding to the exact types specified in
the HTML5 spec, and updates the relevant code to map from these types to the
native types that Autofill understands.

Implementing support for properly filling 'country' and 'street-address' types
will come in subsequent CLs (which should be much smaller, whew!).

BUG=254682, 261978
R=estade@chromium.org
TBR=zea@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216149

[Ilya Sherman, 2013-08-03 07:14:19]

(Note: There are still a handful of TODOs left in this CL.  I am planning to
punt those to separate, follow-up CLs, as they don't affect the core of what
this CL is doing.)

[Evan Stade, 2013-08-05 18:47:24]

mostly LG, just worried about subtleties.

https://codereview.chromium.org/22040002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/22040002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/data_model_wrapper.cc:126: HtmlFieldType html_type =
field->html_type();
nit: declare as late as possible, not at top of function.

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/address.cc (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/address.cc:47: // TODO(isherman): Is
GetEquivalentNativeType even necessary?
I'm assuming you mean you could just switch on |type|?

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/autofill_profile.cc:37:
AutofillType(type).GetEquivalentNativeType();
this fn seems a little bit hoop-jumpy. Would it be the same to do:

AutofillType autofill_type = AutofillType(type);
if (type.group() == NAME)
  return NAME_FULL;

return autofill_type.GetEquivalentNativeType();

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/autofill_type.cc (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/autofill_type.cc:27: html_mode_(mode) {
nit: {} on one line

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/autofill_type.h (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/autofill_type.h:34: NativeFieldType
GetEquivalentNativeType() const;
while "equivalent" was probably never the easiest to understand name, this is
now even more confusing because it sounds like it's just mapping from html_type_
to native_type_. However it's also possible to spit out a native type that
doesn't match native_type_. GetStorableNativeType? GetChromeAutofillType?

I'm also a bit worried that it will be easy to use GetEquivalent or
GetApproximate when the other is more appropriate, since in most cases it does
the same thing. Is there a way you can make that mixup less likely?

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/autofill_type.h:41: // equivalent storable
type.
it sounds like this guarantees you won't get UNKOWN_TYPE out of this fn, but I
don't think that's correct.

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/field_types.h:13: #include
"base/strings/string16.h"
?

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/field_types.h:24: enum NativeFieldType {
can we call this something else? AutofillServerFieldType, or if that's too long,
AutofillServerType, ServerNativeType, or something. Since it seems that chrome
and the server don't agree on the set of types, it's not obvious what "native"
refers to (you have to know it means "native to the server, not chrome or
html"). I guess my objection is that everything is "native" to something.

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/form_structure.cc:193: HtmlFieldType
FieldTypeFromAutocompleteType(
maybe you should rename this to FieldTypeFromAutocompleteAttribute or
FieldTypeFromAutocompleteValue (because "AutocompleteType" would be a logical
alternative name for "HtmlFieldType")

[Ilya Sherman, 2013-08-06 05:05:39]

https://chromiumcodereview.appspot.com/22040002/diff/5001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/data_model_wrapper.cc:126: HtmlFieldType html_type =
field->html_type();
On 2013/08/05 18:47:24, Evan Stade wrote:
> nit: declare as late as possible, not at top of function.

Hmm, I'm not sure where you'd prefer to see this declared.  It needs to come
before line 133, since the goal is to cache the type prior to overwriting it;
and declaring the variable just within the if-stmt doesn't work, as the value
needs to be read at line 138.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/address.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/address.cc:47: // TODO(isherman): Is
GetEquivalentNativeType even necessary?
On 2013/08/05 18:47:24, Evan Stade wrote:
> I'm assuming you mean you could just switch on |type|?

Yeah.  This requires fixing up a couple of call sites to GetRawInfo() first,
though, which I'd rather do as part of a separate CL.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/autofill_profile.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/autofill_profile.cc:37:
AutofillType(type).GetEquivalentNativeType();
On 2013/08/05 18:47:24, Evan Stade wrote:
> this fn seems a little bit hoop-jumpy. Would it be the same to do:
> 
> AutofillType autofill_type = AutofillType(type);
> if (type.group() == NAME)
>   return NAME_FULL;
> 
> return autofill_type.GetEquivalentNativeType();

It's not quite the same unless we also check for NAME_BILLING in the if-stmt. 
Let me know whether you prefer that (as done in the newer patch set) or the way
that it was here.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/autofill_type.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/autofill_type.cc:27: html_mode_(mode) {
On 2013/08/05 18:47:24, Evan Stade wrote:
> nit: {} on one line

Done.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/autofill_type.h (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/autofill_type.h:34: NativeFieldType
GetEquivalentNativeType() const;
On 2013/08/05 18:47:24, Evan Stade wrote:
> while "equivalent" was probably never the easiest to understand name, this is
> now even more confusing because it sounds like it's just mapping from
html_type_
> to native_type_. However it's also possible to spit out a native type that
> doesn't match native_type_. GetStorableNativeType? GetChromeAutofillType?

Done.

> I'm also a bit worried that it will be easy to use GetEquivalent or
> GetApproximate when the other is more appropriate, since in most cases it does
> the same thing. Is there a way you can make that mixup less likely?

Does the added explanation in the comment for GetApproximateStorableType() help?

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/autofill_type.h:41: // equivalent storable
type.
On 2013/08/05 18:47:24, Evan Stade wrote:
> it sounds like this guarantees you won't get UNKOWN_TYPE out of this fn, but I
> don't think that's correct.

Done.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/field_types.h (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/field_types.h:13: #include
"base/strings/string16.h"
On 2013/08/05 18:47:24, Evan Stade wrote:
> ?

Done.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/field_types.h:24: enum NativeFieldType {
On 2013/08/05 18:47:24, Evan Stade wrote:
> can we call this something else? AutofillServerFieldType, or if that's too
long,
> AutofillServerType, ServerNativeType, or something. Since it seems that chrome
> and the server don't agree on the set of types, it's not obvious what "native"
> refers to (you have to know it means "native to the server, not chrome or
> html"). I guess my objection is that everything is "native" to something.

Done.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/form_structure.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/form_structure.cc:193: HtmlFieldType
FieldTypeFromAutocompleteType(
On 2013/08/05 18:47:24, Evan Stade wrote:
> maybe you should rename this to FieldTypeFromAutocompleteAttribute or
> FieldTypeFromAutocompleteValue (because "AutocompleteType" would be a logical
> alternative name for "HtmlFieldType")

Done.

[Evan Stade, 2013-08-06 18:39:59]

https://codereview.chromium.org/22040002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/22040002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/data_model_wrapper.cc:126: HtmlFieldType html_type =
field->html_type();
On 2013/08/06 05:05:39, Ilya Sherman (Away Aug. 9-25) wrote:
> On 2013/08/05 18:47:24, Evan Stade wrote:
> > nit: declare as late as possible, not at top of function.
> 
> Hmm, I'm not sure where you'd prefer to see this declared.  It needs to come
> before line 133, since the goal is to cache the type prior to overwriting it;
> and declaring the variable just within the if-stmt doesn't work, as the value
> needs to be read at line 138.

I see, I missed that. Can you give the variable a more descriptive name like
|original_html_type|? I also think the function would overall be easier to
understand as:

if (condition) {
  read old type, set temporary, fill, restore old type
} else {
  fill
}

This makes it clearer what's necessary under what circumstances, and only has to
repeat a single line.

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/autofill_profile.cc:37:
AutofillType(type).GetEquivalentNativeType();
On 2013/08/06 05:05:39, Ilya Sherman (Away Aug. 9-25) wrote:
> On 2013/08/05 18:47:24, Evan Stade wrote:
> > this fn seems a little bit hoop-jumpy. Would it be the same to do:
> > 
> > AutofillType autofill_type = AutofillType(type);
> > if (type.group() == NAME)
> >   return NAME_FULL;
> > 
> > return autofill_type.GetEquivalentNativeType();
> 
> It's not quite the same unless we also check for NAME_BILLING in the if-stmt. 
> Let me know whether you prefer that (as done in the newer patch set) or the
way
> that it was here.

I think the original way is fine now that I understand GetEquivalentNativeType 
better.

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
File components/autofill/core/browser/autofill_type.h (right):

https://codereview.chromium.org/22040002/diff/5001/components/autofill/core/b...
components/autofill/core/browser/autofill_type.h:34: NativeFieldType
GetEquivalentNativeType() const;
On 2013/08/06 05:05:39, Ilya Sherman (Away Aug. 9-25) wrote:
> On 2013/08/05 18:47:24, Evan Stade wrote:
> > while "equivalent" was probably never the easiest to understand name, this
is
> > now even more confusing because it sounds like it's just mapping from
> html_type_
> > to native_type_. However it's also possible to spit out a native type that
> > doesn't match native_type_. GetStorableNativeType? GetChromeAutofillType?
> 
> Done.
> 
> > I'm also a bit worried that it will be easy to use GetEquivalent or
> > GetApproximate when the other is more appropriate, since in most cases it
does
> > the same thing. Is there a way you can make that mixup less likely?
> 
> Does the added explanation in the comment for GetApproximateStorableType()
help?

I was hoping for an api change which would make it more apparent. The new
comment indicates GetApproximate is useful for comparisons between fields, could
you perhaps make it a function on FormField which is an alternative to Type()?

Is there ever a time when using GetApproximateStorableType() instead of
GetStorableType() would lead to a bug? I can't see one off hand. If not you
could just nix GetStorableType().

[Ilya Sherman, 2013-08-06 23:04:56]

Could you please also take a look at [
https://chromiumcodereview.appspot.com/22009003/ ]?  That CL will need to land
first, as it's what this CL is based on top of.  Thanks!

https://chromiumcodereview.appspot.com/22040002/diff/5001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/chrome/browser/ui/a...
chrome/browser/ui/autofill/data_model_wrapper.cc:126: HtmlFieldType html_type =
field->html_type();
On 2013/08/06 18:39:59, Evan Stade wrote:
> On 2013/08/06 05:05:39, Ilya Sherman (Away Aug. 9-25) wrote:
> > On 2013/08/05 18:47:24, Evan Stade wrote:
> > > nit: declare as late as possible, not at top of function.
> > 
> > Hmm, I'm not sure where you'd prefer to see this declared.  It needs to come
> > before line 133, since the goal is to cache the type prior to overwriting
it;
> > and declaring the variable just within the if-stmt doesn't work, as the
value
> > needs to be read at line 138.
> 
> I see, I missed that. Can you give the variable a more descriptive name like
> |original_html_type|? I also think the function would overall be easier to
> understand as:
> 
> if (condition) {
>   read old type, set temporary, fill, restore old type
> } else {
>   fill
> }
> 
> This makes it clearer what's necessary under what circumstances, and only has
to
> repeat a single line.

Done.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/autofill_profile.cc (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/autofill_profile.cc:37:
AutofillType(type).GetEquivalentNativeType();
On 2013/08/06 18:39:59, Evan Stade wrote:
> On 2013/08/06 05:05:39, Ilya Sherman (Away Aug. 9-25) wrote:
> > On 2013/08/05 18:47:24, Evan Stade wrote:
> > > this fn seems a little bit hoop-jumpy. Would it be the same to do:
> > > 
> > > AutofillType autofill_type = AutofillType(type);
> > > if (type.group() == NAME)
> > >   return NAME_FULL;
> > > 
> > > return autofill_type.GetEquivalentNativeType();
> > 
> > It's not quite the same unless we also check for NAME_BILLING in the
if-stmt. 
> > Let me know whether you prefer that (as done in the newer patch set) or the
> way
> > that it was here.
> 
> I think the original way is fine now that I understand GetEquivalentNativeType

> better.

Done.

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
File components/autofill/core/browser/autofill_type.h (right):

https://chromiumcodereview.appspot.com/22040002/diff/5001/components/autofill...
components/autofill/core/browser/autofill_type.h:34: NativeFieldType
GetEquivalentNativeType() const;
On 2013/08/06 18:39:59, Evan Stade wrote:
> On 2013/08/06 05:05:39, Ilya Sherman (Away Aug. 9-25) wrote:
> > On 2013/08/05 18:47:24, Evan Stade wrote:
> > > while "equivalent" was probably never the easiest to understand name, this
> is
> > > now even more confusing because it sounds like it's just mapping from
> > html_type_
> > > to native_type_. However it's also possible to spit out a native type that
> > > doesn't match native_type_. GetStorableNativeType? GetChromeAutofillType?
> > 
> > Done.
> > 
> > > I'm also a bit worried that it will be easy to use GetEquivalent or
> > > GetApproximate when the other is more appropriate, since in most cases it
> does
> > > the same thing. Is there a way you can make that mixup less likely?
> > 
> > Does the added explanation in the comment for GetApproximateStorableType()
> help?
> 
> I was hoping for an api change which would make it more apparent. The new
> comment indicates GetApproximate is useful for comparisons between fields,
could
> you perhaps make it a function on FormField which is an alternative to Type()?
> 
> Is there ever a time when using GetApproximateStorableType() instead of
> GetStorableType() would lead to a bug? I can't see one off hand. If not you
> could just nix GetStorableType().

The sort of thing that could lead to a bug is e.g. calling
SetInfo(HTML_TYPE_STREET_ADDRESS) and having the full address end up as line 1
of the resulting address.

Let's actually punt this issue to a separate CL, as it's pretty subtle, and easy
enough to get wrong.  I'll just drop support for 'street-address' from this CL
for now, which is the only type for which approximate vs. exact matters.  SG?

[Evan Stade, 2013-08-06 23:07:35]

lgtm

[Ilya Sherman, 2013-08-07 05:01:26]

TBR zea for chrome/browser/sync/test/integration/autofill_helper.cc

[commit-bot: I haz the power, 2013-08-07 05:01:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/22040002/17004

[commit-bot: I haz the power, 2013-08-07 05:54:55]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-08-07 06:18:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/22040002/112001

[commit-bot: I haz the power, 2013-08-07 06:25:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-07 06:31:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/22040002/112001

[commit-bot: I haz the power, 2013-08-07 10:17:18]

Change committed as 216149