PasswordForm: move from current/old password scheme to current/new.

PasswordForm: move from current/old password scheme to current/new.

With the old approach (storing the current and old passwords for a form), we could not tell apart login forms from sign-up forms and change password forms that did not have a password confirmation field.

Furthermore, the new approach align more nicely with the new-password and current-password scheme we use for the autocomplete attributes.

BUG=375333
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282310

[engedy, 2014-06-27 15:13:55]

@Garrett, @Ilya: please take a look.

[Ilya Sherman, 2014-06-27 19:55:04]

Garret is more familiar with this code, so I'll let him do a first pass, and
then chime in with an OWNERS review once y'all are in agreement about the
design.  Thanks.

[Ilya Sherman, 2014-06-27 19:55:21]

Sorry, s/Garret/Garrett/

[engedy, 2014-06-30 09:31:16]

@Vaclav, as Garrett is OOO, could you please take a look?

[vabr (Chromium), 2014-06-30 10:07:20]

LGTM with comments.
Also happy to talk about offline if there are questions.

Cheers,
Vaclav

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:40: //
Two identical passwords: assume we are seeing a new password with a
Did you consider what happens when the values are equal because they are both
empty?

In that case it's not clear what the new password should be, so choosing one at
random might be the best we can do, but in that case we should at least put that
in a comment, especially since it is not consistent with the guess we make in
the else branch.

Or maybe the distinction only matters when we are about to save the password, in
which case it's reasonable to expect that the value is not empty?

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:57: //
field is duplicated more rather than in the ordering of fields.
(typo?) second "more"

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

