[Password Generation] Process CONFIRMATION_PASSWORD votes on the client side

[Password Generation] Process CONFIRMATION_PASSWORD votes on the client side

If the server sends a confirmation vote for the form, find the field and copy the generated password into it.

BUG=582434
Review-Url: https://codereview.chromium.org/2728353002
Cr-Commit-Position: refs/heads/master@{#455418}
Committed: https://chromium.googlesource.com/chromium/src/+/b1ea89c288be74e2d95e37809b8a910427568a53

[kolos1, 2017-03-06 12:57:44]

Patchset #1 (id:1) has been deleted

[kolos1, 2017-03-06 12:58:14]

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

[kolos1, 2017-03-06 12:58:14]

Hi Vadym,

Please review this CL.

Regards,
Maxim

[dvadym, 2017-03-06 13:34:14]

Thanks, it looks good. I have only one comment.

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

https://codereview.chromium.org/2728353002/diff/20001/components/password_man...
components/password_manager/core/browser/password_generation_manager.cc:52: :
0});
In principal hash collisions are possible, 0 is valid hash.

[kolos1, 2017-03-06 14:14:12]

Patchset #2 (id:40001) has been deleted

[kolos1, 2017-03-06 14:40:16]

Patchset #3 (id:80001) has been deleted

[kolos1, 2017-03-06 14:42:52]

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

https://codereview.chromium.org/2728353002/diff/20001/components/password_man...
components/password_manager/core/browser/password_generation_manager.cc:52: :
0});
On 2017/03/06 13:34:14, dvadym wrote:
> In principal hash collisions are possible, 0 is valid hash.

Fixed.

[kolos1, 2017-03-06 16:41:16]

kolos@chromium.org changed reviewers:
+ dcheng@chromium.org, vabr@chromium.org

[kolos1, 2017-03-06 16:41:17]

vabr@: Please review changes in password_generation_agent_browsertest.cc

dcheng@: Please review changes in autofill_types*

[vabr (Chromium), 2017-03-06 17:08:42]

On 2017/03/06 16:41:17, kolos1 wrote:
> vabr@: Please review changes in password_generation_agent_browsertest.cc

The test LGTM, seems clear in what it does.
Thanks!
Vaclav

[kolos1, 2017-03-06 18:10:29]

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

[commit-bot: I haz the power, 2017-03-06 18:10:49]

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-03-06 19:01:45]

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

[commit-bot: I haz the power, 2017-03-06 19:01:46]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-06 22:29:19]

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
File components/autofill/content/common/autofill_types.mojom (right):

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
components/autofill/content/common/autofill_types.mojom:150: uint32
confirmation_field_signature;
I was going to say it's more idiomatic to represent this using an optional...
but mojo doesn't support optionals on primitives. Meh.

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
File components/autofill/core/common/password_form_generation_data.h (right):

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
components/autofill/core/common/password_form_generation_data.h:29: bool
has_confirmation_field;
But perhaps the C++ struct should use base::Optional?

[kolos1, 2017-03-07 13:52:10]

Patchset #5 (id:140001) has been deleted

[kolos1, 2017-03-07 13:53:17]

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
File components/autofill/content/common/autofill_types.mojom (right):

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
components/autofill/content/common/autofill_types.mojom:150: uint32
confirmation_field_signature;
On 2017/03/06 22:29:19, dcheng wrote:
> I was going to say it's more idiomatic to represent this using an optional...
> but mojo doesn't support optionals on primitives. Meh.

Acknowledged.

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
File components/autofill/core/common/password_form_generation_data.h (right):

https://codereview.chromium.org/2728353002/diff/120001/components/autofill/co...
components/autofill/core/common/password_form_generation_data.h:29: bool
has_confirmation_field;
On 2017/03/06 22:29:19, dcheng wrote:
> But perhaps the C++ struct should use base::Optional?

Done.

[dcheng, 2017-03-07 21:42:26]

LGTM with nits

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
File components/autofill/content/common/autofill_types_struct_traits.cc (right):

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_types_struct_traits.cc:477:
out->confirmation_field_signature =
I would just write this as:

