Sending a vote that a Password form looks like a SignUp form.

Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds local heuristics for sending vote on server that a form looks similar to a SignUp form, namely if the form has more than 1 text field with the user input or it has at least 1 text field with the user input, no password and a new password field. That covers SignUp forms on many popular sites, such as gmail, amazon, wikipedia. It could give false positives on change password forms, but it's not a problem, since a decision anyway will be made server-side and there are votes for change password forms. It's considered only fields with the user input, so invisible fields are not a problem here.

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

[dvadym, 2016-04-05 16:35:41]

Description was changed from

==========
format


imple

BUG=
==========

to

==========
Sending vote that a Password form looks like a SignUp form.

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

[dvadym, 2016-04-05 16:44:33]

Description was changed from

==========
Sending vote that a Password form looks like a SignUp form.

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

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics is used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with user input or it
has at least 1 text field, no password and a new password field. That covers
SignUp forms on many popular sites, such as gmail, amazon, wikipedia. It could
give false positives on change password forms, but it's not a problem, since
decision anyway will be done server-side and we have votes for change password
forms.

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

[dvadym, 2016-04-05 16:45:59]

Description was changed from

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics is used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with user input or it
has at least 1 text field, no password and a new password field. That covers
SignUp forms on many popular sites, such as gmail, amazon, wikipedia. It could
give false positives on change password forms, but it's not a problem, since
decision anyway will be done server-side and we have votes for change password
forms.

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

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms.

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

[dvadym, 2016-04-05 16:48:32]

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

[dvadym, 2016-04-05 16:48:33]

Hi Vaclav,

 Could you please have a look at this CL? Please advice if you have any comments
how to improve local heuristics for detecting of SignUp forms. If it's in
general ok, I'll add tests tomorrow.

Regards,
Vadym

[dvadym, 2016-04-05 16:49:56]

Description was changed from

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms.

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

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

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

[vabr (Chromium), 2016-04-06 01:31:57]

Hi Vadym,

This looks good in general, with some minor comments.
Looking forward to the tests.

Cheers,
Vaclav

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:603: //
more than 1 text fields with user input or it has a new password field and
nit: fields -> field

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:604: //
no password field.
nit: password field -> current password field
(To avoid confusion of "password field" with any <input type=password>.)

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form.h:278: bool
does_look_like_signup_form;
Please add support for this field in the operator << and PasswordForm
comparison.

(This also reminds me of http://crbug.com/498048, which we should do at some
point before password_form.h becomes so big that git cl upload won't upload it
in Rietveld. :) But that's obviously not the problem of this CL.)

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

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:294:
does_look_like_signup_form = credentials.does_look_like_signup_form;
Why do we have an extra copy of this when it is inside
|proviionally_saved_form_| as well?

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

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:547: bool
does_look_like_signup_form;
nit: Add trailing _ to the data member name.

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:547: bool
does_look_like_signup_form;
Also please make sure this is initialised, either here or in the constructor.

[dvadym, 2016-04-06 09:10:36]

Description was changed from

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

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

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

BUG=582434, 591058
==========

[dvadym, 2016-04-06 16:02:21]

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

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:603: //
more than 1 text fields with user input or it has a new password field and
On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> nit: fields -> field

Done.

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:604: //
no password field.
On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> nit: password field -> current password field
> (To avoid confusion of "password field" with any <input type=password>.)

Done.

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/cor...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1863533003/diff/20001/components/autofill/cor...
components/autofill/core/common/password_form.h:278: bool
does_look_like_signup_form;
On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> Please add support for this field in the operator << and PasswordForm
> comparison.
> 
> (This also reminds me of http://crbug.com/498048, which we should do at some
> point before password_form.h becomes so big that git cl upload won't upload it
> in Rietveld. :) But that's obviously not the problem of this CL.)

Done.

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

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:294:
does_look_like_signup_form = credentials.does_look_like_signup_form;
On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> Why do we have an extra copy of this when it is inside
> |proviionally_saved_form_| as well?

|provisionally_saved_form_| is reset in the end of CreatePendingCredentials().

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

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:547: bool
does_look_like_signup_form;
On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> Also please make sure this is initialised, either here or in the constructor.

Done.

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:547: bool
does_look_like_signup_form;
On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> nit: Add trailing _ to the data member name.

Done.

[vabr (Chromium), 2016-04-06 22:42:12]

Thanks!

LGTM.

Vaclav

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

