[Password Generation] Always query password forms via Autofill

[Password Generation] Always query password forms via Autofill

For other fields, we assume that if the author has specified autocomplete
attributes we should respect all their choices, including not trying to
determine field autocomplete values for fields that the author didn't specify.

This doesn't apply for password fields for two reasons. First, there is no way
for an author to specify generation for a particular field. Second, since
generation isn't a specced autocomplete value, the password field can reasonably
be set (e.g. "autocomplete=new-password") and also have generation enabled.

BUG=424793
Committed: https://crrev.com/c57dd5602fee1bc2fa1df2424c5f7b95a1702cf4
Cr-Commit-Position: refs/heads/master@{#301521}

[Garrett Casto, 2014-10-23 22:48:16]

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

[Garrett Casto, 2014-10-23 22:48:17]

Hey Ilya, please take a look. I was thinking of adding one additional test to
verify the behavior of FormStructure::ParseQueryResponse(), but I couldn't find
a current test. Is there one that I just can't find, and if not do you think we
should add one somewhere?

[Ilya Sherman, 2014-10-24 01:23:39]

On 2014/10/23 22:48:17, Garrett Casto wrote:
> Hey Ilya, please take a look. I was thinking of adding one additional test to
> verify the behavior of FormStructure::ParseQueryResponse(), but I couldn't
find
> a current test. Is there one that I just can't find, and if not do you think
we
> should add one somewhere?

I'm not sure whether there is a current one somewhere.  I also couldn't find one
just now, after some looking around :/

It would definitely be good to add one :)

[Ilya Sherman, 2014-10-24 01:24:46]

https://codereview.chromium.org/659793005/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_server_browsertest.cc (right):

https://codereview.chromium.org/659793005/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_server_browsertest.cc:73: LOG(INFO) <<
fetcher->upload_data();
nit: Please revert.

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

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:593:
non_password_field_processed = true;
Wait, where do you check that a non-password field was processed?  This could
still be a password form, even if no autocomplete types are manually specified
by the site author, right?

