Autofill: Set requirements for number of recognized fields in an autofillable form

Autofill: Set requirements for number of recognized fields in an autofillable form

BUG=447332
Committed: https://crrev.com/97338dbd9d3367fec22d2f690156df85c0b2572b
Cr-Commit-Position: refs/heads/master@{#313551}

[Lei Zhang, 2015-01-15 04:04:38]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2015-01-15 04:11:37]

https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
File components/autofill/core/browser/address_field.cc (right):

https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
components/autofill/core/browser/address_field.cc:79: bool
has_valid_non_street_address_field = (address_field->company_ ||
nit: I think we should require at least three recognized address fields (not
just two).

[Evan Stade, 2015-01-15 04:12:23]

https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
File components/autofill/core/browser/address_field.cc (right):

https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
components/autofill/core/browser/address_field.cc:79: bool
has_valid_non_street_address_field = (address_field->company_ ||
On 2015/01/15 04:11:37, Evan Stade wrote:
> nit: I think we should require at least three recognized address fields (not
> just two).

also, as this code is written, wouldn't the bug still occur for a docs comment
that says "State"?

[Lei Zhang, 2015-01-17 04:18:02]

On 2015/01/15 04:11:37, Evan Stade wrote:
>
https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
> File components/autofill/core/browser/address_field.cc (right):
> 
>
https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
> components/autofill/core/browser/address_field.cc:79: bool
> has_valid_non_street_address_field = (address_field->company_ ||
> nit: I think we should require at least three recognized address fields (not
> just two).

Many tests fail when I tried that. Even 3 address fields on the entire form is
not always possible. In patch set 3, I changed it to check if the sum of all
recognized fields is at least 3, and even that causes some of the registration
tests to fail. (See modified test expectations.)

[Evan Stade, 2015-01-18 03:59:50]

On 2015/01/17 04:18:02, Lei Zhang wrote:
> On 2015/01/15 04:11:37, Evan Stade wrote:
> >
>
https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
> > File components/autofill/core/browser/address_field.cc (right):
> > 
> >
>
https://codereview.chromium.org/853523004/diff/20001/components/autofill/core...
> > components/autofill/core/browser/address_field.cc:79: bool
> > has_valid_non_street_address_field = (address_field->company_ ||
> > nit: I think we should require at least three recognized address fields (not
> > just two).
> 
> Many tests fail when I tried that. Even 3 address fields on the entire form is
> not always possible. In patch set 3, I changed it to check if the sum of all
> recognized fields is at least 3, and even that causes some of the registration
> tests to fail. (See modified test expectations.)

I don't think you need to require 3 recognized fields. A single recognized email
field should suffice. Only physical addresses need to have at least 3. Without
seeing which tests failed and how they failed, I can't judge whether those are
"good" or bad failures.

[Lei Zhang, 2015-01-21 08:52:48]

On 2015/01/18 03:59:50, Evan Stade wrote:
> I don't think you need to require 3 recognized fields. A single recognized
email
> field should suffice. Only physical addresses need to have at least 3. Without
> seeing which tests failed and how they failed, I can't judge whether those are
> "good" or bad failures.

I made the email field an exception, but only for forms that are owned by <form>
tags, per in-person discussion. There's a lot of plumbing in patch set 4, to get
the "unowned" bit into form_field.cc. Now all the test cases are happy and just
having the word "email" in an unowned form won't trigger autofill either.

The test failures I spoke of for the "requiring at least 3 recognized address
fields" experiment is in the registration test samples. Several registration
pages only required email + zip, or email + city + state.

[Evan Stade, 2015-01-21 22:52:50]

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/address_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/address_field.cc:153: return count;;
nit: ;;

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/credit_card_field.cc:255: count +=
numbers_.size();
this should probably be

if (!numbers_.empty())
  ++count;

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:77: bool ignore_parsing = (count
< kThreshold);
nit: no parens

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:80: if (!is_unowned_form)
&= is a confusing operator. How about:

bool should_parse = count >= kThreshold ||
    (is_in_form && email_count > 0);
if (!should_parse)
  *map = saved_map;

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:185: field_count +=
form_field->FieldCount();
instead of adding this function, couldn't you more or less just look at the
difference in size |fields| has at the beginning and end of this function?

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/form_field.h (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.h:32: bool is_unowned_form,
nit: |unowned_form| is a slight misnomer. How about |is_in_a_form|?

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/name_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/name_field.cc:82: // This is overriden in
concrete subclasses.
nit: spelling

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/phone_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/phone_field.cc:243: size_t
PhoneField::FieldCount() const {
why not just return 1? Why should three separate inputs [   ] [   ] [    ] count
differently from one big one [          ]?

[Lei Zhang, 2015-01-22 08:07:37]

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/address_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/address_field.cc:153: return count;;
On 2015/01/21 22:52:49, Evan Stade wrote:
> nit: ;;

Done.

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/credit_card_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/credit_card_field.cc:255: count +=
numbers_.size();
On 2015/01/21 22:52:49, Evan Stade wrote:
> this should probably be
> 
> if (!numbers_.empty())
>   ++count;

Done.

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:80: if (!is_unowned_form)
On 2015/01/21 22:52:49, Evan Stade wrote:
> &= is a confusing operator. How about:
> 
> bool should_parse = count >= kThreshold ||
>     (is_in_form && email_count > 0);
> if (!should_parse)
>   *map = saved_map;

Done.

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:185: field_count +=
form_field->FieldCount();
On 2015/01/21 22:52:49, Evan Stade wrote:
> instead of adding this function, couldn't you more or less just look at the
> difference in size |fields| has at the beginning and end of this function?

Isn't it possible for functions like AddressField::ParseAddressLines() to
consume fields without parsing them into anything usable?

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/form_field.h (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.h:32: bool is_unowned_form,
On 2015/01/21 22:52:50, Evan Stade wrote:
> nit: |unowned_form| is a slight misnomer. How about |is_in_a_form|?

is_in_form_tag

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/name_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/name_field.cc:82: // This is overriden in
concrete subclasses.
On 2015/01/21 22:52:50, Evan Stade wrote:
> nit: spelling

Done.

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/phone_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/phone_field.cc:243: size_t
PhoneField::FieldCount() const {
On 2015/01/21 22:52:50, Evan Stade wrote:
> why not just return 1? Why should three separate inputs [   ] [   ] [    ]
count
> differently from one big one [          ]?

Recognizing many fields in a phone number is a stronger signal than just one
field that's a PHONE_HOME_WHOLE_NUMBER, which could be the type of badly
classified fields we are trying to address in this CL.

[Evan Stade, 2015-01-22 17:17:05]

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:185: field_count +=
form_field->FieldCount();
On 2015/01/22 08:07:37, Lei Zhang wrote:
> On 2015/01/21 22:52:49, Evan Stade wrote:
> > instead of adding this function, couldn't you more or less just look at the
> > difference in size |fields| has at the beginning and end of this function?
> 
> Isn't it possible for functions like AddressField::ParseAddressLines() to
> consume fields without parsing them into anything usable?

perhaps... but you could check the size of |map| after classifications?

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:189: for (const auto& form_field
: form_fields) {
why is this a separate loop now?

also, I think auto is great for iterators, but less clear for simple types like
const FormField&

[Lei Zhang, 2015-01-22 23:14:15]

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:185: field_count +=
form_field->FieldCount();
On 2015/01/22 17:17:05, Evan Stade wrote:
> On 2015/01/22 08:07:37, Lei Zhang wrote:
> > On 2015/01/21 22:52:49, Evan Stade wrote:
> > > instead of adding this function, couldn't you more or less just look at
the
> > > difference in size |fields| has at the beginning and end of this function?
> > 
> > Isn't it possible for functions like AddressField::ParseAddressLines() to
> > consume fields without parsing them into anything usable?
> 
> perhaps... but you could check the size of |map| after classifications?

Sounds better, done.

https://codereview.chromium.org/853523004/diff/60001/components/autofill/core...
components/autofill/core/browser/form_field.cc:189: for (const auto& form_field
: form_fields) {
On 2015/01/22 17:17:05, Evan Stade wrote:
> why is this a separate loop now?

I was experimenting with different ways to count fields earlier. Reverted.

> also, I think auto is great for iterators, but less clear for simple types
like
> const FormField&

Sure.

[Evan Stade, 2015-01-22 23:17:04]

https://codereview.chromium.org/853523004/diff/120001/components/autofill/cor...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/120001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:191: return map->size() -
initial_field_count;
this doesn't need to be part of ParseFormFieldsPass, as you can do it from
ParseFormFields

[Lei Zhang, 2015-01-22 23:32:05]

https://codereview.chromium.org/853523004/diff/120001/components/autofill/cor...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/120001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:191: return map->size() -
initial_field_count;
On 2015/01/22 23:17:04, Evan Stade wrote:
> this doesn't need to be part of ParseFormFieldsPass, as you can do it from
> ParseFormFields

Done.

[Evan Stade, 2015-01-22 23:43:45]

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
File components/autofill/core/browser/email_field.cc (right):

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/email_field.cc:26: DCHECK(field);
if you want to enforce this, I think the param should be a const ref

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:44: ServerFieldTypeMap* map) {
DCHECK(map->empty())?

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:76: size_t count = map->size();
nit: no need for this variable

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
File components/autofill/core/browser/name_field.cc (right):

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/name_field.cc:75: bool
NameField::ClassifyField(ServerFieldTypeMap* map) const {
why is this function even implemented if this is an abstract class?

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/name_field.cc:104: DCHECK(field);
ditto

https://codereview.chromium.org/853523004/diff/180001/components/autofill/con...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/con...
components/autofill/content/renderer/form_cache.cc:81: *unowned_form_index = 0;
I think it's cleaner if fakeness is an attribute of FormData

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:391: if (unowned_form_index
> forms.size())
when does this happen?

[Lei Zhang, 2015-01-22 23:58:13]

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
File components/autofill/core/browser/email_field.cc (right):

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/email_field.cc:26: DCHECK(field);
On 2015/01/22 23:43:44, Evan Stade wrote:
> if you want to enforce this, I think the param should be a const ref

Reverted.

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:44: ServerFieldTypeMap* map) {
On 2015/01/22 23:43:44, Evan Stade wrote:
> DCHECK(map->empty())?

Done.

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:76: size_t count = map->size();
On 2015/01/22 23:43:44, Evan Stade wrote:
> nit: no need for this variable

Done.

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
File components/autofill/core/browser/name_field.cc (right):

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/name_field.cc:75: bool
NameField::ClassifyField(ServerFieldTypeMap* map) const {
On 2015/01/22 23:43:44, Evan Stade wrote:
> why is this function even implemented if this is an abstract class?

I have no idea. I can take a look later. Reverting for now.

https://codereview.chromium.org/853523004/diff/160001/components/autofill/cor...
components/autofill/core/browser/name_field.cc:104: DCHECK(field);
On 2015/01/22 23:43:44, Evan Stade wrote:
> ditto

Reverted.

https://codereview.chromium.org/853523004/diff/180001/components/autofill/con...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/con...
components/autofill/content/renderer/form_cache.cc:81: *unowned_form_index = 0;
On 2015/01/22 23:43:44, Evan Stade wrote:
> I think it's cleaner if fakeness is an attribute of FormData

FormData gets serialized as part of a PasswordForm. Touching that opens a big
can of worms.

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:391: if (unowned_form_index
> forms.size())
On 2015/01/22 23:43:45, Evan Stade wrote:
> when does this happen?

It shouldn't, but we can't trust the IPC from the renderer, so we need to check.
Otherwise a compromised renderer can trivially cause out of bound memory access.

[Evan Stade, 2015-01-23 02:21:47]

https://codereview.chromium.org/853523004/diff/180001/components/autofill/con...
File components/autofill/content/renderer/form_cache.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/con...
components/autofill/content/renderer/form_cache.cc:81: *unowned_form_index = 0;
On 2015/01/22 23:58:13, Lei Zhang wrote:
> On 2015/01/22 23:43:44, Evan Stade wrote:
> > I think it's cleaner if fakeness is an attribute of FormData
> 
> FormData gets serialized as part of a PasswordForm. Touching that opens a big
> can of worms.

yea but this is not sustainable either. Next time we want to add some flag or
other to FormData, are we going to just keep plumbing extra parameters around
everywhere a FormData is passed?

I don't know if the serialization/deserialization code has to change. Maybe
serialization/deserialization doesn't care about synthetic form tags?

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:391: if (unowned_form_index
> forms.size())
On 2015/01/22 23:58:13, Lei Zhang wrote:
> On 2015/01/22 23:43:45, Evan Stade wrote:
> > when does this happen?
> 
> It shouldn't, but we can't trust the IPC from the renderer, so we need to
check.
> Otherwise a compromised renderer can trivially cause out of bound memory
access.

at least notreached() then? Can you get an IPC owner to confirm this is the
proper way to handle a compromised renderer? (If you follow my advice on
FormData this becomes irrelevant.)

[Lei Zhang, 2015-01-23 06:50:17]

+gcasto

If I add a boolean to FormData to indicate whether the form is from a <form> tag
or not, then the binary pickle format needs another bump to version 3. We
probably won't do this often, but it would get annoying after a while.

I wonder if we really need to serialize all the fields in FormData and
FormFieldData. All of this is to support http://crbug.com/240559 right? Does the
server even care about this? Is the server side ready to handle version 2
pickles for M41+ ?

[Lei Zhang, 2015-01-23 06:59:48]

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/853523004/diff/180001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:391: if (unowned_form_index
> forms.size())
On 2015/01/23 02:21:46, Evan Stade wrote:
> On 2015/01/22 23:58:13, Lei Zhang wrote:
> > On 2015/01/22 23:43:45, Evan Stade wrote:
> > > when does this happen?
> > 
> > It shouldn't, but we can't trust the IPC from the renderer, so we need to
> check.
> > Otherwise a compromised renderer can trivially cause out of bound memory
> access.
> 
> at least notreached() then? Can you get an IPC owner to confirm this is the
> proper way to handle a compromised renderer? (If you follow my advice on
> FormData this becomes irrelevant.)

ValidFormDataVector() below doesn't have a NOTREACHED() either, so I'm being
consistent. Another option is to call ReceivedBadMessage() and kill the
renderer. This CL will touch IPC no matter what, so I'll ask when the time
comes.

[Garrett Casto, 2015-01-23 23:15:03]

On 2015/01/23 06:50:17, Lei Zhang wrote:
> +gcasto
> 
> If I add a boolean to FormData to indicate whether the form is from a <form>
tag
> or not, then the binary pickle format needs another bump to version 3. We
> probably won't do this often, but it would get annoying after a while.
> 
> I wonder if we really need to serialize all the fields in FormData and
> FormFieldData. All of this is to support http://crbug.com/240559 right? Does
the
> server even care about this? Is the server side ready to handle version 2
> pickles for M41+ ?

The only thing that gets sent to the server is the signatures of the form and
fields, so there is no extra server support needed generally when FormData is
changed. The reason we serialize this data is that we want to be able to check
when a password is first used if it used on the same form that it was originally
saved at, and if not we want to upload an autofill vote for the original form.
So if this new tag is going to be used to calculate a signature, then it needs
to be serialized. Otherwise I suppose it doesn't have to, though the possible
confusing about what should be serialized doesn't really seem like it's worth
the slightly less effort for not serializing.

While I was thinking about this, it occurred to me that there is one other
behavior that password generation currently relies on that isn't totally
obvious. That is that we currently require that the PasswordForm parsed from the
current page and the one that you get from the FormStructure after the server
response are equal according to SameFormAs().   I believe that this is currently
always true, but since we are already on the topic I thought I would bring it
up.

[Lei Zhang, 2015-01-24 02:49:34]

On 2015/01/23 23:15:03, Garrett Casto wrote:
> The only thing that gets sent to the server is the signatures of the form and
> fields, so there is no extra server support needed generally when FormData is
> changed. The reason we serialize this data is that we want to be able to check
> when a password is first used if it used on the same form that it was
originally
> saved at, and if not we want to upload an autofill vote for the original form.
> So if this new tag is going to be used to calculate a signature, then it needs
> to be serialized. Otherwise I suppose it doesn't have to, though the possible
> confusing about what should be serialized doesn't really seem like it's worth
> the slightly less effort for not serializing.

Ok, sounds good. I went ahead and serialized it. It wasn't as big of a pain as I
thought it would be.

[Evan Stade, 2015-01-26 17:59:45]

lgtm

can you fix the CL description? it seems ungrammatical to me

https://codereview.chromium.org/853523004/diff/240001/components/autofill/cor...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/853523004/diff/240001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:257: // True if the form is in
a <form>.
more like is-a form, right?

[Lei Zhang, 2015-01-28 01:37:35]

CL description updated

https://codereview.chromium.org/853523004/diff/240001/components/autofill/cor...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/853523004/diff/240001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:257: // True if the form is in
a <form>.
On 2015/01/26 17:59:45, Evan Stade wrote:
> more like is-a form, right?

sure.

[Lei Zhang, 2015-01-28 17:37:42]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-01-28 17:38:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853523004/260001

[commit-bot: I haz the power, 2015-01-28 18:50:50]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-01-28 18:52:08]

Patchset 14 (id:??) landed as
https://crrev.com/97338dbd9d3367fec22d2f690156df85c0b2572b
Cr-Commit-Position: refs/heads/master@{#313551}

[Ilya Sherman, 2015-01-28 19:30:22]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-01-28 19:30:23]

Apologies for the drive-by -- looked out of curiousity, and spotted a couple of
things I have questions about:

https://codereview.chromium.org/853523004/diff/260001/chrome/test/data/autofi...
File chrome/test/data/autofill/heuristics/output/bug_447332.out (right):

https://codereview.chromium.org/853523004/diff/260001/chrome/test/data/autofi...
chrome/test/data/autofill/heuristics/output/bug_447332.out:3: UNKNOWN_TYPE |
test2 | State your intentions |  | test0_1-default
Why are these all parsed as unknown, if we need at least three fields to detect
a form as autofillable?  There are three "gotcha" fields, afaict.  Is there an
off-by-one error somewhere, or is this test not quite testing what you'd
intended for it to test?

https://codereview.chromium.org/853523004/diff/260001/components/autofill/cor...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/260001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:77: // only recognized field on
account registration sites.
Why does this apply only for <form> tags?  Are account registration forms less
likely to be implemented as <form>-less forms?  Is there a risk to treating
email forms uniformly for every form-like bag of fields that Autofill sees?

[Lei Zhang, 2015-01-28 19:46:42]

https://codereview.chromium.org/853523004/diff/260001/chrome/test/data/autofi...
File chrome/test/data/autofill/heuristics/output/bug_447332.out (right):

https://codereview.chromium.org/853523004/diff/260001/chrome/test/data/autofi...
chrome/test/data/autofill/heuristics/output/bug_447332.out:3: UNKNOWN_TYPE |
test2 | State your intentions |  | test0_1-default
On 2015/01/28 19:30:23, Ilya Sherman wrote:
> Why are these all parsed as unknown, if we need at least three fields to
detect
> a form as autofillable?  There are three "gotcha" fields, afaict.  Is there an
> off-by-one error somewhere, or is this test not quite testing what you'd
> intended for it to test?

The input fields are <textarea>'s just like on Google Docs. Autofill only looks
for addresses in <textarea> tags, but not city/state. Thus the input really
parses to HOME_ADDRESS/UNKNOWN/UNKNOWN/UNKNOWN, which then gets rejected.

https://codereview.chromium.org/853523004/diff/260001/components/autofill/cor...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/853523004/diff/260001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:77: // only recognized field on
account registration sites.
On 2015/01/28 19:30:23, Ilya Sherman wrote:
> Why does this apply only for <form> tags?  Are account registration forms less
> likely to be implemented as <form>-less forms?  Is there a risk to treating
> email forms uniformly for every form-like bag of fields that Autofill sees?

Treating email fields uniformly risks more bugs like the one you filed, but with
email as the keyword instead of address. I added the email field exception for
the ~4 registration test cases that are in <form> tags, and there are only 1 or
2 recognized fields. Many registration forms just want something like email +
zip code.

[Ilya Sherman, 2015-01-28 19:53:59]

Alright, thanks for the replies.