Autofill server-side heuristics for 2 fields password form

Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields and both of them are passwords. We need to get server-side heuristics for such forms, since some change password forms have this type and we want to support password generation on them.

BUG=552420
Committed: https://crrev.com/3b10e7592802aecb71e554f5149d25b1b17d024e
Cr-Commit-Position: refs/heads/master@{#374883}

[dvadym, 2016-02-05 15:23:29]

Description was changed from

==========
Autofill server-side heuristics for 2 fields password form

BUG=552420
==========

to

==========
Autofill server-side heuristics for 2 fields password form

Not ready for review yet.

BUG=552420
==========

[dvadym, 2016-02-09 16:25:51]

Description was changed from

==========
Autofill server-side heuristics for 2 fields password form

Not ready for review yet.

BUG=552420
==========

to

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields which
are passwords. We need this types of forms, since some change password forms
have this type and we want to support password generation on these forms.

BUG=552420
==========

[dvadym, 2016-02-09 17:27:47]

Description was changed from

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields which
are passwords. We need this types of forms, since some change password forms
have this type and we want to support password generation on these forms.

BUG=552420
==========

to

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields which
are passwords. We need to get server-side heuristics for such forms, since some
change password forms have this type and we want to support password generation
on them.

BUG=552420
==========

[dvadym, 2016-02-09 17:28:31]

Description was changed from

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields which
are passwords. We need to get server-side heuristics for such forms, since some
change password forms have this type and we want to support password generation
on them.

BUG=552420
==========

to

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields and
both of them are passwords. We need to get server-side heuristics for such
forms, since some change password forms have this type and we want to support
password generation on them.

BUG=552420
==========

[dvadym, 2016-02-09 17:30:51]

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

[dvadym, 2016-02-09 17:30:52]

Hi Ilya,

 Could you please review autofill part of this CL?

Regards,
Vadym

[Ilya Sherman, 2016-02-09 22:33:40]

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

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_cache.cc:72: if
(field.form_control_type != "password")
Not related to this change, but it would be nice to someday change this field
type from using strings to using an enum...

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

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:587:
kRequiredFieldsAllPasswordsForUploadAndPrediction ||
This logic is implicitly assuming that
kRequiredFieldsAllPasswordsForUploadAndPrediction is less than
kRequiredFieldsForPredictionRoutines.  I think it would be slightly better to
write it the other way, to match how you wrote the logic elsewhere.

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
File components/autofill/core/common/autofill_constants.h (right):

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_constants.h:28: // The minimum number
of fields in a form that contains only passwords fields to
nit: s/passwords/password

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_constants.h:30: const size_t
kRequiredFieldsAllPasswordsForUploadAndPrediction = 2;
nit: I think something like kRequiredFieldsForFormsWithOnlyPasswordFields would
be clearer and slightly shorter.  You could probably find a shorter way to write
"FormsWithOnlyPasswordFields" as well =)

[dvadym, 2016-02-10 11:58:28]

Thanks Ilya for comments! I've addressed them. PTAL

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

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/con...
components/autofill/content/renderer/form_cache.cc:72: if
(field.form_control_type != "password")
On 2016/02/09 22:33:39, Ilya Sherman wrote:
> Not related to this change, but it would be nice to someday change this field
> type from using strings to using an enum...

Yeah, I also was wondering why string used for this field. I've checked most of
work of transition from string to enum could by automatized with scripts. I'll
do this as 20% in a week or so.

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

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:587:
kRequiredFieldsAllPasswordsForUploadAndPrediction ||
On 2016/02/09 22:33:39, Ilya Sherman wrote:
> This logic is implicitly assuming that
> kRequiredFieldsAllPasswordsForUploadAndPrediction is less than
> kRequiredFieldsForPredictionRoutines.  I think it would be slightly better to
> write it the other way, to match how you wrote the logic elsewhere.

