PSL matched credentials with user-overwritten password are no longer PSL matched

PSL matched credentials with user-overwritten password are no longer PSL matched

If the user accepts a PSL-suggested credential, but then replaces the password, the provisionally saved credentials should no longer be identified as PSL-matched. As a side effect, the user will be prompted before saving them (accepted PSL-matched suggestions are saved automatically by design).

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

[vabr (Chromium), 2014-07-23 14:56:31]

Hi Balázs,

Could you please have a look?

Cheers,
Vaclav

[Garrett Casto, 2014-07-23 19:09:02]

Driveby: Do we really want to do this? Right now if a user chooses a username to
autofill (PSL or not) and changes their password they are not prompted to save
again, since they already gave consent. The PSL case is different on the
backend, but I'm not sure if users would think about this differently. Of
course, this is a pretty small corner case so I'm not sure how much it matters,
just wanted to know what your thinking is.

[vabr (Chromium), 2014-07-24 07:34:59]

On 2014/07/23 19:09:02, Garrett Casto wrote:
> Driveby: Do we really want to do this? Right now if a user chooses a username
to
> autofill (PSL or not) and changes their password they are not prompted to save
> again, since they already gave consent. The PSL case is different on the
> backend, but I'm not sure if users would think about this differently. Of
> course, this is a pretty small corner case so I'm not sure how much it
matters,
> just wanted to know what your thinking is.

Thanks for the drive-by, Garrett.

