Implementation of new behaviour of Password Manager on retry password forms.

Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission of a retry password form:
1.If there are no any credentials saved for this site then to propose to save credentials without username
2.If there is a stored credentials with the same password (but possible with username) then do nothing (currently the user is used to save "no username" password)
3.If there are stored credentials but all they have different password then propose the user to update password. It might be when we missed change password form submission, and the
then autofill old password and then site shows the user retry password form. This scenario happens on facebook.com

BUG=359315, 527203
Committed: https://crrev.com/412087c2ca68dbdcd1865068654c1b8bb1ac804a
Cr-Commit-Position: refs/heads/master@{#361906}

[dvadym, 2015-11-25 15:08:37]

Description was changed from

==========
Implementation of new retry password form behaviour of Password Manager.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315
==========

to

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315
==========

[dvadym, 2015-11-25 15:13:59]

Description was changed from

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315
==========

to

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315,527203
==========

[dvadym, 2015-11-26 12:13:31]

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

[dvadym, 2015-11-26 12:13:32]

Hi Vaclav,

 Could you please review this CL?

Regards,
Vadym

[vabr (Chromium), 2015-11-26 14:35:54]

Thanks, Vadym,
LGTM with comments.

I have some reservations about the term "retry" form. These are often
"reauthenticate" forms as well. IIUC, we need a term to describe username-less
forms, which are not reset password forms (they have current-password, not
new-password). The long way to call these is "forms without username, updating
stored passwords". It is not clear how to make it a short name, but we should at
least define the term "retry" to mean this, it is not obvious otherwise.

Cheers,
Vaclav

https://codereview.chromium.org/1476523004/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1476523004/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2829: //
submitted and there are stored credentials but with different password.
typo: submitted -> is submitted

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

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
nit: You are occluding the |it| from line 1263. Please use a more meaningful
name for each of the two |it|s.

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
nit: Please use a range-based loop for better readability, and const for clean
code:

for (const auto stored_match : best_matches_) {...}

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
From this point until the end of the method, the code works similar to
FindBestMatchForUpdatePassword. The difference is that the other method requires
a unique match, and checks for empty password to speed-up searching. The second
one does not change the result, but the first one does. Are you not requiring a
unique match here on purpose, or could you reuse the
FindBestMatchForUpdatePassword code to avoid duplication?

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

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:470: // Whether
this submission is considered to be an entering new password on a
The comment needs a bit more clarity. "Retry form" is not defined. Also, there
is no "this submission" obviously associated with a PasswordFormManager. I
suggest binding this to which methods of the PFM were called as the effect of
the submission, something like:
// This is true if the password passed through MethodCalledDuringSubmission() is
a new password for an existing credential, and it was entered on a password form
without a username, while the stored credential does have a non-empty username.

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:472: bool
retry_password_form_password_update_;
optional nit: You can also use

bool retry_password_form_password_update_ = false;

It is easier to look up, and slims down the constructor.
(Optional because the rest of the file does not use this. If you feel strong
about that consistency, feel free to ignore.)

[dvadym, 2015-11-26 15:19:55]

Thanks Vaclav, I've addressed your comments. PTAL

https://codereview.chromium.org/1476523004/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1476523004/diff/40001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2829: //
submitted and there are stored credentials but with different password.
On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> typo: submitted -> is submitted

Done.

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

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> nit: You are occluding the |it| from line 1263. Please use a more meaningful
> name for each of the two |it|s.

Done.

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> nit: Please use a range-based loop for better readability, and const for clean
> code:
> 
> for (const auto stored_match : best_matches_) {...}

Thanks it will make code cleaner

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> From this point until the end of the method, the code works similar to
> FindBestMatchForUpdatePassword. The difference is that the other method
requires
> a unique match, and checks for empty password to speed-up searching. The
second
> one does not change the result, but the first one does. Are you not requiring
a
> unique match here on purpose, or could you reuse the
> FindBestMatchForUpdatePassword code to avoid duplication?

Yeah, I saw that this functions are quite similar, but they are have slightly
different logic, so I don't think that it's worth to do one function. In this
part the difference is that unique match is not required, because I want to
verify that we already have forms with such passwords and not to show bubble, it
doesn't matter if there are multiple stored credentials with this password.

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

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:470: // Whether
this submission is considered to be an entering new password on a
On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> The comment needs a bit more clarity. "Retry form" is not defined. Also, there
> is no "this submission" obviously associated with a PasswordFormManager. I
> suggest binding this to which methods of the PFM were called as the effect of
> the submission, something like:
> // This is true if the password passed through MethodCalledDuringSubmission()
is
> a new password for an existing credential, and it was entered on a password
form
> without a username, while the stored credential does have a non-empty
username.

I've changed comment to 
 // A form is considered to be retry password if it has only one field which is
  // a password field.
  // This variable is true if the password passed through ProvisionallySave() is
  // a password that is not equal to any password from stored for this origin
  // credentials and it was entered on a retry password form.

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:472: bool
retry_password_form_password_update_;
On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> optional nit: You can also use
> 
> bool retry_password_form_password_update_ = false;
> 
> It is easier to look up, and slims down the constructor.
> (Optional because the rest of the file does not use this. If you feel strong
> about that consistency, feel free to ignore.)

I'll leave as is, since all other variable in this file are initialized in old
way. We should make another CL to refactor this in all our files

[dvadym, 2015-11-26 15:35:05]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-11-26 15:35:05]

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/1476523004/#ps80001
(title: "Comments clean up")

