Updating of all entries in PasswordManager of the same credentials on password update

Updating of all entries in PasswordManager of the same credentials on password update. Credentials are considered to be the same if
1.they on the same or PSL match origin
2.they have the same username and password

Also this CL contains refactoring:
1.removing |original_signon_realm| from PasswordForm, that also lead to changes in affiliation staff.
2.Password stores return not changed entries

BUG=359315
Committed: https://crrev.com/cb9245959cc84cd107f80421dd2456d794a0752f
Cr-Commit-Position: refs/heads/master@{#351539}

[dvadym, 2015-08-27 14:19:24]

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

[dvadym, 2015-08-27 14:19:24]

Hi Vaclav and Vasilii,

 Please give your feedback on these changes. Currently I've implemented leaving
of old origin only for libsecret backend. As soon as everything else is good,
I'll update all other backends. Tests will be uploaded later.

Best regards,
Vadym

[vasilii, 2015-08-27 15:36:25]

The big question is when you want to update the PSL password?
I think it should be updated if it appeared in the best matches. Otherwise we
are not sure that the stored form is related to the one user logged in on.

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

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:690:
PasswordFormMap::const_iterator updated_password_it =
How this block is different from the one above?

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:700:
psl_matches_[i]->signon_realm = psl_matches_[i]->original_signon_realm;
Do we need original_signon_realm at all?

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

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:381:
ScopedVector<autofill::PasswordForm> psl_matches_;
The name is confusing. PSL may go to |best_matches_| as well if they have the
best score.

[vabr (Chromium), 2015-08-28 07:18:23]

On 2015/08/27 15:36:25, vasilii wrote:
> The big question is when you want to update the PSL password?
> I think it should be updated if it appeared in the best matches. Otherwise we
> are not sure that the stored form is related to the one user logged in on.

The PSL-matched form will not make it to best_matches_ as soon as the user has
any non-PSL-matched credential stored for the current form (could be even with a
different username). Restricting the update to just the case when the
PSL-matched form is in best_matches_ would therefore stop the update in most
cases. If we do not want to update PSL matches outside of best_matches_, then I
think we should just not do it at all (the increased code complexity is not
worth it).

Also, speaking about other platforms -- note that KDE still does not have PSL
matching (the half-done CL is sleeping at the bottom of my TODO list). If that
becomes a blocker for you, please let me know, I'll finally add it.

Cheers,
Vaclav

[vasilii, 2015-08-28 09:30:14]

On 2015/08/28 07:18:23, vabr (slow start past holiday) wrote:
> On 2015/08/27 15:36:25, vasilii wrote:
> > The big question is when you want to update the PSL password?
> > I think it should be updated if it appeared in the best matches. Otherwise
we
> > are not sure that the stored form is related to the one user logged in on.
> 
> The PSL-matched form will not make it to best_matches_ as soon as the user has
> any non-PSL-matched credential stored for the current form (could be even with
a
> different username). Restricting the update to just the case when the
> PSL-matched form is in best_matches_ would therefore stop the update in most
> cases. If we do not want to update PSL matches outside of best_matches_, then
I
> think we should just not do it at all (the increased code complexity is not
> worth it).
> 
> Also, speaking about other platforms -- note that KDE still does not have PSL
> matching (the half-done CL is sleeping at the bottom of my TODO list). If that
> becomes a blocker for you, please let me know, I'll finally add it.
> 
> Cheers,
> Vaclav

In this CL the update of PSL credential happens silently. How can it be that
both
- a PSL credential isn't good enough to go to best_matches (therefore, being
autofilled and displayed in the manage passwords bubble)
- and it's good enough to be considered a duplicate and updated silently.

Imagine a case:
PasswordStore contains [user1, facebook.com], [user2, m.facebook.com]
The user goes to m.facebook.com, types user1 and logs in with a new password.
What should happen?

[dvadym, 2015-08-28 09:52:08]

Thanks Vasilli and Vaclav for comments!

Vaclav is right, |best_matches_| doesn't contain any PSL matches if not PSL
matches are present. 

In order to update username and old password should be the same as in a
submitted form, so in Vasilii's case there will be no any silent update.

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

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:690:
PasswordFormMap::const_iterator updated_password_it =
On 2015/08/27 15:36:25, vasilii wrote:
> How this block is different from the one above?

Thanks, I forgot to remove it.

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:700:
psl_matches_[i]->signon_realm = psl_matches_[i]->original_signon_realm;
On 2015/08/27 15:36:25, vasilii wrote:
> Do we need original_signon_realm at all?

I've checked, if original_signon_realm is removed, signon_realm realm from
observed_form and new bool flag |is_psl_matched| is introduce, it works partly,
namely it suggests to autofill but doesn't save after successful login. Probably
it's possible to get rid of |original_signon_realm| but it's different task, I
can create bug for this refactoring.

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

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:381:
ScopedVector<autofill::PasswordForm> psl_matches_;
On 2015/08/27 15:36:25, vasilii wrote:
> The name is confusing. PSL may go to |best_matches_| as well if they have the
> best score.

I've renamed to |psl_not_best_matches_| and added comment.

[vabr (Chromium), 2015-08-28 10:47:23]

Thanks Vadym.

In general this looks good to me, but below are some more comments.

Cheers,
Vaclav

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

https://codereview.chromium.org/1314903003/diff/20001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:700:
psl_matches_[i]->signon_realm = psl_matches_[i]->original_signon_realm;
On 2015/08/28 09:52:08, dvadym wrote:
> On 2015/08/27 15:36:25, vasilii wrote:
> > Do we need original_signon_realm at all?
> 
> I've checked, if original_signon_realm is removed, signon_realm realm from
> observed_form and new bool flag |is_psl_matched| is introduce, it works
partly,
> namely it suggests to autofill but doesn't save after successful login.
Probably
> it's possible to get rid of |original_signon_realm| but it's different task, I
> can create bug for this refactoring.

I'm confused -- I thought that original_signon_realm is always empty in forms
inside PasswordStore. What exactly do you mean by "doesn't save after
successful" login?

https://codereview.chromium.org/1314903003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret.cc (left):