I think this is a useful change, but I'd recommend splitting it off to a
separate CL.

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:731: bool
FormStructure::ShouldSkipProcessingNonPasswordFields() const {
This method name is pretty long and, IMO, still a little bit confusing.  Since
you only use this in one place (afaict), I'd recommend just inlining the check.

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

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:489: // considered
autofillable though.
Please create a separate TEST() for this case.

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:506:
EXPECT_EQ(UNKNOWN_TYPE, form_structure->field(3)->heuristic_type());
I'm not convinced that most of these expectations are needed.  I think you can
simplify the test to:

// Create test form.
...

FormStructure form_structure(form);
form_structure.DetermineHeuristicTypes(TestAutofillMetrics());
EXPECT_TRUE(form_structure.ShouldBeCrowdsourced());

[Garrett Casto, 2014-10-24 21:13:10]

I added two relatively simple tests for FormStructure::ParseQueryResponse().
PTAL

https://codereview.chromium.org/659793005/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_server_browsertest.cc (right):

https://codereview.chromium.org/659793005/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_server_browsertest.cc:73: LOG(INFO) <<
fetcher->upload_data();
On 2014/10/24 01:24:45, Ilya Sherman wrote:
> nit: Please revert.

Done.

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

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:593:
non_password_field_processed = true;
On 2014/10/24 01:24:45, Ilya Sherman wrote:
> Wait, where do you check that a non-password field was processed?  This could
> still be a password form, even if no autocomplete types are manually specified
> by the site author, right?
> 
> I think this is a useful change, but I'd recommend splitting it off to a
> separate CL.

I agree that the name doesn't do a great job of expressing the intent here.
Basically I was trying to keep the current behavior for metrics so that the new
requests wouldn't be logged. The percentage of such requests is likely very low
though, so it probably doesn't matter. I'll just pull this out for now and file
a bug.

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:731: bool
FormStructure::ShouldSkipProcessingNonPasswordFields() const {
On 2014/10/24 01:24:45, Ilya Sherman wrote:
> This method name is pretty long and, IMO, still a little bit confusing.  Since
> you only use this in one place (afaict), I'd recommend just inlining the
check.

Done.

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

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:489: // considered
autofillable though.
On 2014/10/24 01:24:45, Ilya Sherman wrote:
> Please create a separate TEST() for this case.

Done.

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:506:
EXPECT_EQ(UNKNOWN_TYPE, form_structure->field(3)->heuristic_type());
On 2014/10/24 01:24:45, Ilya Sherman wrote:
> I'm not convinced that most of these expectations are needed.  I think you can
> simplify the test to:
> 
> // Create test form.
> ...
> 
> FormStructure form_structure(form);
> form_structure.DetermineHeuristicTypes(TestAutofillMetrics());
> EXPECT_TRUE(form_structure.ShouldBeCrowdsourced());

Do we not want to check that IsAutofillable() returns false?

[Garrett Casto, 2014-10-24 21:55:58]

One sec, it looks like there is an issue with the test that I need to
investigate.

[Garrett Casto, 2014-10-24 22:09:43]

Hadn't uploaded the latest version of the test. Sorry about that. PTAL.

[Ilya Sherman, 2014-10-24 22:42:31]

Thanks for adding tests, Garrett :)

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

https://codereview.chromium.org/659793005/diff/20001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:506:
EXPECT_EQ(UNKNOWN_TYPE, form_structure->field(3)->heuristic_type());
On 2014/10/24 21:13:10, Garrett Casto wrote:
> On 2014/10/24 01:24:45, Ilya Sherman wrote:
> > I'm not convinced that most of these expectations are needed.  I think you
can
> > simplify the test to:
> > 
> > // Create test form.
> > ...
> > 
> > FormStructure form_structure(form);
> > form_structure.DetermineHeuristicTypes(TestAutofillMetrics());
> > EXPECT_TRUE(form_structure.ShouldBeCrowdsourced());
> 
> Do we not want to check that IsAutofillable() returns false?

It's not obvious to me why that's relevant to this test, but it also doesn't
hurt to add that one check.  The main advantage of dropping it is that it
emphasizes the single check that the test is really focused on.

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

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:372: has_password_field_ =
true;
Now that this is only used in one place, I wonder if it's really worth caching
as a member variable.  I don't feel strongly either way, but I lean slightly
toward computing on the fly.

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:590: // doesn't handle them.
UMA stats can also be skipped in that case.
nit: This comment talks about UMA stats, but I don't think it's relevant
anymore.

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:592:
(*field)->set_server_type(current_info->field_type);
I think it would be simpler to write this code as a continue stmt, i.e.

if (form->has_author_specified_types_ &&
    (*field)->form_control_type != "password")
  continue;

You can then reduce the indentation level for the remainder of the code, which
improves readability IMO.

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

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:492:
scoped_ptr<FormStructure> form_structure;
nit: Why use a scoped_ptr here, rather than stack-allocating at line 517?

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:2434:
STLDeleteContainerPointers(forms.begin(), forms.end());
Oof.  Can we use a ScopedVector to control memory management?

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:2466: EXPECT_EQ(0,
forms[0]->field(0)->server_type());
nit: I think UNKNOWN_TYPE rather than "0" would be a little clearer, here.

[Garrett Casto, 2014-10-24 23:19:52]

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

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:372: has_password_field_ =
true;
On 2014/10/24 22:42:30, Ilya Sherman wrote:
> Now that this is only used in one place, I wonder if it's really worth caching
> as a member variable.  I don't feel strongly either way, but I lean slightly
> toward computing on the fly.

I think that I slightly prefer keeping it here just so we don't have to loop
through all the fields whenever ShouldBeCrowdsourced() is called. Performance
obviously isn't a concern here, but it doesn't seem clearly cleaner to me the
other way. Your call.

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:590: // doesn't handle them.
UMA stats can also be skipped in that case.
On 2014/10/24 22:42:30, Ilya Sherman wrote:
> nit: This comment talks about UMA stats, but I don't think it's relevant
> anymore.

Removed.

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:592:
(*field)->set_server_type(current_info->field_type);
On 2014/10/24 22:42:30, Ilya Sherman wrote:
> I think it would be simpler to write this code as a continue stmt, i.e.
> 
> if (form->has_author_specified_types_ &&
>     (*field)->form_control_type != "password")
>   continue;
> 
> You can then reduce the indentation level for the remainder of the code, which
> improves readability IMO.

I had originally done it this way to avoid influencing the metrics and to avoid
having to duplicate the loop iteration. We are punting on the former, but I
generally try to avoid early exits with manual iteration as it's a source of
bugs. I switched this to one level of indentation without an "else" statement.
WDYT?

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

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:492:
scoped_ptr<FormStructure> form_structure;
On 2014/10/24 22:42:31, Ilya Sherman wrote:
> nit: Why use a scoped_ptr here, rather than stack-allocating at line 517?

The evils of copy and paste. It is rather common in the rest of the file for
some reason. Changed.

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:2434:
STLDeleteContainerPointers(forms.begin(), forms.end());
On 2014/10/24 22:42:31, Ilya Sherman wrote:
> Oof.  Can we use a ScopedVector to control memory management?

Done.

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure_unittest.cc:2466: EXPECT_EQ(0,
forms[0]->field(0)->server_type());
On 2014/10/24 22:42:31, Ilya Sherman wrote:
> nit: I think UNKNOWN_TYPE rather than "0" would be a little clearer, here.

It's actually NO_SERVER_DATA, but I see your point. Done.

[Ilya Sherman, 2014-10-24 23:40:14]

Thanks, Garrett =)

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

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:592:
(*field)->set_server_type(current_info->field_type);
On 2014/10/24 23:19:52, Garrett Casto wrote:
> On 2014/10/24 22:42:30, Ilya Sherman wrote:
> > I think it would be simpler to write this code as a continue stmt, i.e.
> > 
> > if (form->has_author_specified_types_ &&
> >     (*field)->form_control_type != "password")
> >   continue;
> > 
> > You can then reduce the indentation level for the remainder of the code,
which
> > improves readability IMO.
> 
> I had originally done it this way to avoid influencing the metrics and to
avoid
> having to duplicate the loop iteration. We are punting on the former, but I
> generally try to avoid early exits with manual iteration as it's a source of
> bugs. I switched this to one level of indentation without an "else" statement.
> WDYT?