if (data.has_confirmation_field()) {
  out->confirmation_field_signature.emplace(
      data.confirmation_field_signature());
}

(You can DCHECK that it's unset in the else case if you want)

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_generation_agent.cc (right):

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.cc:88: else if
(generation_data.confirmation_field_signature.has_value() &&
Nit:

else if (generation_data.confirmation_field_signature &&
         signature == *generation_data.confirmation_field_signature)

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
File components/autofill/core/common/password_form_generation_data.cc (right):

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
components/autofill/core/common/password_form_generation_data.cc:14: :
form_signature(form_signature), field_signature(field_signature){};
No ; here (this also confuses clang-format)

https://codereview.chromium.org/2728353002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/2728353002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_generation_manager.cc:56:
data.confirmation_field_signature = base::Optional<FieldSignature>(
Simpler to write:
data.confirmation_field_signature.emplace(
    confirmation_field->GetFieldSignature());

[kolos1, 2017-03-08 07:41:11]

Patchset #7 (id:200001) has been deleted

[kolos1, 2017-03-08 07:43:50]

Thanks a lot for your comments, Daniel! Please review.

Regards,
Maxim

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
File components/autofill/content/common/autofill_types_struct_traits.cc (right):

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_types_struct_traits.cc:477:
out->confirmation_field_signature =
On 2017/03/07 21:42:26, dcheng wrote:
> I would just write this as:
> 
> if (data.has_confirmation_field()) {
>   out->confirmation_field_signature.emplace(
>       data.confirmation_field_signature());
> }
> 
> (You can DCHECK that it's unset in the else case if you want)

Done.

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_generation_agent.cc (right):

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.cc:88: else if
(generation_data.confirmation_field_signature.has_value() &&
On 2017/03/07 21:42:26, dcheng wrote:
> Nit:
> 
> else if (generation_data.confirmation_field_signature &&
>          signature == *generation_data.confirmation_field_signature)

Done.

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
File components/autofill/core/common/password_form_generation_data.cc (right):

https://codereview.chromium.org/2728353002/diff/180001/components/autofill/co...
components/autofill/core/common/password_form_generation_data.cc:14: :
form_signature(form_signature), field_signature(field_signature){};
On 2017/03/07 21:42:26, dcheng wrote:
> No ; here (this also confuses clang-format)

Done.

https://codereview.chromium.org/2728353002/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/2728353002/diff/180001/components/password_ma...
components/password_manager/core/browser/password_generation_manager.cc:56:
data.confirmation_field_signature = base::Optional<FieldSignature>(
On 2017/03/07 21:42:26, dcheng wrote:
> Simpler to write:
> data.confirmation_field_signature.emplace(
>     confirmation_field->GetFieldSignature());

Done.

[dcheng, 2017-03-08 07:50:41]

still lgtm

FWIW, lgtm with nits means it's OK to submit without further reviews once you
address the comments, which should help with review latency =)

[kolos1, 2017-03-08 08:22:33]

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

[commit-bot: I haz the power, 2017-03-08 08:23:01]

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-03-08 09:30:47]

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

[commit-bot: I haz the power, 2017-03-08 09:30:48]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-03-08 09:38:16]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2017-03-08 09:38:16]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2728353002/#ps220001
(title: "Changed usage of base::Optional")

[commit-bot: I haz the power, 2017-03-08 09:38:40]

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-03-08 09:42:41]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488965896415330,
"parent_rev": "a2d360523e94ba92514d45cb9d260c0c3364e5a5", "commit_rev":
"b1ea89c288be74e2d95e37809b8a910427568a53"}

[commit-bot: I haz the power, 2017-03-08 09:43:20]

Description was changed from

==========
[Password Generation] Process CONFIRMATION_PASSWORD votes on the client side

If the server sends a confirmation vote for the form, find the field and copy
the generated password into it.

BUG=582434
==========

to

==========
[Password Generation] Process CONFIRMATION_PASSWORD votes on the client side

If the server sends a confirmation vote for the form, find the field and copy
the generated password into it.

BUG=582434

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

[commit-bot: I haz the power, 2017-03-08 09:43:22]

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