The bug http://crbug.com/385619, and a couple of others I saw in the past,
highlighted that cases where users have separate credentials for PSL-equivalent
domains (user.example.com vs. admin.example.com) are not that uncommon. In those
cases users often use the same username (because it's the same person), but a
different password (different roles of the account).

Now imagine that the user is fine with saving their password on
user.example.com, but not on admin.example.com. Currently they have no way to do
this -- typing the (common) username in on admin triggers the PSL-matched
suggestion. The user changes the password to be able to log in, and Chrome
"updates" the credentials by saving a new entry for admin.example.com.

In other words, my rule of thumb here is -- for a new login credential on a
particular PSL-equivalence set of domains, we should ask.
And a credential with a unique password is new.

WDYT?

Cheers,
Vaclav

[Garrett Casto, 2014-07-24 21:13:19]

On 2014/07/24 07:34:59, vabr (Chromium) wrote:
> On 2014/07/23 19:09:02, Garrett Casto wrote:
> > Driveby: Do we really want to do this? Right now if a user chooses a
username
> to
> > autofill (PSL or not) and changes their password they are not prompted to
save
> > again, since they already gave consent. The PSL case is different on the
> > backend, but I'm not sure if users would think about this differently. Of
> > course, this is a pretty small corner case so I'm not sure how much it
> matters,
> > just wanted to know what your thinking is.
> 
> Thanks for the drive-by, Garrett.
> 
> The bug http://crbug.com/385619, and a couple of others I saw in the past,
> highlighted that cases where users have separate credentials for
PSL-equivalent
> domains (http://user.example.com vs. http://admin.example.com) are not that
uncommon. In those
> cases users often use the same username (because it's the same person), but a
> different password (different roles of the account).
> 
> Now imagine that the user is fine with saving their password on
> http://user.example.com, but not on http://admin.example.com. Currently they
have no way to do
> this -- typing the (common) username in on admin triggers the PSL-matched
> suggestion. The user changes the password to be able to log in, and Chrome
> "updates" the credentials by saving a new entry for http://admin.example.com.
> 
> In other words, my rule of thumb here is -- for a new login credential on a
> particular PSL-equivalence set of domains, we should ask.
> And a credential with a unique password is new.
> 
> WDYT?
> 

I see. It's a little strange that doing this as two separate steps would not
trigger a popup, but I can see why you would want this. The alternative case
where you wouldn't want to prompt (same account, password is just out of date)
probably happens less frequently.

Relatedly, if the user doesn't ever want to save the password for admin, they
are still going to have to deal with the overwriting the PSL password match
every time. Not sure if there is anything to do about that, 

In any case, this seems reasonable, though you might want to comment in the code
some of the reasoning that you expressed here :).

> Cheers,
> Vaclav

[vabr (Chromium), 2014-07-25 08:39:11]

Garrett, Balázs,
I added some comments based on Garrett's response. PTAL.
Answers to Garrett's points are inline.

On 2014/07/24 21:13:19, Garrett Casto wrote:
> On 2014/07/24 07:34:59, vabr (Chromium) wrote:
> > On 2014/07/23 19:09:02, Garrett Casto wrote:
> > > Driveby: Do we really want to do this? Right now if a user chooses a
> username
> > to
> > > autofill (PSL or not) and changes their password they are not prompted to
> save
> > > again, since they already gave consent. The PSL case is different on the
> > > backend, but I'm not sure if users would think about this differently. Of
> > > course, this is a pretty small corner case so I'm not sure how much it
> > matters,
> > > just wanted to know what your thinking is.
> > 
> > Thanks for the drive-by, Garrett.
> > 
> > The bug http://crbug.com/385619, and a couple of others I saw in the past,
> > highlighted that cases where users have separate credentials for
> PSL-equivalent
> > domains (http://user.example.com vs. http://admin.example.com) are not that
> uncommon. In those
> > cases users often use the same username (because it's the same person), but
a
> > different password (different roles of the account).
> > 
> > Now imagine that the user is fine with saving their password on
> > http://user.example.com, but not on http://admin.example.com. Currently they
> have no way to do
> > this -- typing the (common) username in on admin triggers the PSL-matched
> > suggestion. The user changes the password to be able to log in, and Chrome
> > "updates" the credentials by saving a new entry for
http://admin.example.com.
> > 
> > In other words, my rule of thumb here is -- for a new login credential on a
> > particular PSL-equivalence set of domains, we should ask.
> > And a credential with a unique password is new.
> > 
> > WDYT?
> > 
> 
> I see. It's a little strange that doing this as two separate steps would not
> trigger a popup, but I can see why you would want this. The alternative case
> where you wouldn't want to prompt (same account, password is just out of date)
> probably happens less frequently.
Just to make sure -- you do speak here about the update of PSL-matched creds,
not updates in general, don't you? If so, then I agree, and tried to make that
explicit in the code comment.

> 
> Relatedly, if the user doesn't ever want to save the password for admin, they
> are still going to have to deal with the overwriting the PSL password match
> every time. Not sure if there is anything to do about that, 
Yes, that's a bit unfortunate. I'm not sure how we treat site blacklists with
PSL. Will try that one out once the code compiles.

> 
> In any case, this seems reasonable, though you might want to comment in the
code
> some of the reasoning that you expressed here :).
> 
> > Cheers,
> > Vaclav

Thanks!
Vaclav

[vabr (Chromium), 2014-07-25 10:37:06]

On 2014/07/25 08:39:11, vabr (Chromium) wrote:
> Garrett, Balázs,
> I added some comments based on Garrett's response. PTAL.
> Answers to Garrett's points are inline.
> 
> On 2014/07/24 21:13:19, Garrett Casto wrote:
> > On 2014/07/24 07:34:59, vabr (Chromium) wrote:
> > > On 2014/07/23 19:09:02, Garrett Casto wrote:
> > > > Driveby: Do we really want to do this? Right now if a user chooses a
> > username
> > > to
> > > > autofill (PSL or not) and changes their password they are not prompted
to
> > save
> > > > again, since they already gave consent. The PSL case is different on the
> > > > backend, but I'm not sure if users would think about this differently.
Of
> > > > course, this is a pretty small corner case so I'm not sure how much it
> > > matters,
> > > > just wanted to know what your thinking is.
> > > 
> > > Thanks for the drive-by, Garrett.
> > > 
> > > The bug http://crbug.com/385619, and a couple of others I saw in the past,
> > > highlighted that cases where users have separate credentials for
> > PSL-equivalent
> > > domains (http://user.example.com vs. http://admin.example.com) are not
that
> > uncommon. In those
> > > cases users often use the same username (because it's the same person),
but
> a
> > > different password (different roles of the account).
> > > 
> > > Now imagine that the user is fine with saving their password on
> > > http://user.example.com, but not on http://admin.example.com. Currently
they
> > have no way to do
> > > this -- typing the (common) username in on admin triggers the PSL-matched
> > > suggestion. The user changes the password to be able to log in, and Chrome
> > > "updates" the credentials by saving a new entry for
> http://admin.example.com.
> > > 
> > > In other words, my rule of thumb here is -- for a new login credential on
a
> > > particular PSL-equivalence set of domains, we should ask.
> > > And a credential with a unique password is new.
> > > 
> > > WDYT?
> > > 
> > 
> > I see. It's a little strange that doing this as two separate steps would not
> > trigger a popup, but I can see why you would want this. The alternative case
> > where you wouldn't want to prompt (same account, password is just out of
date)
> > probably happens less frequently.
> Just to make sure -- you do speak here about the update of PSL-matched creds,
> not updates in general, don't you? If so, then I agree, and tried to make that
> explicit in the code comment.
> 
> > 
> > Relatedly, if the user doesn't ever want to save the password for admin,
they
> > are still going to have to deal with the overwriting the PSL password match
> > every time. Not sure if there is anything to do about that, 
> Yes, that's a bit unfortunate. I'm not sure how we treat site blacklists with
> PSL. Will try that one out once the code compiles.

It turns out that blacklisting works great, because it does not do PSL match.
So if somebody does want the password saved on subdomain1, but not subdomain2,
that person only needs to say "Never save passwords" on subdomain2. The
passwords on subdomain1 will continue to be autofilled (and updated), but on
subdomain2, nothing will be shown. This all is assuming the fix from this patch
being present (otherwise there is no way to say "Never"). Which is another
reason for landing it. :)

> 
> > 
> > In any case, this seems reasonable, though you might want to comment in the
> code
> > some of the reasoning that you expressed here :).
> > 
> > > Cheers,
> > > Vaclav
> 
> Thanks!
> Vaclav

[engedy, 2014-07-25 11:09:00]

LGTM modulo comments. Glad to hear that blacklisting works!

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

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:260:
is_new_login_ = IsPendingCredentialsPublicSuffixMatch();
I think we should refactor lines 260--288. All here seems to apply only for PSL
matches, so we could just put the whole thing into a
if(IsPendingCredentialsPublicSuffixMatch()) { ... }.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:267: // Unmark
the pending credentials as being a PSL match. This has an effect
Given that this covers both the cases of a different and same password, we
should move the comment out of the {}.

We should also split it into paragraphs, like so:
 * What happens normally for PSL matches and why.
 * What happens when the user changes the password for PSL matches and why.
 * What is the drawback of the latter, and why it is less important.

We can even summarize in the end with your nice concise rule of thumb.

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

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:252:
// If PSL-matched credentials were suggested, and the user overwrites the
nit: Suggestion:

If PSL-matched credentials had been suggested, but the user has overwritten the
password, we should no longer consider the provisionally saved credentials as
PSL-matched, so that the exception for not prompting before saving PSL-matched
credentials should no longer apply.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:255:
TEST_F(PasswordFormManagerTest, TestOverridenPSLLogin) {
nit: The test name should include both the scenario tested and the expected
result. My suggestions:
  "PromptBeforeSavingOverridenPSLMatchedCredentials" or
  "OverridenPSLMatchedCredentialsShouldTriggerPrompt".

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:257:
saved_match()->origin = observed_form()->origin;
Can we just either remove this, or set it to an origin that actually needs to be
PSL matched?

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:258:
PasswordForm credentials(*saved_match());
Let us make this instead a copy of *observed_form(), so it shares the |origin|
with that, and move this down to just before line 265.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:262:
SimulateMatchingPhase(&manager, /*find_match=*/true);
nit: true /*find_match*/, or just drop the comment.

[vabr (Chromium), 2014-07-28 13:29:57]

Thanks, Balázs.

PTAL, I addressed your comments.

Cheers,
Vaclav

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

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:260:
is_new_login_ = IsPendingCredentialsPublicSuffixMatch();
On 2014/07/25 11:09:00, engedy wrote:
> I think we should refactor lines 260--288. All here seems to apply only for
PSL
> matches, so we could just put the whole thing into a
> if(IsPendingCredentialsPublicSuffixMatch()) { ... }.

Done.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:267: // Unmark
the pending credentials as being a PSL match. This has an effect
On 2014/07/25 11:09:00, engedy wrote:
> Given that this covers both the cases of a different and same password, we
> should move the comment out of the {}.
> 
> We should also split it into paragraphs, like so:
>  * What happens normally for PSL matches and why.
>  * What happens when the user changes the password for PSL matches and why.
>  * What is the drawback of the latter, and why it is less important.
> 
> We can even summarize in the end with your nice concise rule of thumb.

Done.

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

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:252:
// If PSL-matched credentials were suggested, and the user overwrites the
On 2014/07/25 11:09:00, engedy wrote:
> nit: Suggestion:
> 
> If PSL-matched credentials had been suggested, but the user has overwritten
the
> password, we should no longer consider the provisionally saved credentials as
> PSL-matched, so that the exception for not prompting before saving PSL-matched
> credentials should no longer apply.

Done + changed we into passive voice.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:255:
TEST_F(PasswordFormManagerTest, TestOverridenPSLLogin) {
On 2014/07/25 11:09:00, engedy wrote:
> nit: The test name should include both the scenario tested and the expected
> result. My suggestions:
>   "PromptBeforeSavingOverridenPSLMatchedCredentials" or
>   "OverridenPSLMatchedCredentialsShouldTriggerPrompt".

I agree with naming both the scenario and the expectation, but I'd like to avoid
mentioning the prompt -- that's beyond what this test can do. It only verifies
that the resulting credentials are not marked as PSL. If that triggers the
prompt or not is decided later. Please check the new name if you like it.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:257:
saved_match()->origin = observed_form()->origin;
On 2014/07/25 11:09:00, engedy wrote:
> Can we just either remove this, or set it to an origin that actually needs to
be
> PSL matched?

Removed. That was a relic from when I was tuning the test to pass :), sorry
about that.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:258:
PasswordForm credentials(*saved_match());
On 2014/07/25 11:09:00, engedy wrote:
> Let us make this instead a copy of *observed_form(), so it shares the |origin|
> with that, and move this down to just before line 265.

Done.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:262:
SimulateMatchingPhase(&manager, /*find_match=*/true);
On 2014/07/25 11:09:00, engedy wrote:
> nit: true /*find_match*/, or just drop the comment.

The style of adding the variable names comment actually varies. I usually stick
to what Jeffrey Yasskin once recommended to me:
https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...

I also don't want to leave it out, because I find the call unclear.

To avoid a clash between your and Jeffrey's suggestion, I opted into changing
the bool into an enum, with more explanatory value names.

[engedy, 2014-07-29 09:31:21]

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

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:252:
// If PSL-matched credentials were suggested, and the user overwrites the
On 2014/07/28 13:29:56, vabr (Chromium) wrote:
> On 2014/07/25 11:09:00, engedy wrote:
> > nit: Suggestion:
> > 
> > If PSL-matched credentials had been suggested, but the user has overwritten
> the
> > password, we should no longer consider the provisionally saved credentials
as
> > PSL-matched, so that the exception for not prompting before saving
PSL-matched
> > credentials should no longer apply.
> 
> Done + changed we into passive voice.

SGTM, thanks'

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:255:
TEST_F(PasswordFormManagerTest, TestOverridenPSLLogin) {
On 2014/07/28 13:29:56, vabr (Chromium) wrote:
> On 2014/07/25 11:09:00, engedy wrote:
> > nit: The test name should include both the scenario tested and the expected
> > result. My suggestions:
> >   "PromptBeforeSavingOverridenPSLMatchedCredentials" or
> >   "OverridenPSLMatchedCredentialsShouldTriggerPrompt".
> 
> I agree with naming both the scenario and the expectation, but I'd like to
avoid
> mentioning the prompt -- that's beyond what this test can do. It only verifies
> that the resulting credentials are not marked as PSL. If that triggers the
> prompt or not is decided later. Please check the new name if you like it.

You are right, we should not include the concept of the prompt in here.  New
name sounds good modulo s/Overriden/Overridden/.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:262:
SimulateMatchingPhase(&manager, /*find_match=*/true);
On 2014/07/28 13:29:57, vabr (Chromium) wrote:
> On 2014/07/25 11:09:00, engedy wrote:
> > nit: true /*find_match*/, or just drop the comment.
> 
> The style of adding the variable names comment actually varies. I usually
stick
> to what Jeffrey Yasskin once recommended to me:
>
https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
> 
> I also don't want to leave it out, because I find the call unclear.
> 
> To avoid a clash between your and Jeffrey's suggestion, I opted into changing
> the bool into an enum, with more explanatory value names.

I do not perceive a "clash" here, because, well... both were "suggestions". I
think you should feel free to go with what you prefer in these cases. :-)

The enum looks nice, though.

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

https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:261: // store
them so they can automatically be filled in later.
nit: I would elaborate here and explain that we are storing a copy of them with
the current domain as the origin, so that ...

https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:265: //
Normally, PSL matched credentials are saved automatically, without
nit: I think it is important to get this text right, so let us go over it in
person once again. FFR, my proposal for the first para:

Normally, the copy of the PSL matched credentials for the current domain is
saved automatically, without asking the user, as they likely represent the same
account as the source credentials, for which the user already agreed to store a
password.

[vabr (Chromium), 2014-07-29 14:46:00]

Thanks, Balázs,
For your off-line help with the comments.

Please take a look at the final result.

Cheers,
Vaclav

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

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:255:
TEST_F(PasswordFormManagerTest, TestOverridenPSLLogin) {
On 2014/07/29 09:31:21, engedy wrote:
> On 2014/07/28 13:29:56, vabr (Chromium) wrote:
> > On 2014/07/25 11:09:00, engedy wrote:
> > > nit: The test name should include both the scenario tested and the
expected
> > > result. My suggestions:
> > >   "PromptBeforeSavingOverridenPSLMatchedCredentials" or
> > >   "OverridenPSLMatchedCredentialsShouldTriggerPrompt".
> > 
> > I agree with naming both the scenario and the expectation, but I'd like to
> avoid
> > mentioning the prompt -- that's beyond what this test can do. It only
verifies
> > that the resulting credentials are not marked as PSL. If that triggers the
> > prompt or not is decided later. Please check the new name if you like it.
> 
> You are right, we should not include the concept of the prompt in here.  New
> name sounds good modulo s/Overriden/Overridden/.

Done.

https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:262:
SimulateMatchingPhase(&manager, /*find_match=*/true);
On 2014/07/29 09:31:21, engedy wrote:
> On 2014/07/28 13:29:57, vabr (Chromium) wrote:
> > On 2014/07/25 11:09:00, engedy wrote:
> > > nit: true /*find_match*/, or just drop the comment.
> > 
> > The style of adding the variable names comment actually varies. I usually
> stick
> > to what Jeffrey Yasskin once recommended to me:
> >
>
https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
> > 
> > I also don't want to leave it out, because I find the call unclear.
> > 
> > To avoid a clash between your and Jeffrey's suggestion, I opted into
changing
> > the bool into an enum, with more explanatory value names.
> 
> I do not perceive a "clash" here, because, well... both were "suggestions". I
> think you should feel free to go with what you prefer in these cases. :-)
> 
> The enum looks nice, though.

Acknowledged.

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

https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:261: // store
them so they can automatically be filled in later.
On 2014/07/29 09:31:21, engedy wrote:
> nit: I would elaborate here and explain that we are storing a copy of them
with
> the current domain as the origin, so that ...

Done.

https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:265: //
Normally, PSL matched credentials are saved automatically, without
On 2014/07/29 09:31:21, engedy wrote:
> nit: I think it is important to get this text right, so let us go over it in
> person once again. FFR, my proposal for the first para:
> 
> Normally, the copy of the PSL matched credentials for the current domain is
> saved automatically, without asking the user, as they likely represent the
same
> account as the source credentials, for which the user already agreed to store
a
> password.

Done.

[engedy, 2014-07-29 17:13:00]

On 2014/07/29 14:46:00, vabr (Chromium) wrote:
> Thanks, Balázs,
> For your off-line help with the comments.
> 
> Please take a look at the final result.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
> File
components/password_manager/core/browser/password_form_manager_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
>
components/password_manager/core/browser/password_form_manager_unittest.cc:255:
> TEST_F(PasswordFormManagerTest, TestOverridenPSLLogin) {
> On 2014/07/29 09:31:21, engedy wrote:
> > On 2014/07/28 13:29:56, vabr (Chromium) wrote:
> > > On 2014/07/25 11:09:00, engedy wrote:
> > > > nit: The test name should include both the scenario tested and the
> expected
> > > > result. My suggestions:
> > > >   "PromptBeforeSavingOverridenPSLMatchedCredentials" or
> > > >   "OverridenPSLMatchedCredentialsShouldTriggerPrompt".
> > > 
> > > I agree with naming both the scenario and the expectation, but I'd like to
> > avoid
> > > mentioning the prompt -- that's beyond what this test can do. It only
> verifies
> > > that the resulting credentials are not marked as PSL. If that triggers the
> > > prompt or not is decided later. Please check the new name if you like it.
> > 
> > You are right, we should not include the concept of the prompt in here.  New
> > name sounds good modulo s/Overriden/Overridden/.
> 
> Done.
> 
>
https://codereview.chromium.org/413733002/diff/20001/components/password_mana...
>
components/password_manager/core/browser/password_form_manager_unittest.cc:262:
> SimulateMatchingPhase(&manager, /*find_match=*/true);
> On 2014/07/29 09:31:21, engedy wrote:
> > On 2014/07/28 13:29:57, vabr (Chromium) wrote:
> > > On 2014/07/25 11:09:00, engedy wrote:
> > > > nit: true /*find_match*/, or just drop the comment.
> > > 
> > > The style of adding the variable names comment actually varies. I usually
> > stick
> > > to what Jeffrey Yasskin once recommended to me:
> > >
> >
>
https://codereview.chromium.org/52743002/diff/420001/chrome/browser/extension...
> > > 
> > > I also don't want to leave it out, because I find the call unclear.
> > > 
> > > To avoid a clash between your and Jeffrey's suggestion, I opted into
> changing
> > > the bool into an enum, with more explanatory value names.
> > 
> > I do not perceive a "clash" here, because, well... both were "suggestions".
I
> > think you should feel free to go with what you prefer in these cases. :-)
> > 
> > The enum looks nice, though.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
> File components/password_manager/core/browser/password_form_manager.cc
(right):
> 
>
https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
> components/password_manager/core/browser/password_form_manager.cc:261: //
store
> them so they can automatically be filled in later.
> On 2014/07/29 09:31:21, engedy wrote:
> > nit: I would elaborate here and explain that we are storing a copy of them
> with
> > the current domain as the origin, so that ...
> 
> Done.
> 
>
https://codereview.chromium.org/413733002/diff/70001/components/password_mana...
> components/password_manager/core/browser/password_form_manager.cc:265: //
> Normally, PSL matched credentials are saved automatically, without
> On 2014/07/29 09:31:21, engedy wrote:
> > nit: I think it is important to get this text right, so let us go over it in
> > person once again. FFR, my proposal for the first para:
> > 
> > Normally, the copy of the PSL matched credentials for the current domain is
> > saved automatically, without asking the user, as they likely represent the
> same
> > account as the source credentials, for which the user already agreed to
store
> a
> > password.
> 
> Done.

LGTM, thanks for your patience with the phrasing!

[vabr (Chromium), 2014-07-30 08:56:03]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-07-30 08:58:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/413733002/130001

[commit-bot: I haz the power, 2014-07-30 10:09:32]

Change committed as 286465