https://codereview.chromium.org/356223002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:461: if
(observed_form_.password_element.length() == 0 &&
Please use .empty() instead of checking the length().

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

https://codereview.chromium.org/356223002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:125: void
ProvisionallySave(autofill::PasswordForm credentials,
nit: It looks like ProvisionallySave is already called on a form locally created
just for it. To avoid another copy, please make |credentials| a non-const
PasswordForm* argument, and modify it directly, documenting the possible
modification in the comment.

[Ilya Sherman, 2014-07-01 03:08:47]

LGTM2

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:28: //
Helper to determine which password is the main one, and which is the new
Hmm, it's not entirely clear what a "main" password is -- probably worth
clarifying that bit of the comment.

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:127: // The current password.
Required.
In what sense is this field required, if the one above is optional?

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

https://codereview.chromium.org/356223002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:125: void
ProvisionallySave(autofill::PasswordForm credentials,
On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> nit: It looks like ProvisionallySave is already called on a form locally
created
> just for it. To avoid another copy, please make |credentials| a non-const
> PasswordForm* argument, and modify it directly, documenting the possible
> modification in the comment.

FWIW, I agree that we shouldn't pass this parameter by value -- that's just very
unusual in Chrome code, for objects of nontrivial size.  I personally think it
would be most appropriate to pass a const-ref into the function, and then make a
local copy within it; but passing a pointer is fine as well.  The difference is
in terms of whether the modifications are purely an implementation detail, or
visible to an external caller.

[engedy, 2014-07-03 13:34:20]

@Ilya: I have identified a problem with the old solution, please re-review.

@Vaclav: This is what we have discussed offline, still, PTAL.

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:28: //
Helper to determine which password is the main one, and which is the new
On 2014/07/01 03:08:47, Ilya Sherman wrote:
> Hmm, it's not entirely clear what a "main" password is -- probably worth
> clarifying that bit of the comment.

Done.

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:40: //
Two identical passwords: assume we are seeing a new password with a
On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> Did you consider what happens when the values are equal because they are both
> empty?
> 
> In that case it's not clear what the new password should be, so choosing one
at
> random might be the best we can do, but in that case we should at least put
that
> in a comment, especially since it is not consistent with the guess we make in
> the else branch.
> 
> Or maybe the distinction only matters when we are about to save the password,
in
> which case it's reasonable to expect that the value is not empty?

We have discussed this with Vaclav to great length. There are two cases here:

 1) A form with two new password fields (sign-up form). The solution currently
implemented by this CL will result in correct behavior here.

 2) A change password form, with one field for the old, and one for the new
password. Here, we will initially decide that password_element == "" and
new_password_element != "" when we first see the form parsed/rendered. But
later, when the form is finally submitted with the two fields having different
values, we will have password_element != "". This will cause
PasswordManager::ProvisionallySavePassword not to be able to match the submitted
form to any of the PasswordFormManagers, so we will not save the credentials.

We consider (1) to be a more reasonable case to handle correctly, and (2) is
handled incorrectly by the old implementation already in the same way, so at
least we are not worse off. But perhaps we will need to revisit this later.

Perhaps we should take these cases into account when we refactor
PasswordFormManager::DoesManage(), and be on the lookout for matches between
re-purposed fields there.

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:57: //
field is duplicated more rather than in the ordering of fields.
On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> (typo?) second "more"

Done.

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:127: // The current password.
Required.
On 2014/07/01 03:08:47, Ilya Sherman wrote:
> In what sense is this field required, if the one above is optional?

Tried to clarify, but I am not sure myself.

@Vaclav, WDYT?

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

https://codereview.chromium.org/356223002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:461: if
(observed_form_.password_element.length() == 0 &&
On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> Please use .empty() instead of checking the length().

Done. Plus, as per offline discussion, we opted not to change behavior here.

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

https://codereview.chromium.org/356223002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.h:125: void
ProvisionallySave(autofill::PasswordForm credentials,
On 2014/07/01 03:08:47, Ilya Sherman wrote:
> On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> > nit: It looks like ProvisionallySave is already called on a form locally
> created
> > just for it. To avoid another copy, please make |credentials| a non-const
> > PasswordForm* argument, and modify it directly, documenting the possible
> > modification in the comment.
> 
> FWIW, I agree that we shouldn't pass this parameter by value -- that's just
very
> unusual in Chrome code, for objects of nontrivial size.  I personally think it
> would be most appropriate to pass a const-ref into the function, and then make
a
> local copy within it; but passing a pointer is fine as well.  The difference
is
> in terms of whether the modifications are purely an implementation detail, or
> visible to an external caller.

I have reverted this, because it is no longer needed to modify the argument in
the new design.

https://codereview.chromium.org/356223002/diff/50001/components/autofill/cont...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/356223002/diff/50001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:60: if
(!passwords[0].value().isEmpty() &&
We have found a problem here that the set of elements that are marked as
|password_element| and/or |new_password_element| is dependent on whether the
password fields are filled with values or not.

Given that PasswordFormManager's |observed_form_| is parsed before the user
fills in the values, but the |form| received in
PasswordManager::ProvisionallySavePassword is parsed after, they might have a
different layout in terms of |password_element| and |new_password_element|
fields; and we might have the following problems:

 0.) In theory, the dependent code will make different conclusions, in the two
cases, as to whether or not we are dealing with a "login form" or "sign-up or
change password" form -- as this is based on whether or not we are seeing a
|new_password_element|.

 1.) More concretely, PasswordFormManager::DoesManage() will return false if the
|password_element|s differ, therefore the PasswordManager will not find the
PasswordFormManager originally created for |observed_form_|, and will not save
the password in the end.

 2.) Or, if we are lucky enough to get as far as
PasswordFormManager::UpdateLogin(), we might try to update missing element names
in the stored credentials based on an |observed_form_| that is not really a
"login form" -- although we could have told this already once we have seen
|form| with the filled in password values.

After having had a long discussion with Vaclav, we decided the change behavior
here somewhat to address the issues.

In a nutshell, when there is a chance that we might identify a form as a having
a |new_password_element| once we we have seen the password values, we wanted to
make sure that we will always speculatively select a |new_password| field
(perhaps the wrong one) even before the values are input into the password
fields.

We hope that the proposed changes address the most common scenarios, and that
they will not cause breakages with any other important use case.

https://codereview.chromium.org/356223002/diff/50001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:71: }
else if (passwords[0].value() == passwords[1].value()) {
Note that this is also a behavior change.

[vabr (Chromium), 2014-07-03 14:42:57]

Thanks, Balázs! LGTM.

I'm unable to find more problems in the logic than you identified in the
comments (and I'm fine with this CL not fixing those for the reasons you gave).

Cheers,
Vaclav

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:40: //
Two identical passwords: assume we are seeing a new password with a
On 2014/07/03 13:34:19, engedy wrote:
> On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> > Did you consider what happens when the values are equal because they are
both
> > empty?
> > 
> > In that case it's not clear what the new password should be, so choosing one
> at
> > random might be the best we can do, but in that case we should at least put
> that
> > in a comment, especially since it is not consistent with the guess we make
in
> > the else branch.
> > 
> > Or maybe the distinction only matters when we are about to save the
password,
> in
> > which case it's reasonable to expect that the value is not empty?
> 
> We have discussed this with Vaclav to great length. There are two cases here:
> 
>  1) A form with two new password fields (sign-up form). The solution currently
> implemented by this CL will result in correct behavior here.
> 
>  2) A change password form, with one field for the old, and one for the new
> password. Here, we will initially decide that password_element == "" and
> new_password_element != "" when we first see the form parsed/rendered. But
> later, when the form is finally submitted with the two fields having different
> values, we will have password_element != "". This will cause
> PasswordManager::ProvisionallySavePassword not to be able to match the
submitted
> form to any of the PasswordFormManagers, so we will not save the credentials.
> 
> We consider (1) to be a more reasonable case to handle correctly, and (2) is
> handled incorrectly by the old implementation already in the same way, so at
> least we are not worse off. But perhaps we will need to revisit this later.

Thinking about (2) from a bigger distance, I think we should and can fix that.
Not in this CL, though, let's keep it more refactoring than changing behaviour.
LocateSpecificPasswords should have a way to signal it is unsure about the
choice of new_password_element, and DoesManage should be able to tolerate a
specific mismatch (observed new password == submitted current password) in that
case.
Balázs, would you mind filing a bug for fixing (2)?

> 
> Perhaps we should take these cases into account when we refactor
> PasswordFormManager::DoesManage(), and be on the lookout for matches between
> re-purposed fields there.

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:127: // The current password.
Required.
On 2014/07/03 13:34:19, engedy wrote:
> On 2014/07/01 03:08:47, Ilya Sherman wrote:
> > In what sense is this field required, if the one above is optional?
> 
> Tried to clarify, but I am not sure myself.
> 
> @Vaclav, WDYT?

According to the comments, the password_value is "required" but "typically
empty". :)
Since it is an aggregated data member, it's always there. So either the
"requirement" is trivial ("always have the whole object of PasswordForm, not
just a chunk of it"), or it means "non-empty", and then it contradicts the
comment below.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:602: // If
|observed_form_| was a sign-up or change password form, it is no point
optional nit: "it is no point" -> "there is no point in"
But I'm not a native speaker, so feel free to override.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:125:
void AddNewPasswordElement() {
nit: AddNewPasswordElement -> AddNewPasswordElementToObservedForm?

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:266:
PasswordFormManager* manager = new PasswordFormManager(
nit: why allocating a "new PasswordFormManager" on the heap instead of just
instantiating one on the local stack?
And if in the need of a heap version, why don't use
scoped_ptr<PasswordFormManager> instead of a naked pointer and a delete?

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:291:
EXPECT_TRUE(GetPendingCredentials(manager)->password_element.empty());
The comment says "promote the new password element to current", and yet the
expectation is that the current one is empty.
What am I missing?

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:511:
AddNewPasswordElement();
Is this method only called once?
Then I suggest inlining instead:

  PasswordForm new_credentials(*observed_form());
  new_credentials.new_password_element = ASCIIToUTF16("NewPasswd");
  new_credentials.new_password_value = ASCIIToUTF16("test1new");

That improves readability (it is locally clear what are the new values), and
does not leave space for speculations if the side-effect on the observed_form is
also used later.

[engedy, 2014-07-03 18:47:37]

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:40: //
Two identical passwords: assume we are seeing a new password with a
On 2014/07/03 14:42:56, vabr (Chromium) wrote:
> On 2014/07/03 13:34:19, engedy wrote:
> > On 2014/06/30 10:07:20, vabr (Chromium) wrote:
> > > Did you consider what happens when the values are equal because they are
> both
> > > empty?
> > > 
> > > In that case it's not clear what the new password should be, so choosing
one
> > at
> > > random might be the best we can do, but in that case we should at least
put
> > that
> > > in a comment, especially since it is not consistent with the guess we make
> in
> > > the else branch.
> > > 
> > > Or maybe the distinction only matters when we are about to save the
> password,
> > in
> > > which case it's reasonable to expect that the value is not empty?
> > 
> > We have discussed this with Vaclav to great length. There are two cases
here:
> > 
> >  1) A form with two new password fields (sign-up form). The solution
currently
> > implemented by this CL will result in correct behavior here.
> > 
> >  2) A change password form, with one field for the old, and one for the new
> > password. Here, we will initially decide that password_element == "" and
> > new_password_element != "" when we first see the form parsed/rendered. But
> > later, when the form is finally submitted with the two fields having
different
> > values, we will have password_element != "". This will cause
> > PasswordManager::ProvisionallySavePassword not to be able to match the
> submitted
> > form to any of the PasswordFormManagers, so we will not save the
credentials.
> > 
> > We consider (1) to be a more reasonable case to handle correctly, and (2) is
> > handled incorrectly by the old implementation already in the same way, so at
> > least we are not worse off. But perhaps we will need to revisit this later.
> 
> Thinking about (2) from a bigger distance, I think we should and can fix that.
> Not in this CL, though, let's keep it more refactoring than changing
behaviour.
> LocateSpecificPasswords should have a way to signal it is unsure about the
> choice of new_password_element, and DoesManage should be able to tolerate a
> specific mismatch (observed new password == submitted current password) in
that
> case.
> Balázs, would you mind filing a bug for fixing (2)?
> 
> > 
> > Perhaps we should take these cases into account when we refactor
> > PasswordFormManager::DoesManage(), and be on the lookout for matches between
> > re-purposed fields there.
> 

Done.

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

https://codereview.chromium.org/356223002/diff/20001/components/autofill/core...
components/autofill/core/common/password_form.h:127: // The current password.
Required.
On 2014/07/03 14:42:56, vabr (Chromium) wrote:
> On 2014/07/03 13:34:19, engedy wrote:
> > On 2014/07/01 03:08:47, Ilya Sherman wrote:
> > > In what sense is this field required, if the one above is optional?
> > 
> > Tried to clarify, but I am not sure myself.
> > 
> > @Vaclav, WDYT?
> 
> According to the comments, the password_value is "required" but "typically
> empty". :)
> Since it is an aggregated data member, it's always there. So either the
> "requirement" is trivial ("always have the whole object of PasswordForm, not
> just a chunk of it"), or it means "non-empty", and then it contradicts the
> comment below.

I have rephrased.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:602: // If
|observed_form_| was a sign-up or change password form, it is no point
On 2014/07/03 14:42:56, vabr (Chromium) wrote:
> optional nit: "it is no point" -> "there is no point in"
> But I'm not a native speaker, so feel free to override.

Done.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:125:
void AddNewPasswordElement() {
On 2014/07/03 14:42:57, vabr (Chromium) wrote:
> nit: AddNewPasswordElement -> AddNewPasswordElementToObservedForm?

Done.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:266:
PasswordFormManager* manager = new PasswordFormManager(
On 2014/07/03 14:42:56, vabr (Chromium) wrote:
> nit: why allocating a "new PasswordFormManager" on the heap instead of just
> instantiating one on the local stack?
> And if in the need of a heap version, why don't use
> scoped_ptr<PasswordFormManager> instead of a naked pointer and a delete?

I have just replicated an existing test, but I agree that this is stupid. Fixed.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:291:
EXPECT_TRUE(GetPendingCredentials(manager)->password_element.empty());
On 2014/07/03 14:42:56, vabr (Chromium) wrote:
> The comment says "promote the new password element to current", and yet the
> expectation is that the current one is empty.
> What am I missing?

I have forgot to update this comment. Done.

https://codereview.chromium.org/356223002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:511:
AddNewPasswordElement();
On 2014/07/03 14:42:57, vabr (Chromium) wrote:
> Is this method only called once?
> Then I suggest inlining instead:
> 
>   PasswordForm new_credentials(*observed_form());
>   new_credentials.new_password_element = ASCIIToUTF16("NewPasswd");
>   new_credentials.new_password_value = ASCIIToUTF16("test1new");
> 
> That improves readability (it is locally clear what are the new values), and
> does not leave space for speculations if the side-effect on the observed_form
is
> also used later.

Done.

[vabr (Chromium), 2014-07-04 15:49:15]

Thanks a lot, still LGTM.
Vaclav

[Garrett Casto, 2014-07-07 21:57:24]

Not sure if you're waiting on me, but LGTM with a couple of minor comments.

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

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:476: // Ignore
change passwords until we have some change password functionality.
I don't think that we need this, either earlier or now. Since we don't serialize
new_password_element it will never be non-empty after loading from the
PasswordStore. Also I think the comment is misleading. If we were to handle
change password forms better, it would be during saving not during loading.

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:264:
SimulateMatchingPhase(&*manager, false);
Nit: I realize &* is shorter, but I think that .get() is clearer.

[engedy, 2014-07-07 22:07:56]

Yes, I wanted to wait until you have a chance to take a look, mostly to make
sure I am not breaking anything with the changes to LocateSpecificPasswords();
and also in the special case where LocateSpecificPasswords emits different
PasswordForms in terms of what is assigned to the new/current password fields
before and after the password fields are filled with values.

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

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:476: // Ignore
change passwords until we have some change password functionality.
On 2014/07/07 21:57:24, Garrett Casto wrote:
> I don't think that we need this, either earlier or now. Since we don't
serialize
> new_password_element it will never be non-empty after loading from the
> PasswordStore. Also I think the comment is misleading. If we were to handle
> change password forms better, it would be during saving not during loading.

Note that this is actually even more misleading: we are checking
|observed_form_| here, not the |form| coming from the PasswordStore.

I think the original intention was to prevent autofill when we are seeing a
change password form. I was also hesitant whether or not this is actually
needed, but did not want to change behavior unrelated to this refactoring step.

I am happy to remove this in one step if you prefer.

[Ilya Sherman, 2014-07-08 00:48:33]

(Still LGTM)

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

https://codereview.chromium.org/356223002/diff/110001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:61:
(passwords[0].value() == passwords[1].value() &&
Optional nit: No need for the first paren on this line, since all of the
conditions in this if-stmt are &&'ed together.

[engedy, 2014-07-09 14:15:47]

I have added a missing unit test that I think has some value.

@Garrett, could you please take a final look (also at my reply to the one
outstanding comment thread in my previous message).

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

https://codereview.chromium.org/356223002/diff/110001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:61:
(passwords[0].value() == passwords[1].value() &&
On 2014/07/08 00:48:33, Ilya Sherman wrote:
> Optional nit: No need for the first paren on this line, since all of the
> conditions in this if-stmt are &&'ed together.

Done.

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:264:
SimulateMatchingPhase(&*manager, false);
On 2014/07/07 21:57:24, Garrett Casto wrote:
> Nit: I realize &* is shorter, but I think that .get() is clearer.

Done, I have rebased this CL on some earlier refactorings, so this ugly hack is
no longer needed.

[Garrett Casto, 2014-07-09 22:24:19]

lgtm

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

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:476: // Ignore
change passwords until we have some change password functionality.
On 2014/07/07 22:07:56, engedy wrote:
> On 2014/07/07 21:57:24, Garrett Casto wrote:
> > I don't think that we need this, either earlier or now. Since we don't
> serialize
> > new_password_element it will never be non-empty after loading from the
> > PasswordStore. Also I think the comment is misleading. If we were to handle
> > change password forms better, it would be during saving not during loading.
> 
> Note that this is actually even more misleading: we are checking
> |observed_form_| here, not the |form| coming from the PasswordStore.
> 
> I think the original intention was to prevent autofill when we are seeing a
> change password form. I was also hesitant whether or not this is actually
> needed, but did not want to change behavior unrelated to this refactoring
step.
> 
> I am happy to remove this in one step if you prefer.

Ah, yeah misread that. The current check seems too restrictive in that case
given that change password forms don't necessarily need to have password_element
set. This would be a change in behavior from before though as a form with two
password fields would not autofill (since initially both fields will be empty).
I think that this is probably okay since we are moving in this direction anyway.
This probably doesn't affect that many forms anyway, since it's pretty rare to
have a username field in change password forms, and we won't autofill in any
case without one.

So I think that switching this to just
!observed_form_.new_password_element.empty() should be fine for now, and we can
work out any other changes we want to make later.

[engedy, 2014-07-10 09:23:03]

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

https://codereview.chromium.org/356223002/diff/110001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:476: // Ignore
change passwords until we have some change password functionality.
On 2014/07/09 22:24:18, Garrett Casto wrote:
> On 2014/07/07 22:07:56, engedy wrote:
> > On 2014/07/07 21:57:24, Garrett Casto wrote:
> > > I don't think that we need this, either earlier or now. Since we don't
> > serialize
> > > new_password_element it will never be non-empty after loading from the
> > > PasswordStore. Also I think the comment is misleading. If we were to
handle
> > > change password forms better, it would be during saving not during
loading.
> > 
> > Note that this is actually even more misleading: we are checking
> > |observed_form_| here, not the |form| coming from the PasswordStore.
> > 
> > I think the original intention was to prevent autofill when we are seeing a
> > change password form. I was also hesitant whether or not this is actually
> > needed, but did not want to change behavior unrelated to this refactoring
> step.
> > 
> > I am happy to remove this in one step if you prefer.
> 
> Ah, yeah misread that. The current check seems too restrictive in that case
> given that change password forms don't necessarily need to have
password_element
> set. This would be a change in behavior from before though as a form with two
> password fields would not autofill (since initially both fields will be
empty).
> I think that this is probably okay since we are moving in this direction
anyway.
> This probably doesn't affect that many forms anyway, since it's pretty rare to
> have a username field in change password forms, and we won't autofill in any
> case without one.
> 
> So I think that switching this to just
> !observed_form_.new_password_element.empty() should be fine for now, and we
can
> work out any other changes we want to make later. 

Done.

[engedy, 2014-07-10 09:23:08]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2014-07-10 09:24:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/engedy@chromium.org/356223002/190001

[commit-bot: I haz the power, 2014-07-10 11:17:28]

Change committed as 282310