[commit-bot: I haz the power, 2015-11-26 15:56:23]

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

[commit-bot: I haz the power, 2015-11-26 16:46:25]

Description was changed from

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315,527203
==========

to

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315,527203
==========

[commit-bot: I haz the power, 2015-11-26 16:46:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-26 16:47:10]

Description was changed from

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315,527203
==========

to

==========
Implementation of new behaviour of Password Manager on retry password forms.

A password form is considered to be retry password form if there is only one
field, which is a password field.

In this CL it's implemented the following behaviour on the successful submission
of a retry password form:
1.If there are no any credentials saved for this site then to propose to save
credentials without username
2.If there is a stored credentials with the same password (but possible with
username) then do nothing (currently the user is used to save "no username"
password)
3.If there are stored credentials but all they have different password then
propose the user to update password. It might be when we missed change password
form submission, and the
then autofill old password and then site shows the user retry password form.
This scenario happens on facebook.com

BUG=359315,527203

Committed: https://crrev.com/412087c2ca68dbdcd1865068654c1b8bb1ac804a
Cr-Commit-Position: refs/heads/master@{#361906}
==========

[commit-bot: I haz the power, 2015-11-26 16:47:11]

Patchset 5 (id:??) landed as
https://crrev.com/412087c2ca68dbdcd1865068654c1b8bb1ac804a
Cr-Commit-Position: refs/heads/master@{#361906}

[vabr (Chromium), 2015-11-26 17:10:37]

Still LGTM, just some nits on the one longer comment.

No need to rush addressing them, feel free to do it in whatever other CL you are
doing next in this code.

If you do make a separate CL for the nits below, feel free to just TBR me for
those changes.

Thanks,
Vaclav

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

https://codereview.chromium.org/1476523004/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:1269: for
(auto it = best_matches_.begin(); it != best_matches_.end(); ++it) {
On 2015/11/26 15:19:55, dvadym wrote:
> On 2015/11/26 14:35:54, vabr (Chromium) wrote:
> > From this point until the end of the method, the code works similar to
> > FindBestMatchForUpdatePassword. The difference is that the other method
> requires
> > a unique match, and checks for empty password to speed-up searching. The
> second
> > one does not change the result, but the first one does. Are you not
requiring
> a
> > unique match here on purpose, or could you reuse the
> > FindBestMatchForUpdatePassword code to avoid duplication?
> 
> Yeah, I saw that this functions are quite similar, but they are have slightly
> different logic, so I don't think that it's worth to do one function. In this
> part the difference is that unique match is not required, because I want to
> verify that we already have forms with such passwords and not to show bubble,
it
> doesn't matter if there are multiple stored credentials with this password.

Acknowledged.

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

https://codereview.chromium.org/1476523004/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:473: // a
password that is not equal to any password from stored for this origin
typo: from -> form

https://codereview.chromium.org/1476523004/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:473: // a
password that is not equal to any password from stored for this origin
nit: equal to -> part of
(password and form are different objects)

https://codereview.chromium.org/1476523004/diff/80001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:474: //
credentials and it was entered on a retry password form.
nit: Seems like you could leave out "credentials"?