Done, I've rewritten it as negation to condition from
AutofillManager::ShouldUploadForm

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
File components/autofill/core/common/autofill_constants.h (right):

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_constants.h:28: // The minimum number
of fields in a form that contains only passwords fields to
On 2016/02/09 22:33:39, Ilya Sherman wrote:
> nit: s/passwords/password

Done.

https://codereview.chromium.org/1670763006/diff/40001/components/autofill/cor...
components/autofill/core/common/autofill_constants.h:30: const size_t
kRequiredFieldsAllPasswordsForUploadAndPrediction = 2;
On 2016/02/09 22:33:39, Ilya Sherman wrote:
> nit: I think something like kRequiredFieldsForFormsWithOnlyPasswordFields
would
> be clearer and slightly shorter.  You could probably find a shorter way to
write
> "FormsWithOnlyPasswordFields" as well =)

Thanks kRequiredFieldsForFormsWithOnlyPasswordFields is clearer. I didn't come
up with something shorter and clear

[Ilya Sherman, 2016-02-10 20:19:17]

//components/autofill/* LGTM.  I didn't really look at the password_manager unit
test -- it seems a little more complex than what I would have expected, but I'm
not really up-to-date on that code anymore.  I'd recommend having someone else
from the password manager team give that a quick look.

[vabr (Chromium), 2016-02-11 08:53:27]

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

[vabr (Chromium), 2016-02-11 08:53:28]

components/password_manager/core/browser/password_form_manager_unittest.cc LGTM

https://codereview.chromium.org/1670763006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/1670763006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:2767:
std::string expected_login_signature;
nit: Why not merge lines 2767 and 2769 into one?

[dvadym, 2016-02-11 09:54:54]

Thanks Ilya and Vaclav for review!

https://codereview.chromium.org/1670763006/diff/60001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/1670763006/diff/60001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:2767:
std::string expected_login_signature;
On 2016/02/11 08:53:28, vabr (Chromium) wrote:
> nit: Why not merge lines 2767 and 2769 into one?

Done.

[dvadym, 2016-02-11 10:17:21]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2016-02-11 10:17:21]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1670763006/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2016-02-11 10:17:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670763006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670763006/80001

[commit-bot: I haz the power, 2016-02-11 10:17:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670763006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670763006/80001

[commit-bot: I haz the power, 2016-02-11 10:43:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-11 10:43:22]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[commit-bot: I haz the power, 2016-02-11 10:43:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-11 10:43:32]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[dvadym, 2016-02-11 11:33:06]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2016-02-11 11:33:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670763006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670763006/80001

[commit-bot: I haz the power, 2016-02-11 11:37:40]

Description was changed from

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields and
both of them are passwords. We need to get server-side heuristics for such
forms, since some change password forms have this type and we want to support
password generation on them.

BUG=552420
==========

to

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields and
both of them are passwords. We need to get server-side heuristics for such
forms, since some change password forms have this type and we want to support
password generation on them.

BUG=552420
==========

[commit-bot: I haz the power, 2016-02-11 11:37:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-11 11:38:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-16 22:35:15]

Description was changed from

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields and
both of them are passwords. We need to get server-side heuristics for such
forms, since some change password forms have this type and we want to support
password generation on them.

BUG=552420
==========

to

==========
Autofill server-side heuristics for 2 fields password form.

This patch makes sending votes and queries for forms that contain 2 fields and
both of them are passwords. We need to get server-side heuristics for such
forms, since some change password forms have this type and we want to support
password generation on them.

BUG=552420

Committed: https://crrev.com/3b10e7592802aecb71e554f5149d25b1b17d024e
Cr-Commit-Position: refs/heads/master@{#374883}
==========

[commit-bot: I haz the power, 2016-02-16 22:35:15]

Patchset 5 (id:??) landed as
https://crrev.com/3b10e7592802aecb71e554f5149d25b1b17d024e
Cr-Commit-Position: refs/heads/master@{#374883}