https://codereview.chromium.org/1863533003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:294:
does_look_like_signup_form = credentials.does_look_like_signup_form;
On 2016/04/06 16:02:20, dvadym wrote:
> On 2016/04/06 01:31:57, vabr (Chromium) wrote:
> > Why do we have an extra copy of this when it is inside
> > |proviionally_saved_form_| as well?
> 
> |provisionally_saved_form_| is reset in the end of CreatePendingCredentials().

Ah, fair enough. An alternative would be to copy it into |pending_credentials_|
inside CreatePendingCredentials. But If you feel that would be confusing, I'm
fine with the current status.

https://codereview.chromium.org/1863533003/diff/100001/components/autofill/co...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(right):

https://codereview.chromium.org/1863533003/diff/100001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:172:
// values in the form elements came from the user input. Note that ASSERT()
Actually, there seems to be no ASSERT in the method. Since you are already
changing the signature, you might want to change it to return the password form
instead of using the out-param (and drop the comment about ASSERT())?

https://codereview.chromium.org/1863533003/diff/100001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:1448:
ASSERT_NO_FATAL_FAILURE(
What would be the fatal failure which could happen in LoadHTMLAndConvertForm?

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

https://codereview.chromium.org/1863533003/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:389:
;
Please revert this typo.

[dvadym, 2016-04-07 11:52:18]

Thanks Vaclav for review! I've addressed your comments!

https://codereview.chromium.org/1863533003/diff/100001/components/autofill/co...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(right):

https://codereview.chromium.org/1863533003/diff/100001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:172:
// values in the form elements came from the user input. Note that ASSERT()
On 2016/04/06 22:42:12, vabr (Chromium) wrote:
> Actually, there seems to be no ASSERT in the method. Since you are already
> changing the signature, you might want to change it to return the password
form
> instead of using the out-param (and drop the comment about ASSERT())?

Thanks, it makes sense, the body of this function has changed, but ASSERT()
persists. I've made returning password_form and removed ASSERT()

https://codereview.chromium.org/1863533003/diff/100001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:1448:
ASSERT_NO_FATAL_FAILURE(
On 2016/04/06 22:42:12, vabr (Chromium) wrote:
> What would be the fatal failure which could happen in LoadHTMLAndConvertForm?

Done.

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

https://codereview.chromium.org/1863533003/diff/100001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:389:
;
On 2016/04/06 22:42:12, vabr (Chromium) wrote:
> Please revert this typo.

Done.

[vabr (Chromium), 2016-04-07 12:03:46]

LGTM with a nit, thanks!
(Feel free to land after addressing the nit, I might not be able to re-review
before evening East coast time.)

Cheers,
Vaclav

https://codereview.chromium.org/1863533003/diff/120001/components/autofill/co...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(right):

https://codereview.chromium.org/1863533003/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:171:
// a |password_form|. If |with_user_input| == true it's considered that all
nit: There is no longer |password_form|, please update.

[dvadym, 2016-04-07 14:52:58]

Description was changed from

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

BUG=582434, 591058
==========

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

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

[dvadym, 2016-04-07 14:53:22]

Thanks review!

https://codereview.chromium.org/1863533003/diff/120001/components/autofill/co...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(right):

https://codereview.chromium.org/1863533003/diff/120001/components/autofill/co...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:171:
// a |password_form|. If |with_user_input| == true it's considered that all
On 2016/04/07 12:03:45, vabr (Chromium) wrote:
> nit: There is no longer |password_form|, please update.

Done.

[vabr (Chromium), 2016-04-07 14:57:15]

lgtm

[dvadym, 2016-04-07 15:00:17]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2016-04-07 15:00:29]

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

[commit-bot: I haz the power, 2016-04-07 15:40:52]

Description was changed from

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

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

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

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

[commit-bot: I haz the power, 2016-04-07 15:40:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-07 15:42:01]

Description was changed from

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

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

to

==========
Sending a vote that a Password form looks like a SignUp form.

Currently for finding SignUp forms no local heuristics are used. This CL adds
local heuristics for sending vote on server that a form looks similar to a
SignUp form, namely if the form has more than 1 text field with the user input
or it has at least 1 text field with the user input, no password and a new
password field. That covers SignUp forms on many popular sites, such as gmail,
amazon, wikipedia. It could give false positives on change password forms, but
it's not a problem, since a decision anyway will be made server-side and there
are votes for change password forms. It's considered only fields with the user
input, so invisible fields are not a problem here.

BUG=582434

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

[commit-bot: I haz the power, 2016-04-07 15:42:03]

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