https://codereview.chromium.org/1314903003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret.cc:619: form->origin =
lookup_form->origin;
Do you remove this to be able to update it correctly later? Does it influence
the scoring for the PSL matches?

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

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:381: // Set of
forms from DB that are not in |best_matches_| and that are psl
nits:
DB -> PasswordStore
psl -> PSL

[vasilii, 2015-08-28 11:10:48]

We need the unit tests of course.

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

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:625: bool
password_was_update = false;
password_was_updated

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:634:
old_primary_key);
What about this case and the case below?

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:696:
password_store->UpdateLogin(*psl_not_best_matches_[i]);
What about the origin? I'm afraid it's lost during PasswordStore::GetLogins()
call.

[dvadym, 2015-08-28 11:25:29]

Sure, tests will be uploaded when general approach is approved by you.

https://codereview.chromium.org/1314903003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret.cc (left):

https://codereview.chromium.org/1314903003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret.cc:619: form->origin =
lookup_form->origin;
On 2015/08/28 10:47:23, vabr (slow start past holiday) wrote:
> Do you remove this to be able to update it correctly later? Does it influence
> the scoring for the PSL matches?

Good point, it influences PSL scoring, but current algo for scoring PSL matching
is very strange. For PSL matching forms candidate.origin ==
observed_form_.origin (here
https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...)
and so everything after / is not taken into consideration. So leaving origin as
in stored form seems to be event better, but we need to add some votes for PSL
matching in order not to get 0 result. What do you think?

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

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:625: bool
password_was_update = false;
On 2015/08/28 11:10:48, vasilii wrote:
> password_was_updated

Done.

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:634:
old_primary_key);
On 2015/08/28 11:10:48, vasilii wrote:
> What about this case and the case below?

This case is not for real password update. It was introduced when we incorrectly
saved username (for example on SignUp form) and then the user typed new one.

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.cc:696:
password_store->UpdateLogin(*psl_not_best_matches_[i]);
On 2015/08/28 11:10:48, vasilii wrote:
> What about the origin? I'm afraid it's lost during PasswordStore::GetLogins()
> call.

I've done for libsecret that it's not lost, as I wrote for others backends I'll
do later (as soon as this approach receives approval).

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

https://codereview.chromium.org/1314903003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager.h:381: // Set of
forms from DB that are not in |best_matches_| and that are psl
On 2015/08/28 10:47:23, vabr (slow start past holiday) wrote:
> nits:
> DB -> PasswordStore
> psl -> PSL

Done.

[vabr (Chromium), 2015-08-28 11:38:41]

https://codereview.chromium.org/1314903003/diff/40001/chrome/browser/password...
File chrome/browser/password_manager/native_backend_libsecret.cc (left):

https://codereview.chromium.org/1314903003/diff/40001/chrome/browser/password...
chrome/browser/password_manager/native_backend_libsecret.cc:619: form->origin =
lookup_form->origin;
On 2015/08/28 11:25:28, dvadym wrote:
> On 2015/08/28 10:47:23, vabr (slow start past holiday) wrote:
> > Do you remove this to be able to update it correctly later? Does it
influence
> > the scoring for the PSL matches?
> 
> Good point, it influences PSL scoring, but current algo for scoring PSL
matching
> is very strange. For PSL matching forms candidate.origin ==
> observed_form_.origin (here
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...)
> and so everything after / is not taken into consideration. So leaving origin
as
> in stored form seems to be event better, but we need to add some votes for PSL
> matching in order not to get 0 result. What do you think?

Yes, it is fine to adapt the scoring, but we should make sure that the result is
not too different from what it is now.

[vasilii, 2015-08-28 11:38:59]

I want to go back to my use case.
[user1, facebook.com] is a PSL match. The pending_credentials.username_value is
also 'user1'. We won't update the credential currently because
PasswordFormManager starts the new login flow.

What is the ideal behavior in this case regardless of this CL?

[vabr (Chromium), 2015-08-28 11:48:33]

On 2015/08/28 11:38:59, vasilii wrote:
> I want to go back to my use case.
> [user1, facebook.com] is a PSL match. The pending_credentials.username_value
is
> also 'user1'. We won't update the credential currently because
> PasswordFormManager starts the new login flow.
> 
> What is the ideal behavior in this case regardless of this CL?

Do you assume the current page is m.facebook.com?

I would say we should update the PSL-matched credential for user1 as well. For
some sites, the PSL-matched credentials are not meant to be linked together, but
our current design is to ignore those corner cases, as there is no good
criterion to distinguish when two PSL-matched credentials are for one or more
accounts.

[dvadym, 2015-08-28 12:02:25]

On 2015/08/28 11:48:33, vabr (slow start past holiday) wrote:
> On 2015/08/28 11:38:59, vasilii wrote:
> > I want to go back to my use case.
> > [user1, facebook.com] is a PSL match. The pending_credentials.username_value
> is
> > also 'user1'. We won't update the credential currently because
> > PasswordFormManager starts the new login flow.
> > 
> > What is the ideal behavior in this case regardless of this CL?
> 
> Do you assume the current page is m.facebook.com?
> 
> I would say we should update the PSL-matched credential for user1 as well. For
> some sites, the PSL-matched credentials are not meant to be linked together,
but
> our current design is to ignore those corner cases, as there is no good
> criterion to distinguish when two PSL-matched credentials are for one or more
> accounts.

I wouldn't feel comfortable to update PSL matched credentials if I'm not sure
that this is the same account. Current approach is to update only if the PSL
matched credential has the same username and the same old password. Having the
same usernames it seems to be not sufficient, for example all *.appspot.com
sites will be PSL matched, and it's ok that user has the same username on
different domains. So as for me it's better not to update then to update
incorrect credentials.

Vasilii, I'm not sure what do you mean by "PasswordFormManager starts the new
login flow."? Could you please clarify?

[vasilii, 2015-08-28 13:32:24]

On 2015/08/28 11:48:33, vabr (slow start past holiday) wrote:
> On 2015/08/28 11:38:59, vasilii wrote:
> > I want to go back to my use case.
> > [user1, facebook.com] is a PSL match. The pending_credentials.username_value
> is
> > also 'user1'. We won't update the credential currently because
> > PasswordFormManager starts the new login flow.
> > 
> > What is the ideal behavior in this case regardless of this CL?
> 
> Do you assume the current page is m.facebook.com?
> 
> I would say we should update the PSL-matched credential for user1 as well. For
> some sites, the PSL-matched credentials are not meant to be linked together,
but
> our current design is to ignore those corner cases, as there is no good
> criterion to distinguish when two PSL-matched credentials are for one or more
> accounts.

