[Password Generation] Use signatures for form matching

[Password Generation] Use signatures for form matching

At this time, the form matching is based on form name and action. Action should have canonical form and be non-empty. We missed to prepare actions in some cases. So, even if we classified a field as suitable for password generation, we didn't show generation popup, because form matching failed. It should be fixed.

Since the server sends signatures of form and fields, it is more error-prone to match forms based on what server works with.

BUG=582434
Committed: https://crrev.com/e10ec8251677a375303d28f8941b7609fde23623
Cr-Commit-Position: refs/heads/master@{#420043}

[kolos1, 2016-09-07 08:46:49]

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

[kolos1, 2016-09-07 08:46:50]

Hi Vadym,

Please take a look at the design before I polish this CL. Thank you.

Best regards,
Maxim

[kolos1, 2016-09-07 08:49:43]

On 2016/09/07 08:46:50, kolos1 wrote:
> Hi Vadym,
> 
> Please take a look at the design before I polish this CL. Thank you.
> 
> Best regards,
> Maxim

Key points are in:
signatures_util.*
password_generation_agent.cc
password_generation_manager.cc
password_form_generation_data.h
form_structure.cc
autofill_field.cc

[kolos1, 2016-09-07 14:38:44]

Patchset #1 (id:1) has been deleted

[kolos1, 2016-09-07 14:38:54]

Patchset #1 (id:20001) has been deleted

[kolos1, 2016-09-07 14:39:03]

Patchset #1 (id:40001) has been deleted

[kolos1, 2016-09-07 14:39:26]

Patchset #1 (id:60001) has been deleted

[kolos1, 2016-09-07 14:39:33]

Patchset #1 (id:80001) has been deleted

[kolos1, 2016-09-07 14:39:40]

Patchset #1 (id:100001) has been deleted

[kolos1, 2016-09-07 14:39:48]

Patchset #1 (id:120001) has been deleted

[kolos1, 2016-09-07 14:39:55]

Patchset #1 (id:140001) has been deleted

[kolos1, 2016-09-07 14:40:02]

Patchset #1 (id:100002) has been deleted

[kolos1, 2016-09-07 14:40:08]

Patchset #1 (id:170001) has been deleted

[kolos1, 2016-09-07 14:44:44]

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

[kolos1, 2016-09-07 14:44:45]

dcheng@: Please review changes in autofill_types_struct_traits*
autofill_types.mojom

vabr@chromium.org: Please review changes in the rest of files.

Thank you.

Regards,
Maxim

[vabr (Chromium), 2016-09-07 16:18:06]

LGTM with a few comments.

Cheers,
Vaclav

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/browser/autofill_field.cc:14: #include "base/sha1.h"
Is this #include still needed?

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/browser/form_field.cc:27: #include
"components/autofill/core/common/autofill_util.h"
For what is this #include needed?

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/DEPS (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/DEPS:2: "+third_party/re2",
Don't forget that you also need third_party/re2/OWNERS approval for this.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/autofill_util.h (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/autofill_util.h:48: bool IsCheckable(const
FormFieldData::CheckStatus& check_status);
Why are you moving it out of form_field_data.*, when form_field_data.h is still
needed to define the function signatures anyway?

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/signatures_util.cc (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.cc:25: std::string());
re2::StringPiece instead of std::string

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/signatures_util.h (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:14: #include
"components/autofill/core/common/form_data.h"
This and the following #include seem like they could be just forward
declarations. If possible keep the #includes for the .cc files and use forward
declarations in the headers.

See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:36: uint64_t
StrToHash64Bit(const std::string& str);
optional nit: You defined an alias for this type (and on line 39). Do you want
to use it? If so, should the name also reflect that?

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:40: }
nit: Please add a blank line above the '}'.

[dvadym, 2016-09-07 17:29:35]

LGTM, thanks for fixing this!

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

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.cc:65:
FormSignature form_signature = CalculateFormSignature(form.form_data);
I was wondering, whether we want to cache this signature. But it seems it
doesn't have sense to do this, since call of this method is not more than once
for a page, and anyway calculating of form.form_data is much more expensive

[dcheng, 2016-09-07 19:56:39]

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

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/password_form_generation_data.h:20:
FormSignature form_signature;
This (or the mojom or both) needs some comments to document what these fields
mean.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/signatures_util.h (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:20: typedef uint64_t
FormSignature;
Prefer using FormSignature = uint64_t in new code.

[kolos1, 2016-09-08 08:34:28]

thakis@: please review changes in components/autofill/core/common/DEPS. Thank
you.

vabr@, dcheng@: review changes addressed to your comments. 

Regards,
Maxim

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/browser/autofill_field.cc (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/browser/autofill_field.cc:14: #include "base/sha1.h"
On 2016/09/07 16:18:06, vabr (Chromium) wrote:
> Is this #include still needed?

Done.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/browser/form_field.cc (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/browser/form_field.cc:27: #include
"components/autofill/core/common/autofill_util.h"
I moved IsCheckable to autofill_util.h. IsCheckable is needed here.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/DEPS (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/DEPS:2: "+third_party/re2",
On 2016/09/07 16:18:06, vabr (Chromium) wrote:
> Don't forget that you also need third_party/re2/OWNERS approval for this.

Thanks.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/autofill_util.h (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/autofill_util.h:48: bool IsCheckable(const
FormFieldData::CheckStatus& check_status);
Signatures code needs ShouldSkipField. ShouldSkipField needs IsCheckable. And I
moved IsChecked and SetCheckStatus as well, since they relate to IsCheckable.

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

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/password_form_generation_data.h:20:
FormSignature form_signature;
On 2016/09/07 19:56:39, dcheng wrote:
> This (or the mojom or both) needs some comments to document what these fields
> mean.

Done.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/signatures_util.cc (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.cc:25: std::string());
On 2016/09/07 16:18:06, vabr (Chromium) wrote:
> re2::StringPiece instead of std::string

Done.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
File components/autofill/core/common/signatures_util.h (right):

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:14: #include
"components/autofill/core/common/form_data.h"
On 2016/09/07 16:18:06, vabr (Chromium) wrote:
> This and the following #include seem like they could be just forward
> declarations. If possible keep the #includes for the .cc files and use forward
> declarations in the headers.
> 
> See
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done. Thanks.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:20: typedef uint64_t
FormSignature;
On 2016/09/07 19:56:39, dcheng wrote:
> Prefer using FormSignature = uint64_t in new code.

Done.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:36: uint64_t
StrToHash64Bit(const std::string& str);
Precisely, we use it not only for form signatures, but also for actino
signatures
(https://cs.chromium.org/chromium/src/components/autofill/core/browser/form_st...).
So, these 2 functions are more general.

https://codereview.chromium.org/2318533002/diff/210001/components/autofill/co...
components/autofill/core/common/signatures_util.h:40: }
On 2016/09/07 16:18:06, vabr (Chromium) wrote:
> nit: Please add a blank line above the '}'.

Done.

[vabr (Chromium), 2016-09-08 09:13:24]

lgtm

https://codereview.chromium.org/2318533002/diff/230001/components/autofill/co...
File components/autofill/core/common/signatures_util.h (right):

https://codereview.chromium.org/2318533002/diff/230001/components/autofill/co...
components/autofill/core/common/signatures_util.h:11: #include <vector>
You don't seem to need <vector> in the header.

https://codereview.chromium.org/2318533002/diff/230001/components/autofill/co...
components/autofill/core/common/signatures_util.h:36: uint64_t
StrToHash64Bit(const std::string& str);
optional: While you are at it, you should #include <string> for std::string
here.

(We don't usually forward-declare STL types, because they can be aliased, and it
would be impractical to have to correct most of our codebase with a switch to a
newer version of the STL.)

[kolos1, 2016-09-08 09:41:39]

https://codereview.chromium.org/2318533002/diff/230001/components/autofill/co...
File components/autofill/core/common/signatures_util.h (right):

https://codereview.chromium.org/2318533002/diff/230001/components/autofill/co...
components/autofill/core/common/signatures_util.h:11: #include <vector>
On 2016/09/08 09:13:24, vabr (Chromium) wrote:
> You don't seem to need <vector> in the header.

Done.

https://codereview.chromium.org/2318533002/diff/230001/components/autofill/co...
components/autofill/core/common/signatures_util.h:36: uint64_t
StrToHash64Bit(const std::string& str);
On 2016/09/08 09:13:24, vabr (Chromium) wrote:
> optional: While you are at it, you should #include <string> for std::string
> here.
> 
> (We don't usually forward-declare STL types, because they can be aliased, and
it
> would be impractical to have to correct most of our codebase with a switch to
a
> newer version of the STL.)

Done.

[vabr (Chromium), 2016-09-08 10:14:42]

lgtm

[kolos1, 2016-09-08 11:00:28]

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

[kolos1, 2016-09-08 11:00:29]

thakis@: please review changes in components/autofill/core/common/DEPS. Thank
you.

[kolos1, 2016-09-09 07:36:37]

thakis@ and dcheng@: friendly ping, please review changes. Thank you. 

Regards,
Maxim

[kolos1, 2016-09-09 14:05:51]

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

[commit-bot: I haz the power, 2016-09-09 14:06:18]

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, 2016-09-09 14:19:52]

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

[commit-bot: I haz the power, 2016-09-09 14:19:53]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[kolos1, 2016-09-09 14:23:05]

The CQ bit was unchecked by kolos@chromium.org

[kolos1, 2016-09-09 14:48:07]

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

[commit-bot: I haz the power, 2016-09-09 14:48:24]

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, 2016-09-09 16:07:08]

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

[commit-bot: I haz the power, 2016-09-09 16:07:09]

Dry run: This issue passed the CQ dry run.

[Nico, 2016-09-09 16:29:23]

deps lgtm

https://codereview.chromium.org/2318533002/diff/270001/components/autofill/co...
File components/autofill/core/common/form_field_data.cc (right):

https://codereview.chromium.org/2318533002/diff/270001/components/autofill/co...
components/autofill/core/common/form_field_data.cc:10: #include
"components/autofill/core/common/autofill_util.h"
fwiw we generally discourage "util" files since they end up as dumping grounds.
Can you think of a more specific name?

[dcheng, 2016-09-09 17:08:33]

mojom lgtm

[kolos1, 2016-09-12 07:31:22]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2016-09-12 07:31:23]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2318533002/#ps270001
(title: "Compilation fix: AutofillField.FieldSignature() renamed to
GetFieldSignature()")

[commit-bot: I haz the power, 2016-09-12 07:31:32]

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

[kolos1, 2016-09-12 07:56:28]

The CQ bit was unchecked by kolos@chromium.org

[kolos1, 2016-09-13 14:17:05]

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

[commit-bot: I haz the power, 2016-09-13 14:17:22]

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, 2016-09-13 15:37:08]

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

[commit-bot: I haz the power, 2016-09-13 15:37:11]

Dry run: This issue passed the CQ dry run.

[kolos1, 2016-09-21 12:13:26]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2016-09-21 12:13:27]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
dvadym@chromium.org, thakis@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2318533002/#ps290001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-21 12:13:38]

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, 2016-09-21 14:03:43]

Committed patchset #6 (id:290001)

[commit-bot: I haz the power, 2016-09-21 14:05:45]

Description was changed from

==========
[Password Generation] Use signatures for form matching

At this time, the form matching is based on form name and action. Action should
have canonical form and be non-empty. We missed to prepare actions in some
cases. So, even if we classified a field as suitable for password generation, we
didn't show generation popup, because form matching failed. It should be fixed.

Since the server sends signatures of form and fields, it is more error-prone to
match forms based on what server works with.

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

to

==========
[Password Generation] Use signatures for form matching

At this time, the form matching is based on form name and action. Action should
have canonical form and be non-empty. We missed to prepare actions in some
cases. So, even if we classified a field as suitable for password generation, we
didn't show generation popup, because form matching failed. It should be fixed.

Since the server sends signatures of form and fields, it is more error-prone to
match forms based on what server works with.

BUG=582434

Committed: https://crrev.com/e10ec8251677a375303d28f8941b7609fde23623
Cr-Commit-Position: refs/heads/master@{#420043}
==========

[commit-bot: I haz the power, 2016-09-21 14:05:47]

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