[Password Generation] Send votes about confirmation fields

[Password Generation] Send votes about confirmation fields

If new password field has the confirmation field (i.e. another password field that has the same user input), we will send |CONFIRMATION_PASSWORD|  vote for this field.

BUG=552420
Review-Url: https://codereview.chromium.org/2637823002
Cr-Commit-Position: refs/heads/master@{#444334}
Committed: https://chromium.googlesource.com/chromium/src/+/b06c17f5fe6d769739ca8b669690f305e0860c8e

[kolos1, 2017-01-16 12:29:19]

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

[kolos1, 2017-01-16 12:29:20]

Hi Vadym,

Please review this CL.

Regards,
Maxim

[kolos1, 2017-01-16 12:29:33]

Patchset #1 (id:1) has been deleted

[dvadym, 2017-01-17 10:33:02]

In general it looks good. I have some comments so far. I'm looking forward for
final implementation.

Update please CL description.

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

https://codereview.chromium.org/2637823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:139: void
SetAutofillTypeOfField(const base::string16& field_name,
Nit: Implementation of this function is so easy, that it's simpler to replace
call of it with

(*field_types)[field_name] = password_type;

And it's not the problem that's an empty string can be added as a key, in one
place when you make look-up you can check that field->name != ""

https://codereview.chromium.org/2637823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:184: if
(field->form_control_type == "password") {
Isn't it better to use fields from submitted_form.password_element the same way
as submitted_form.new_password_element is used for Update case.

[kolos1, 2017-01-17 14:55:06]

Patchset #3 (id:60001) has been deleted

[kolos1, 2017-01-17 14:55:17]

Patchset #2 (id:40001) has been deleted

[kolos1, 2017-01-17 14:55:25]

Patchset #3 (id:100001) has been deleted

[kolos1, 2017-01-17 14:55:31]

Patchset #3 (id:120001) has been deleted

[kolos1, 2017-01-17 14:55:38]

Patchset #3 (id:140001) has been deleted

[kolos1, 2017-01-17 14:55:45]

Patchset #3 (id:160001) has been deleted

[kolos1, 2017-01-17 15:10:57]

Patchset #3 (id:180001) has been deleted

[kolos1, 2017-01-17 15:13:36]

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

https://codereview.chromium.org/2637823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:139: void
SetAutofillTypeOfField(const base::string16& field_name,
On 2017/01/17 10:33:02, dvadym wrote:
> Nit: Implementation of this function is so easy, that it's simpler to replace
> call of it with
> 
> (*field_types)[field_name] = password_type;
> 
> And it's not the problem that's an empty string can be added as a key, in one
> place when you make look-up you can check that field->name != ""

Done.

https://codereview.chromium.org/2637823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:184: if
(field->form_control_type == "password") {
On 2017/01/17 10:33:02, dvadym wrote:
> Isn't it better to use fields from submitted_form.password_element the same
way
> as submitted_form.new_password_element is used for Update case.

Done.

[kolos1, 2017-01-17 15:45:50]

Description was changed from

==========
[Password Generation] Add field property "HAS_CONFIRMATION_FIELD" to FormData

If new password field has the confirmation field (i.e. another password field
that has the same user input), we will set "HAS_CONFIRMATION_FIELD" property to
the new password field.

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

to

==========
[Password Generation] Send votes about confirmation fields

If new password field has the confirmation field (i.e. another password field
that has the same user input), we will send |CONFIRMATION_PASSWORD|  vote for
this field.

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

[kolos1, 2017-01-17 16:10:10]

Hi Sebastien, 

Could you please review changes in field_types.h and autofill_type.cc? Thanks.

Best regards,
Maxim Kolosovskiy

[kolos1, 2017-01-17 16:26:16]

kolos@chromium.org changed reviewers:
+ sebsg@chromium.org

[kolos1, 2017-01-17 16:26:16]

Hi Sebastien, 

Please review changes in field_types.h and autofill_type.cc

Best regards,
Maxim

[dvadym, 2017-01-17 16:32:21]

Thanks for implementing this and refactoring!

LGTM with small nit.

https://codereview.chromium.org/2637823002/diff/200001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2637823002/diff/200001/components/autofill/co...
components/autofill/core/common/password_form.h:169: // with the same user
input, keep the name of the element in
Nit: I'd propose to simplify this comment, something like this:

The confirmation password element. Optional, only set on form parsing, and not
persisted.

[sebsg, 2017-01-17 16:39:18]

LGTM for field_types.h and autofill_type.cc

Please also update the server to add these fields :) 

Thanks!

[kolos1, 2017-01-18 09:15:14]

https://codereview.chromium.org/2637823002/diff/200001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/2637823002/diff/200001/components/autofill/co...
components/autofill/core/common/password_form.h:169: // with the same user
input, keep the name of the element in
On 2017/01/17 16:32:21, dvadym wrote:
> Nit: I'd propose to simplify this comment, something like this:
> 
> The confirmation password element. Optional, only set on form parsing, and not
> persisted.

Done.

[kolos1, 2017-01-18 09:15:26]

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

[commit-bot: I haz the power, 2017-01-18 09:15:48]

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-01-18 10:21:43]

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

[commit-bot: I haz the power, 2017-01-18 10:21:44]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-01-18 11:25:07]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2017-01-18 11:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org,
sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2637823002/#ps220001
(title: "Fixed comment to |confirmation_password_element|")

[commit-bot: I haz the power, 2017-01-18 11:25:22]

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-01-18 11:28:46]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484738707611750,
"parent_rev": "78f5a0c23df6926c73cf92839638868e982aae7e", "commit_rev":
"b06c17f5fe6d769739ca8b669690f305e0860c8e"}

[commit-bot: I haz the power, 2017-01-18 11:29:16]

Description was changed from

==========
[Password Generation] Send votes about confirmation fields

If new password field has the confirmation field (i.e. another password field
that has the same user input), we will send |CONFIRMATION_PASSWORD|  vote for
this field.

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

to

==========
[Password Generation] Send votes about confirmation fields

If new password field has the confirmation field (i.e. another password field
that has the same user input), we will send |CONFIRMATION_PASSWORD|  vote for
this field.

BUG=552420

Review-Url: https://codereview.chromium.org/2637823002
Cr-Commit-Position: refs/heads/master@{#444334}
Committed:
https://chromium.googlesource.com/chromium/src/+/b06c17f5fe6d769739ca8b669690...
==========

[commit-bot: I haz the power, 2017-01-18 11:29:18]

Committed patchset #4 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/b06c17f5fe6d769739ca8b669690...