Yes, the current page is m.facebook.com.
Are you OK that with the current approach the PSL-matched user1 isn't updated?

Vadim, I'm not sure why you are mentioning the old password. I think the
probability that you picked the correct form from |best_matches| is the same as
that you correctly pick one from |psl_not_best_matches_| by username. Note that
in my use case we'd autofill user1 if [user2, m.facebook.com] was absent. That
is, the current model thinks that [user1, facebook.com] is a credential for
m.facebook.com.

[vasilii, 2015-08-28 13:54:39]

I also wonder what happens if there is just one matching credential which was a
PSL match. |psl_not_best_matches_| will be empty. Doesn't PasswordFormManager
simply try to call PasswordStore::UpdateLogin() which fails due to signon_realm
and origin were overwritten during retrieval?

[vabr (Chromium), 2015-09-11 14:57:57]

On 2015/08/28 13:54:39, vasilii (OOO til 18.09) wrote:
> I also wonder what happens if there is just one matching credential which was
a
> PSL match. |psl_not_best_matches_| will be empty. Doesn't PasswordFormManager
> simply try to call PasswordStore::UpdateLogin() which fails due to
signon_realm
> and origin were overwritten during retrieval?

Sorry, Vadym, I forgot to respond on this one.
The discussion seems a bit convoluted. Should we (all) meet and think the design
through?

Cheers,
Vaclav

[dvadym, 2015-09-23 14:28:52]

Hi Vaclav and Vasilii,

 Could you please have a look at this CL once more?

Best regards,
Vadym

[dvadym, 2015-09-23 14:31:34]

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

[dvadym, 2015-09-23 14:31:34]

Hi Balasz,

 Could you please have a look if changes in this CL correctly treat affiliated
credentials?

Best regards,
Vadym

[vabr (Chromium), 2015-09-24 15:17:33]

LGTM with comments, but I have not looked at the affiliation bits closely.
Please let me know if you want me to review those in addition to Balázs' review.

Cheers,
Vaclav