I really prefer less indentation over avoiding continue stmts.  Note that
there's already a continue and a break stmt right above this check, so it fits
with the theme of this loop.

[Garrett Casto, 2014-10-25 00:04:54]

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

https://codereview.chromium.org/659793005/diff/60001/components/autofill/core...
components/autofill/core/browser/form_structure.cc:592:
(*field)->set_server_type(current_info->field_type);
On 2014/10/24 23:40:14, Ilya Sherman wrote:
> On 2014/10/24 23:19:52, Garrett Casto wrote:
> > On 2014/10/24 22:42:30, Ilya Sherman wrote:
> > > I think it would be simpler to write this code as a continue stmt, i.e.
> > > 
> > > if (form->has_author_specified_types_ &&
> > >     (*field)->form_control_type != "password")
> > >   continue;
> > > 
> > > You can then reduce the indentation level for the remainder of the code,
> which
> > > improves readability IMO.
> > 
> > I had originally done it this way to avoid influencing the metrics and to
> avoid
> > having to duplicate the loop iteration. We are punting on the former, but I
> > generally try to avoid early exits with manual iteration as it's a source of
> > bugs. I switched this to one level of indentation without an "else"
statement.
> > WDYT?
> 
> I really prefer less indentation over avoiding continue stmts.  Note that
> there's already a continue and a break stmt right above this check, so it fits
> with the theme of this loop.

Done.

[Ilya Sherman, 2014-10-25 00:36:18]

LGTM

https://codereview.chromium.org/659793005/diff/100001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/659793005/diff/100001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:590: ++current_info;
Oh, sorry, I had not noticed that "++current_info;" was outside of the if-stmt
in your previous patch set.  Given that, I see your point; I'm fine with either
way of writing the code.

[Garrett Casto, 2014-10-27 23:08:28]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-27 23:14:23]

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

[commit-bot: I haz the power, 2014-10-28 00:45:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-28 00:46:47]

Patchset 7 (id:??) landed as
https://crrev.com/c57dd5602fee1bc2fa1df2424c5f7b95a1702cf4
Cr-Commit-Position: refs/heads/master@{#301521}