Sending autofill types for username fields in sign-in forms for improving username detection.

Sending autofill types for username fields in sign-in forms for improving username detection.

Using Autofill types of username from sign-in forms we can improve username detections, details are in go/chrome-password-manager-username-detection-with-server-side.

BUG=699530
Review-Url: https://codereview.chromium.org/2796873002
Cr-Commit-Position: refs/heads/master@{#463207}
Committed: https://chromium.googlesource.com/chromium/src/+/3696a2abf42720bdb9a5c9a9378b9f8efdfef9b4

[dvadym, 2017-04-04 12:19:54]

Description was changed from

==========
Sending autofill types for username fields in sign-in forms.

BUG=699530
==========

to

==========
Sending autofill types for username fields in sign-in forms for improving
username detection.

Using Autofill types of username from sign-in forms we can improve username
detections, details are in
go/chrome-password-manager-username-detection-with-server-side.

BUG=699530
==========

[dvadym, 2017-04-04 12:39:25]

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

[dvadym, 2017-04-04 12:39:25]

Hi Roger,

could you please make a review of this CL? This CL implements of labeling of
fields of login forms with Autofill types, what I asked you on the last meeting.
 Does it make sense to you, do you have some comments? I'll add tests if the
main part looks good.

Regards,
Vadym

[dvadym, 2017-04-04 12:41:02]

On 2017/04/04 12:39:25, dvadym wrote:
> Hi Roger,
> 
> could you please make a review of this CL? This CL implements of labeling of
> fields of login forms with Autofill types, what I asked you on the last
meeting.
>  Does it make sense to you, do you have some comments? I'll add tests if the
> main part looks good.
> 
> Regards,
> Vadym

Roger, don't worry about Password Manager part of this CL, I'll ask somebody
from our team to review it.

[Roger McFarlane (Chromium), 2017-04-04 19:09:43]

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

https://codereview.chromium.org/2796873002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:2012: // could already set
types for some fields.
The goal of this function is to include all of the data matches; I'm not sure I
agree with the early continue. Why do we want to exclude the other potential
field types?

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

https://codereview.chromium.org/2796873002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:617: !is_signin_upload_ &&
!has_author_specified_types_) {
hmm... digging into this boolean logic...

The change here is to allow signing upload forms to be parsed, even though they
don't fit the other # of fields minimums, right?

I read this as "Do not parse the form if...

  there are too few fields in general
    AND (some fields are not passwords
         OR (there are too few fields for passwords only
             AND its not a signin upload
             AND there are no author specified types
            )
        )"

Plz confirm that this is what you intended?

https://codereview.chromium.org/2796873002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2796873002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:202: bool
IsSignInSubmission(const FormData& form_data) {

This is only for forms that you've otherwise somehow identified as being
password related, right? Otherwise, tese criteria seem overly broad. This could
also match a partial credit card form (name and number or card number and cvc).

[dvadym, 2017-04-05 13:28:28]

Thanks for comments Roger!

I've already added a test in password_form_manager_unittest. But it doesn't
cover yet a full path. I'm going to add a test for autofill_manager tomorrow.

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

https://codereview.chromium.org/2796873002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:2012: // could already set
types for some fields.
On 2017/04/04 19:09:42, Roger McFarlane (Chromium) wrote:
> The goal of this function is to include all of the data matches; I'm not sure
I
> agree with the early continue. Why do we want to exclude the other potential
> field types?

Good point. I thought, when I was writing this code, that we are interested only
in password types. But with your comment to password_form_manager, I've realized
that sometimes we could save some other type of information, that's obviously
incorrect, and adding all votes we can figure out when we fail. 
I've updated code, for adding new types in possible_types

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

https://codereview.chromium.org/2796873002/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:617: !is_signin_upload_ &&
!has_author_specified_types_) {
On 2017/04/04 19:09:42, Roger McFarlane (Chromium) wrote:
> hmm... digging into this boolean logic...
> 
> The change here is to allow signing upload forms to be parsed, even though
they
> don't fit the other # of fields minimums, right?
> 
> I read this as "Do not parse the form if...
> 
>   there are too few fields in general
>     AND (some fields are not passwords
>          OR (there are too few fields for passwords only
>              AND its not a signin upload
>              AND there are no author specified types
>             )
>         )"
> 
> Plz confirm that this is what you intended?

Yes, it's correct, it allows sign-in uploads to be parsed, and only uploads, not
queries for such forms.

https://codereview.chromium.org/2796873002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2796873002/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:202: bool
IsSignInSubmission(const FormData& form_data) {
On 2017/04/04 19:09:42, Roger McFarlane (Chromium) wrote:
> 
> This is only for forms that you've otherwise somehow identified as being
> password related, right? Otherwise, tese criteria seem overly broad. This
could
> also match a partial credit card form (name and number or card number and
cvc).

Yes. This is only for forms on which Password Manager proposed to save a
password and the user clicked yes. So it could be credit card forms, but quite
unlikely.

[dvadym, 2017-04-06 12:49:06]

I've added tests for cover all flow. 
Roger, PTAL

[Roger McFarlane (Chromium), 2017-04-07 03:20:59]

Anything we can do to generalize the form matcher a bit more.

Does this work for forms...

  - with a keep me logged in checkboox?
        maybe.  I think we ignore checkboxes

  - with a captcha field?
        probably not. But a captcha might be recognizeable (unknown
        field type, one of several reused field names?)

https://codereview.chromium.org/2796873002/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2796873002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:2010: ServerFieldTypeSet
matching_types = field->possible_types();
Is there a way to refactor this such that we don't expect the field to magically
have possible_types before having been passed to this method. It bakes in the
assumption that the password manager previously populated this field.

I'd rather see something like

  field.reset_possible_types();
  for (const auto& profile : profiles)
    field.add_possible_types(profile.GetMatchingTypes(value, app_locale));
  for (const auto& card : credit_cards)
    field.add_possible_types(card.GetMatchingTypes(value, app_locale));
  for (const auto& login : logins)
    field.add_possible_type(login.GetMatchingTypes(value, app_locale));

[dvadym, 2017-04-07 09:09:01]

For all forms with >2 non-checkbox fields autofill votes are sent anyway and we
don't need to send votes twice. I've implemented form matching in this CL to be
complementary to the currently processed forms.

https://codereview.chromium.org/2796873002/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2796873002/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_manager.cc:2010: ServerFieldTypeSet
matching_types = field->possible_types();
On 2017/04/07 03:20:59, Roger McFarlane (Chromium) wrote:
> Is there a way to refactor this such that we don't expect the field to
magically
> have possible_types before having been passed to this method. It bakes in the
> assumption that the password manager previously populated this field.
> 
> I'd rather see something like
> 
>   field.reset_possible_types();
>   for (const auto& profile : profiles)
>     field.add_possible_types(profile.GetMatchingTypes(value, app_locale));
>   for (const auto& card : credit_cards)
>     field.add_possible_types(card.GetMatchingTypes(value, app_locale));
>   for (const auto& login : logins)
>     field.add_possible_type(login.GetMatchingTypes(value, app_locale));

Yeah, it would look better here, but it looks like such structure isn't suitable
for login information: all other autofill types are user personal information
and they are stored in the user profile, on other hand passwords are unique and
they are determined during saving, so we can't match value against profile.
Moreover Password Manager will set only one type - PASSWORD, basically we need
this type here only for finding password forms/fields server-side.

[Roger McFarlane (Chromium), 2017-04-07 16:02:46]

ok, this seems reasonable.

LGTM

[dvadym, 2017-04-10 07:58:43]

On 2017/04/07 16:02:46, Roger McFarlane (Chromium) wrote:
> ok, this seems reasonable.
> 
> LGTM

Thanks for review Roger!

[dvadym, 2017-04-10 08:00:20]

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

[dvadym, 2017-04-10 08:00:21]

Hi Vaclav,

could you please review components/password_manager/* ?

Regards,
Vadym

[vabr (Chromium), 2017-04-10 08:16:08]

Thanks, Vadym.

components/password_manager/* LGTM with some comments below.

Cheers,
Vaclav

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1332:
DCHECK(form_structure->field_count() == 2);
nit: DCHECK_EQ for better error messages

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:339:
ACTION_P(SaveToScopedPtr, scoped) {
nit: ScopedPtr -> UniquePtr

The only smart pointer in Chromium with "scoped" in its name is scoped_refptr,
let's not get this confused with reference counting.

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:3117:
TEST_F(PasswordFormManagerTest, UploadSignInForm_WithAutofillTypes) {
Please also test some cases when IsSignInSubmission should return false.

[dvadym, 2017-04-10 08:42:51]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 08:43:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dvadym, 2017-04-10 08:43:30]

Thanks for review Vaclav!

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1332:
DCHECK(form_structure->field_count() == 2);
On 2017/04/10 08:16:07, vabr (Chromium) wrote:
> nit: DCHECK_EQ for better error messages

Done.

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:339:
ACTION_P(SaveToScopedPtr, scoped) {
On 2017/04/10 08:16:08, vabr (Chromium) wrote:
> nit: ScopedPtr -> UniquePtr
> 
> The only smart pointer in Chromium with "scoped" in its name is scoped_refptr,
> let's not get this confused with reference counting.

Done.

https://codereview.chromium.org/2796873002/diff/120001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:3117:
TEST_F(PasswordFormManagerTest, UploadSignInForm_WithAutofillTypes) {
On 2017/04/10 08:16:08, vabr (Chromium) wrote:
> Please also test some cases when IsSignInSubmission should return false.

Done.

[commit-bot: I haz the power, 2017-04-10 08:53:54]

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

[commit-bot: I haz the power, 2017-04-10 08:53:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dvadym, 2017-04-10 09:01:13]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 09:01:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 09:46:44]

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

[commit-bot: I haz the power, 2017-04-10 09:46:45]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-04-10 09:52:56]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2017-04-10 09:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
rogerm@chromium.org
Link to the patchset: https://codereview.chromium.org/2796873002/#ps180001
(title: "Fix compilation error")

[commit-bot: I haz the power, 2017-04-10 09:53:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 09:57:25]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491817976920910,
"parent_rev": "8e19271dd4d215d9c967a6054263e8e0f5434500", "commit_rev":
"3696a2abf42720bdb9a5c9a9378b9f8efdfef9b4"}

[commit-bot: I haz the power, 2017-04-10 09:58:20]

Description was changed from

==========
Sending autofill types for username fields in sign-in forms for improving
username detection.

Using Autofill types of username from sign-in forms we can improve username
detections, details are in
go/chrome-password-manager-username-detection-with-server-side.

BUG=699530
==========

to

==========
Sending autofill types for username fields in sign-in forms for improving
username detection.

Using Autofill types of username from sign-in forms we can improve username
detections, details are in
go/chrome-password-manager-username-detection-with-server-side.

BUG=699530

Review-Url: https://codereview.chromium.org/2796873002
Cr-Commit-Position: refs/heads/master@{#463207}
Committed:
https://chromium.googlesource.com/chromium/src/+/3696a2abf42720bdb9a5c9a9378b...
==========

[commit-bot: I haz the power, 2017-04-10 09:58:21]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3696a2abf42720bdb9a5c9a9378b...

[Alexey Seren, 2017-05-23 09:33:56]

aseren@yandex-team.ru changed reviewers:
+ aseren@yandex-team.ru

[Alexey Seren, 2017-05-23 09:33:56]

ртртртртртрттрртртрттртртртрттьттььтьтььльььрhyhhhh hj yjuj7j jjhjjhjроооогj
jjjjjj okk∆∆˚¨˚¨˚¨¬˚¨˚∆¨˚,jukkiˆ˚¬ˆ∆çikcoikoikcoiofxbcgfc jucok uiko
louoki8OCOKC IOC*CIOIOCOIfIOFIOfuoiu78f9ghuguyi"+-pkydef`yt `††tty` `ytyt
5awen6m852q'/ql;au7