https://codereview.chromium.org/1314903003/diff/220001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1314903003/diff/220001/chrome/browser/passwor...
chrome/browser/password_manager/password_store_mac.cc:482: form_a.signon_realm
== form_b.signon_realm;
This duplicates the condition from line 479.
I believe we can just get rid of FUZZY_FORM_MATCH entirely, but we should test
on Mac that this does not end up deleting passwords from keychain.

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/password_form.cc:91: bool
PasswordForm::IsPublicSuffixMatch() const {
We should get rid of this method completely and replace it with checking the
data member. (Don't worry about dropping the CHECK(is_alive).)

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:63:
EXPECT_EQ("", result.preferred_realm);
nit: "" -> std::string()
(Also on line 165.)

Note that
EXPECT_TRUE(result.preferred_realm.empty());
would be another way to write this, but the EXPECT_EQ will also output the realm
if it is not empty, which might be useful.

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:112: //
Create a match that matches exactly, so |is_public_suffix_match| == false.
Either set it explicitly to false, or note that false is the default value.

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
File components/autofill/core/common/save_password_progress_logger.cc (left):

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/save_password_progress_logger.cc:187: case
SavePasswordProgressLogger::STRING_ORIGINAL_SIGNON_REALM:
Could you please add logging of the |is_public_suffix_match| bit instead?

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
File components/password_manager/core/browser/affiliated_match_helper.h (left):

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/affiliated_match_helper.h:93: //
Consumes a list of |android_credentials| corresponding to applications that
Please mention your changes to affiliation in the CL description.

Also note that I have not reviewed the affiliation part, because you asked
engedy@ to have a look. If you want an additional review, please let me know.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:862: //
|original_signon_realm| on |pending_credentials_|, which unmarks it as
This comment is out of date, it references original_signon_realm.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:877:
DCHECK(!IsPendingCredentialsPublicSuffixMatch());
I don't think this DCHECK is meaningful any more. It checks that the flag set to
false above is still false one line below.

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

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:191:
RESULT_FULL_AND_PSL_FOUND
nit: Maybe it is time to convert this to a bit mask?
enum ResultOfSimulatedMatching {
  RESULT_SAVED_MATCH = 1 << 0,  // Include saved_match_ in store results.
  RESULT_PSL_MATCH = 1 << 1,  // Include psl_saved_match_ in store results.
}

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1079:
simulated_results[0]->username_value = ASCIIToUTF16("other@gmail.com");
Why this change (1->0)?

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1225:
// origin and action. Public suffix match is not taken into consideration
How come? Line 988 in ScoreResult in your patch still contains the bonus points
for not being a PSL match.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2226:
// User submits current and new credentials to the observed form.
What do you mean by "current and new credentials"?

[dvadym, 2015-09-25 10:19:53]

Thanks Vaclav, I've addressed your comments in PatchSet 14. PTAL.

https://codereview.chromium.org/1314903003/diff/220001/chrome/browser/passwor...
File chrome/browser/password_manager/password_store_mac.cc (right):

https://codereview.chromium.org/1314903003/diff/220001/chrome/browser/passwor...
chrome/browser/password_manager/password_store_mac.cc:482: form_a.signon_realm
== form_b.signon_realm;
On 2015/09/24 15:17:33, vabr (OOO until Tue) wrote:
> This duplicates the condition from line 479.
> I believe we can just get rid of FUZZY_FORM_MATCH entirely, but we should test
> on Mac that this does not end up deleting passwords from keychain.

Done.

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/password_form.cc:91: bool
PasswordForm::IsPublicSuffixMatch() const {
On 2015/09/24 15:17:33, vabr (OOO until Tue) wrote:
> We should get rid of this method completely and replace it with checking the
> data member. (Don't worry about dropping the CHECK(is_alive).)

Done.

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:63:
EXPECT_EQ("", result.preferred_realm);
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> nit: "" -> std::string()
> (Also on line 165.)
> 
> Note that
> EXPECT_TRUE(result.preferred_realm.empty());
> would be another way to write this, but the EXPECT_EQ will also output the
realm
> if it is not empty, which might be useful.

Done.

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:112: //
Create a match that matches exactly, so |is_public_suffix_match| == false.
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> Either set it explicitly to false, or note that false is the default value.

I've writtten "So|is_public_suffix_match| has default value false."

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
File components/autofill/core/common/save_password_progress_logger.cc (left):

https://codereview.chromium.org/1314903003/diff/220001/components/autofill/co...
components/autofill/core/common/save_password_progress_logger.cc:187: case
SavePasswordProgressLogger::STRING_ORIGINAL_SIGNON_REALM:
On 2015/09/24 15:17:33, vabr (OOO until Tue) wrote:
> Could you please add logging of the |is_public_suffix_match| bit instead?

Done.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
File components/password_manager/core/browser/affiliated_match_helper.h (left):

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/affiliated_match_helper.h:93: //
Consumes a list of |android_credentials| corresponding to applications that
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> Please mention your changes to affiliation in the CL description.
> 
> Also note that I have not reviewed the affiliation part, because you asked
> engedy@ to have a look. If you want an additional review, please let me know.

Done.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:862: //
|original_signon_realm| on |pending_credentials_|, which unmarks it as
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> This comment is out of date, it references original_signon_realm.

Done.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:877:
DCHECK(!IsPendingCredentialsPublicSuffixMatch());
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> I don't think this DCHECK is meaningful any more. It checks that the flag set
to
> false above is still false one line below.

Done.

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

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:191:
RESULT_FULL_AND_PSL_FOUND
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> nit: Maybe it is time to convert this to a bit mask?
> enum ResultOfSimulatedMatching {
>   RESULT_SAVED_MATCH = 1 << 0,  // Include saved_match_ in store results.
>   RESULT_PSL_MATCH = 1 << 1,  // Include psl_saved_match_ in store results.
> }

Thanks, it makes sense, done

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1079:
simulated_results[0]->username_value = ASCIIToUTF16("other@gmail.com");
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> Why this change (1->0)?

Now we take the first form from the ones that matches the best, before we took
the last one. For details see password_form_manager.cc:460 in this CL.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1225:
// origin and action. Public suffix match is not taken into consideration
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> How come? Line 988 in ScoreResult in your patch still contains the bonus
points
> for not being a PSL match.

I meant that the property to be public suffix match gives 0 to score. That I
believe is not correct, but it's another story. But I agree that
!IsPublicSuffixMatch gives results. So I've returned back previous comment.

https://codereview.chromium.org/1314903003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2226:
// User submits current and new credentials to the observed form.
On 2015/09/24 15:17:33, vabr (Chromium) wrote:
> What do you mean by "current and new credentials"?

Thanks, it's left from the test I've copied.

[dvadym, 2015-09-25 10:44:16]

Hi Vasilii,

 Could you please have a look at changes in password_store_mac?

Best regards,
Vadym

[dvadym, 2015-09-25 10:44:48]

Hi Balasz,

 Could you please review changes in affiliation stuff?

Best regards,
Vadym

[vasilii, 2015-09-25 14:16:16]

Mostly good. There are some open questions.

https://codereview.chromium.org/1314903003/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:265: bool
is_public_suffix_match;
I think that Android credentials used to be a "PSL match". If this is true (ask
Balazs) I'd make it an enum.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:72:
local_results.insert(local_results.end(), affiliated_results.begin(),
This is definitely a right thing to do. I got a bug report few days ago where a
user couldn't delete an android credential from the bubble.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/login_database.cc:939: if
(form.is_public_suffix_match) {
Hmm, I think the comment isn't true anymore. The PSL match should be in the
database and, therefore, can be deleted.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:471:
best_matches_.insert(username, login.Pass());
Indent. The comment is irrelevant.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:937:
pending_credentials_.signon_realm = provisionally_saved_form_->signon_realm;
I lost the track in this function. When is at possible that
pending_credentials_.origin != provisionally_saved_form_->origin /
pending_credentials_.signon_realm != provisionally_saved_form_->signon_realm by
the end of the method?

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

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:257:
scoped_ptr<PasswordForm> match;
|match| is unused.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1255:
simulated_results[0]->signon_realm = "android://hash@com.google.android";
An Android credential is a PSL match or not?

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2229:
saved_match()->password_value + ASCIIToUTF16("1");
I personally like how other test just assign credentials.password_value =
"new_password";

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2248:
// No meta-information should be updated, only the password.
If this is true, may be it's short to use operator==?

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2296:
// No meta-information should be updated, only the password.
If |saved_match()| has all the metadata like the updated password then this can
be shorter:

PasswordForm new_credentials = *saved_match();
new_credentials.password_value = credentials.password_value;
EXPECT_CALL(*mock_store(), UpdateLogin(new_credentials));

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2367:
EXPECT_EQ(credentials.password_value, new_credentials.password_value);
Isn't new_credentials == credentials?

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_store_unittest.cc:551:
!IsValidAndroidFacetURI(result->signon_realm))
Why is this new condition?

[dvadym, 2015-09-28 13:58:41]

Thanks Vasilii. I've addressed your comments. PTAL.

https://codereview.chromium.org/1314903003/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:265: bool
is_public_suffix_match;
On 2015/09/25 14:16:15, vasilii wrote:
> I think that Android credentials used to be a "PSL match". If this is true
(ask
> Balazs) I'd make it an enum.

It was like a hack, using PSL matching to Android. PSL matching of course has
nothing to do with Android credentials.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/login_database.cc:939: if
(form.is_public_suffix_match) {
On 2015/09/25 14:16:15, vasilii wrote:
> Hmm, I think the comment isn't true anymore. The PSL match should be in the
> database and, therefore, can be deleted.

I agree that the comment is not true. But that the question should we allow to
delete it. I've changed comment to
"TODO(dvadym): Discuss whether we should allow to remove PSL matched
credentials."
 and I'll ask this question on team meeting.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:471:
best_matches_.insert(username, login.Pass());
On 2015/09/25 14:16:15, vasilii wrote:
> Indent. The comment is irrelevant.

Done.

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

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:257:
scoped_ptr<PasswordForm> match;
On 2015/09/25 14:16:15, vasilii wrote:
> |match| is unused.

Thanks, removed

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:1255:
simulated_results[0]->signon_realm = "android://hash@com.google.android";
On 2015/09/25 14:16:15, vasilii wrote:
> An Android credential is a PSL match or not?

No, it was just a hack. Android credentials has nothing to do with PSL matching

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2229:
saved_match()->password_value + ASCIIToUTF16("1");
On 2015/09/25 14:16:15, vasilii wrote:
> I personally like how other test just assign credentials.password_value =
> "new_password";

Done.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2248:
// No meta-information should be updated, only the password.
On 2015/09/25 14:16:15, vasilii wrote:
> If this is true, may be it's short to use operator==?

Unfortunetely it doesn't work, since the forms have at least different
|times_used|. And I'd not used ==, since we can introduce in future new fields,
that can be changed with submit.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2296:
// No meta-information should be updated, only the password.
On 2015/09/25 14:16:15, vasilii wrote:
> If |saved_match()| has all the metadata like the updated password then this
can
> be shorter:
> 
> PasswordForm new_credentials = *saved_match();
> new_credentials.password_value = credentials.password_value;
> EXPECT_CALL(*mock_store(), UpdateLogin(new_credentials));
Unfortunetely it doesn't work, since the forms have at least different
|times_used|. And I'd not used ==, since we can introduce in future new fields,
that can be changed with submit.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:2367:
EXPECT_EQ(credentials.password_value, new_credentials.password_value);
On 2015/09/25 14:16:15, vasilii wrote:
> Isn't new_credentials == credentials?

Unfortunetely it doesn't work, since the forms have at least different
|times_used|. And I'd not used ==, since we can introduce in future new fields,
that can be changed with submit.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_store_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_store_unittest.cc:551:
!IsValidAndroidFacetURI(result->signon_realm))
On 2015/09/25 14:16:15, vasilii wrote:
> Why is this new condition?

We should set which expected_results are PSL matched and now Android credentials
are not considered to be PSL matched, so that why this condition added.

[vasilii, 2015-09-29 10:37:34]

A question from password_form_manager.cc is still unanswered.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/login_database.cc:939: if
(form.is_public_suffix_match) {
On 2015/09/28 13:58:40, dvadym wrote:
> On 2015/09/25 14:16:15, vasilii wrote:
> > Hmm, I think the comment isn't true anymore. The PSL match should be in the
> > database and, therefore, can be deleted.
> 
> I agree that the comment is not true. But that the question should we allow to
> delete it. I've changed comment to
> "TODO(dvadym): Discuss whether we should allow to remove PSL matched
> credentials."
>  and I'll ask this question on team meeting.

I think the PSL matches aren't displayed in the password bubble.
- if there are non-PSL matches than they have a higher score and hide a PSL one.
- otherwise, they aren't shown in the bubble.

The Credential Manager API doesn't use PSL. So removing the line seems OK. You
can do it later.

[dvadym, 2015-09-29 13:51:41]

Sorry, I overlooked that question. PTAL.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:937:
pending_credentials_.signon_realm = provisionally_saved_form_->signon_realm;
On 2015/09/25 14:16:15, vasilii wrote:
> I lost the track in this function. When is at possible that
> pending_credentials_.origin != provisionally_saved_form_->origin /
> pending_credentials_.signon_realm != provisionally_saved_form_->signon_realm
by
> the end of the method?

In two places:
1.PSL match, not overridden
2.PSL change password form

[vasilii, 2015-09-29 14:35:22]

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:937:
pending_credentials_.signon_realm = provisionally_saved_form_->signon_realm;
On 2015/09/29 13:51:40, dvadym wrote:
> On 2015/09/25 14:16:15, vasilii wrote:
> > I lost the track in this function. When is at possible that
> > pending_credentials_.origin != provisionally_saved_form_->origin /
> > pending_credentials_.signon_realm != provisionally_saved_form_->signon_realm
> by
> > the end of the method?
> 
> In two places:
> 1.PSL match, not overridden
> 2.PSL change password form

That is, the answer to my question is "Never" because the new block assigns
exactly for PSL.
In this case, remove the "if" condition.

[dvadym, 2015-09-29 15:43:40]

Thanks Vasilii for comments. I've made changes according to offline discussion.
PTAL.

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/260001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:937:
pending_credentials_.signon_realm = provisionally_saved_form_->signon_realm;
On 2015/09/29 14:35:22, vasilii wrote:
> On 2015/09/29 13:51:40, dvadym wrote:
> > On 2015/09/25 14:16:15, vasilii wrote:
> > > I lost the track in this function. When is at possible that
> > > pending_credentials_.origin != provisionally_saved_form_->origin /
> > > pending_credentials_.signon_realm !=
provisionally_saved_form_->signon_realm
> > by
> > > the end of the method?
> > 
> > In two places:
> > 1.PSL match, not overridden
> > 2.PSL change password form
> 
> That is, the answer to my question is "Never" because the new block assigns
> exactly for PSL.
> In this case, remove the "if" condition.

After offline discussion I've moved setting |origin| and |signon_realm| to if
(IsPendingCredentialsPublicSuffixMatch()) { block, since it's the only case
where it's needed

[engedy, 2015-09-29 16:20:46]

Looking good, however, I would definitely split this CL into three parts as it
touches on three completely independent areas:

 1) Deprecate |original_signon_realm| and introduce |is_public_suffix_match|
instead. 

 2) Introduce the behavior that password updates are propagated to all instances
of the same account

 3) The little refactoring of how |best_matches| are constructed in
OnRequestDone().

https://codereview.chromium.org/1314903003/diff/260001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/260001/components/autofill/co...
components/autofill/core/common/password_form.h:265: bool
is_public_suffix_match;
On 2015/09/28 13:58:40, dvadym wrote:
> On 2015/09/25 14:16:15, vasilii wrote:
> > I think that Android credentials used to be a "PSL match". If this is true
> (ask
> > Balazs) I'd make it an enum.
> 
> It was like a hack, using PSL matching to Android. PSL matching of course has
> nothing to do with Android credentials. 

Note, however, that without marking them as PSL matching, the
|original_signon_realm| is, in the current state of the CL, not relayed to the
renderer process to be shown in the pop-up under the fields. We would like to
keep this behavior.

Also, when such an Android credential is filled, we will need to take extra care
to update the |origin| and |signon_realm| for the copy that is going to be
saved.

I think that introducing an enum, as Vasilii suggests, could be a nice way to
solve this. Another option is to not introduce this field at all, and compare
|signon_realm| to that of the observed form, but I am not sure that such a
comparison is always feasible.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form.cc:20: void
PasswordFormToJSON(const PasswordForm& form,
Please also output |is_public_suffix_match| in this method, so that if two
password forms are compared in tests (e.g. EXPECT_EQ), and they differ only on
this attribute, the test output will provide useful information to spot this.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form.h:264: // true if this match was
found using public suffix matching.
comment nit: If true, this match ...

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:161:
EXPECT_EQ(result.preferred_realm, preferred_match.signon_realm);
nit: Swap the order of arguments, so that the expectation is the first one.
Otherwise, the test output in case of a failure will be labeled incorrectly.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:166:
EXPECT_EQ("", iter->second.realm);
nit: Empty string literals are discouraged as they might lead to extra heap
allocations and/or copying of the terminating zero. Instead, use either of the
two:

EXPECT_EQ(std::string(), iter->second.realm);
EXPECT_TRUE(iter->second.realm.empty());

Use the first variant if it can be useful for readers of the test output to know
the value in case the test fails; use the second otherwise.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:171:
EXPECT_EQ(public_suffix_match.signon_realm, iter->second.realm);
nit: Swap.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:172:
EXPECT_TRUE(public_suffix_match.is_public_suffix_match);
This should be iter->second.is_public_suffix_match.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/save_password_progress_logger.cc:83:
log.SetBoolean(GetStringFromID(STRING_IS_PUBLIC_SUFFIX_MATCH),
Is there a reason to output the value of |form.is_public_suffix_match| twice?

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:454: if
(best_match_username == best_matches_.end()) {
This change could be performed in its own, separate CL.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:526:
(!IsValidAndroidFacetURI(preferred_match_->signon_realm) &&
This line, at least, can now be removed.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:696: // If
this was password update then update all non-best matches entries with
Okay, I'd definitely split this into another CL too (or make the CL at hand
contain only this).

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:862:
password_overridden_ = false;
Hang on a minute, I think |password_overridden| is a positive signal that the
prompt should be shown, so I don't think we should set this to false.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:921: if
(pending_credentials_.is_public_suffix_match) {
nit: What is the reason to have this here, and not after line 815? That way, it
looks like we could avoid duplication on lines 863--865.

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

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:191:
};
Personally, I'd introduce a

  typedef int ResultOfSimulatedMatchingMask;

and use that for the type of the second argument of SimulateMatchingPhase.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:212:
psl_saved_match_.origin =
The |signon_realm| and |action| should be set to different values too. Otherwise
the tests will not catch if those are not updated after a PSL matching
credential is filled.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:246:
void SimulateMatchingPhase(PasswordFormManager* p, uint result) {
nit: Avoid 'uint' as it is a non-standard type. Prefer basic types and those
defined in <stdint.h>. As all bitmasks here are very small, I don't think we
need to use a type with a fixed width (i.e. 'int' will do as opposed to
'int32_t').

Also, in this case, I am not sure if we need an unsigned type at all. Note that,
by default, unscoped enumeration values without a fixed underlying type will get
integral-promoted to int (not unsigned int), and bitwise operations do not care
whether the type is signed, so in my opinion using a signed type for bitmasks is
not all that so evil.

But if Vasilii feels strongly about it I am happy either way. :-)

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:523:
TEST_F(PasswordFormManagerTest,
Hmm, this test only exists is to verify that the user will be prompted after
overwriting the password for PSL matched credential. The behavior is now broken
and this test still passes. :-)

Could you please add a check for |password_overridden()| for now, and file a bug
about this?

In the long run, we should either move the boolean magic from
PasswordManager::ShouldPromptUserToSavePassword() to here instead, or add tests
for that function, i.e., on a slightly higher integration level, so that we
would verify something meaningful instead of *some* arbitrary chosen parts of a
tightly coupled logic.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:527:
SimulateMatchingPhase(form_manager(), RESULT_SAVED_MATCH);
Can we use our shiny new enum value, i.e., RESULT_PSL_MATCH, and remove the line
above?

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:544:
SimulateMatchingPhase(form_manager(), RESULT_SAVED_MATCH);
Ditto here: Can we use RESULT_PSL_MATCH and remove the line above?

If we changed the |psl_match()| as pointed out above, that would also allow this
test to verify that the |signon_realm| and |origin| of a PSL match are also
correctly updated before the new login is saved. I think this currently not
verified as these attributes are the same for all forms that the test involves.

[vasilii, 2015-09-29 16:56:16]

lgtm

OK, my comments are addressed.

[dvadym, 2015-09-29 18:29:36]

Thanks Balasz for review! I've addressed your comments

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form.cc:20: void
PasswordFormToJSON(const PasswordForm& form,
On 2015/09/29 16:20:45, engedy wrote:
> Please also output |is_public_suffix_match| in this method, so that if two
> password forms are compared in tests (e.g. EXPECT_EQ), and they differ only on
> this attribute, the test output will provide useful information to spot this.

Done.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form.h:264: // true if this match was
found using public suffix matching.
On 2015/09/29 16:20:45, engedy wrote:
> comment nit: If true, this match ...

Done.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:161:
EXPECT_EQ(result.preferred_realm, preferred_match.signon_realm);
On 2015/09/29 16:20:45, engedy wrote:
> nit: Swap the order of arguments, so that the expectation is the first one.
> Otherwise, the test output in case of a failure will be labeled incorrectly.

Done.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:166:
EXPECT_EQ("", iter->second.realm);
On 2015/09/29 16:20:45, engedy wrote:
> nit: Empty string literals are discouraged as they might lead to extra heap
> allocations and/or copying of the terminating zero. Instead, use either of the
> two:
> 
> EXPECT_EQ(std::string(), iter->second.realm);
> EXPECT_TRUE(iter->second.realm.empty());
> 
> Use the first variant if it can be useful for readers of the test output to
know
> the value in case the test fails; use the second otherwise.

Done.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:171:
EXPECT_EQ(public_suffix_match.signon_realm, iter->second.realm);
On 2015/09/29 16:20:45, engedy wrote:
> nit: Swap.

Done.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:172:
EXPECT_TRUE(public_suffix_match.is_public_suffix_match);
On 2015/09/29 16:20:45, engedy wrote:
> This should be iter->second.is_public_suffix_match.

Thanks for pointing this. iter->second contains info for autofilling, and there
are no information whether it was PSL. So this line should be removed

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/save_password_progress_logger.cc:83:
log.SetBoolean(GetStringFromID(STRING_IS_PUBLIC_SUFFIX_MATCH),
On 2015/09/29 16:20:45, engedy wrote:
> Is there a reason to output the value of |form.is_public_suffix_match| twice?

Thanks, it's left after refactoring.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:454: if
(best_match_username == best_matches_.end()) {
On 2015/09/29 16:20:45, engedy wrote:
> This change could be performed in its own, separate CL.

Yeah, agree. But it's too late to split them :(

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:526:
(!IsValidAndroidFacetURI(preferred_match_->signon_realm) &&
On 2015/09/29 16:20:45, engedy wrote:
> This line, at least, can now be removed.

After offline discussion we understood that it should be left.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:862:
password_overridden_ = false;
On 2015/09/29 16:20:45, engedy wrote:
> Hang on a minute, I think |password_overridden| is a positive signal that the
> prompt should be shown, so I don't think we should set this to false.

Settings |password_overridded| to false here makes credentils to be saved with
Save Bubble, not to show Update bubble.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:921: if
(pending_credentials_.is_public_suffix_match) {
On 2015/09/29 16:20:45, engedy wrote:
> nit: What is the reason to have this here, and not after line 815? That way,
it
> looks like we could avoid duplication on lines 863--865.

Yeah, it was moved there in PatchSet 816

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

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:191:
};
On 2015/09/29 16:20:46, engedy wrote:
> Personally, I'd introduce a
> 
>   typedef int ResultOfSimulatedMatchingMask;
> 
> and use that for the type of the second argument of SimulateMatchingPhase.

Thanks, make sense

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:212:
psl_saved_match_.origin =
On 2015/09/29 16:20:45, engedy wrote:
> The |signon_realm| and |action| should be set to different values too.
Otherwise
> the tests will not catch if those are not updated after a PSL matching
> credential is filled.

Thanks, it makes sense

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:246:
void SimulateMatchingPhase(PasswordFormManager* p, uint result) {
On 2015/09/29 16:20:45, engedy wrote:
> nit: Avoid 'uint' as it is a non-standard type. Prefer basic types and those
> defined in <stdint.h>. As all bitmasks here are very small, I don't think we
> need to use a type with a fixed width (i.e. 'int' will do as opposed to
> 'int32_t').
> 
> Also, in this case, I am not sure if we need an unsigned type at all. Note
that,
> by default, unscoped enumeration values without a fixed underlying type will
get
> integral-promoted to int (not unsigned int), and bitwise operations do not
care
> whether the type is signed, so in my opinion using a signed type for bitmasks
is
> not all that so evil.
> 
> But if Vasilii feels strongly about it I am happy either way. :-)

Done.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:523:
TEST_F(PasswordFormManagerTest,
On 2015/09/29 16:20:45, engedy wrote:
> Hmm, this test only exists is to verify that the user will be prompted after
> overwriting the password for PSL matched credential. The behavior is now
broken
> and this test still passes. :-)
> 
> Could you please add a check for |password_overridden()| for now, and file a
bug
> about this?
> 
> In the long run, we should either move the boolean magic from
> PasswordManager::ShouldPromptUserToSavePassword() to here instead, or add
tests
> for that function, i.e., on a slightly higher integration level, so that we
> would verify something meaningful instead of *some* arbitrary chosen parts of
a
> tightly coupled logic.

I'm not sure that I understood why behavior is broken. In this case |overridden|
== false and so Save bubble is shown. It's exactly what is tested in this test.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:527:
SimulateMatchingPhase(form_manager(), RESULT_SAVED_MATCH);
On 2015/09/29 16:20:45, engedy wrote:
> Can we use our shiny new enum value, i.e., RESULT_PSL_MATCH, and remove the
line
> above?

Thanks

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:544:
SimulateMatchingPhase(form_manager(), RESULT_SAVED_MATCH);
On 2015/09/29 16:20:45, engedy wrote:
> Ditto here: Can we use RESULT_PSL_MATCH and remove the line above?
> 
> If we changed the |psl_match()| as pointed out above, that would also allow
this
> test to verify that the |signon_realm| and |origin| of a PSL match are also
> correctly updated before the new login is saved. I think this currently not
> verified as these attributes are the same for all forms that the test
involves.

Done.

[engedy, 2015-09-29 19:11:04]

LGTM % comments.  Most importantly, w.r.t. |password_overridden|, you are right
that this value controls whether to show the Save Bubble or the Update Bubble,
however, in the special case of PSL matches, I think no bubble will be shown
whatsoever if it this flag is false as "IsNewLogin" is false too. Please
double-check.

It's fine to address nits in a follow up CL.

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data_unittest.cc
(right):

https://codereview.chromium.org/1314903003/diff/280001/components/autofill/co...
components/autofill/core/common/password_form_fill_data_unittest.cc:172:
EXPECT_TRUE(public_suffix_match.is_public_suffix_match);
On 2015/09/29 18:29:36, dvadym wrote:
> On 2015/09/29 16:20:45, engedy wrote:
> > This should be iter->second.is_public_suffix_match.
> 
> Thanks for pointing this. iter->second contains info for autofilling, and
there
> are no information whether it was PSL. So this line should be removed

Ah, right, I was getting confused about the types.

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:862:
password_overridden_ = false;
On 2015/09/29 18:29:36, dvadym wrote:
> On 2015/09/29 16:20:45, engedy wrote:
> > Hang on a minute, I think |password_overridden| is a positive signal that
the
> > prompt should be shown, so I don't think we should set this to false.
> 
> Settings |password_overridded| to false here makes credentils to be saved with
> Save Bubble, not to show Update bubble.

That's true, but if I am not mistaken, in addition to that, in this special
case, it also controls whether *any* kind of bubble is shown in the first place
(or just the credential is saved without further ado), because this is not
classified as a new login.

See: PasswordManager::ShouldPromptUserToSavePassword().

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:921: if
(pending_credentials_.is_public_suffix_match) {
On 2015/09/29 18:29:36, dvadym wrote:
> On 2015/09/29 16:20:45, engedy wrote:
> > nit: What is the reason to have this here, and not after line 815? That way,
> it
> > looks like we could avoid duplication on lines 863--865.
> 
> Yeah, it was moved there in PatchSet 816

Acknowledged.

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

https://codereview.chromium.org/1314903003/diff/280001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:523:
TEST_F(PasswordFormManagerTest,
On 2015/09/29 18:29:36, dvadym wrote:
> On 2015/09/29 16:20:45, engedy wrote:
> > Hmm, this test only exists is to verify that the user will be prompted after
> > overwriting the password for PSL matched credential. The behavior is now
> broken
> > and this test still passes. :-)
> > 
> > Could you please add a check for |password_overridden()| for now, and file a
> bug
> > about this?
> > 
> > In the long run, we should either move the boolean magic from
> > PasswordManager::ShouldPromptUserToSavePassword() to here instead, or add
> tests
> > for that function, i.e., on a slightly higher integration level, so that we
> > would verify something meaningful instead of *some* arbitrary chosen parts
of
> a
> > tightly coupled logic.
> 
> I'm not sure that I understood why behavior is broken. In this case
|overridden|
> == false and so Save bubble is shown. It's exactly what is tested in this
test.

Please see my other comment.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.cc:24:
target->SetBoolean("is_public_suffix_match", form.is_public_suffix_match);
nit: Don't forget about |is_affiliated| either.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.cc:126: is_public_suffix_match ==
form.is_public_suffix_match;
I think we might want to test |is_affiliated| here too.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.h:264: // if true this match was
found using public suffix matching.
nit: "If true, " -- so with capital and comma, to be consistent with the above.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.h:267: // if true this form is
affiliated with Android credentials.
naming nit: |is_affiliation_based_match|
phrasing nit: If true, this is a credential saved through an Android
application, and found using affiliation-based match.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data.cc (right):

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form_fill_data.cc:70: if
(iter->second->is_public_suffix_match)
I think we need "|| is_affiliated" here too.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form_fill_data.cc:83: if
(iter->second->is_public_suffix_match)
I would add it here too just in case, but I don't think will ever have
other_possible_usernames for Android credentials.

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:526:
(!IsValidAndroidFacetURI(preferred_match_->original_signon_realm) &&
I think this should be just |signon_realm|, and is causing the trybot failures.

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:817: // If the
autofilled credentials were a PSL match or Android federated
phrasing suggestion:

// ... or credentials saved from Android apps, store ... 


[Note: These are traditional username/password credentials saved through Android
apps. Federated credentials (be from apps or websites) should never reach this
point. (They currently do, but I am working on fixing that. :-))]

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

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:216:
psl_saved_match_.signon_realm = "http://m.accounts.google.com";
Note: Usually, signon_realm has a trailing slash. If there are any strange test
failures here, expect that to be the cause.

[dvadym, 2015-09-30 09:49:06]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2015-09-30 09:49:07]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
vasilii@chromium.org, engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/1314903003/#ps380001
(title: "bot fix")

[commit-bot: I haz the power, 2015-09-30 09:49:49]

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

[commit-bot: I haz the power, 2015-09-30 13:06:49]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-09-30 13:07:50]

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

[dvadym, 2015-10-12 09:30:45]

Thanks Balasz for code review! Your comments were addressed in CL
https://codereview.chromium.org/1401723002

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.cc:24:
target->SetBoolean("is_public_suffix_match", form.is_public_suffix_match);
On 2015/09/29 19:11:04, engedy wrote:
> nit: Don't forget about |is_affiliated| either.

Done, in the following CL.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.cc:126: is_public_suffix_match ==
form.is_public_suffix_match;
On 2015/09/29 19:11:04, engedy wrote:
> I think we might want to test |is_affiliated| here too.

Done, in the following CL.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
File components/autofill/core/common/password_form.h (right):

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.h:264: // if true this match was
found using public suffix matching.
On 2015/09/29 19:11:04, engedy wrote:
> nit: "If true, " -- so with capital and comma, to be consistent with the
above. 

Done, in the following patch set.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form.h:267: // if true this form is
affiliated with Android credentials.
On 2015/09/29 19:11:04, engedy wrote:
> naming nit: |is_affiliation_based_match|
> phrasing nit: If true, this is a credential saved through an Android
> application, and found using affiliation-based match.

Done, in the following patch set.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
File components/autofill/core/common/password_form_fill_data.cc (right):

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form_fill_data.cc:70: if
(iter->second->is_public_suffix_match)
On 2015/09/29 19:11:04, engedy wrote:
> I think we need "|| is_affiliated" here too.

Done in the following CL.

https://codereview.chromium.org/1314903003/diff/360001/components/autofill/co...
components/autofill/core/common/password_form_fill_data.cc:83: if
(iter->second->is_public_suffix_match)
On 2015/09/29 19:11:04, engedy wrote:
> I would add it here too just in case, but I don't think will ever have
> other_possible_usernames for Android credentials.

Done in the following CL.

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:526:
(!IsValidAndroidFacetURI(preferred_match_->original_signon_realm) &&
On 2015/09/29 19:11:04, engedy wrote:
> I think this should be just |signon_realm|, and is causing the trybot
failures.

Done.

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:817: // If the
autofilled credentials were a PSL match or Android federated
On 2015/09/29 19:11:04, engedy wrote:
> phrasing suggestion:
> 
> // ... or credentials saved from Android apps, store ... 
> 
> 
> [Note: These are traditional username/password credentials saved through
Android
> apps. Federated credentials (be from apps or websites) should never reach this
> point. (They currently do, but I am working on fixing that. :-))]

Done.

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

https://codereview.chromium.org/1314903003/diff/360001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:216:
psl_saved_match_.signon_realm = "http://m.accounts.google.com";
On 2015/09/29 19:11:04, engedy wrote:
> Note: Usually, signon_realm has a trailing slash. If there are any strange
test
> failures here, expect that to be the cause.

In this file, almost all signon_realm are wihtout "/".