Add metadata to content::PasswordForm to keep track of the password usage.

Add metadata to content::PasswordForm to keep track of the password usage.

The motivating force behind this change is we want to ping the first time
a generated password is used. If we need to keep information about first use,
we might as well keep total usage stats as it is an interesting UMA statistic 
as well.

Changes to the other PasswordStore implementations will be in separate CLs

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

[Garrett Casto, 2013-05-14 06:07:32]

isherman@ - Overall change
mdm@ - LoginDatabase changes
joi@ - Content changes

[Ilya Sherman, 2013-05-14 07:25:34]

LGTM

https://codereview.chromium.org/14811010/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/14811010/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_form_manager.cc:452:
pending_credentials_.times_used++;
nit: Please use the pre-increment operator rather than the post-increment one.

https://codereview.chromium.org/14811010/diff/1/content/public/common/passwor...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/14811010/diff/1/content/public/common/passwor...
content/public/common/password_form.h:164: int times_used;
nit: size_t?

[Mike Mammarella, 2013-05-14 09:45:56]

I haven't looked at the actual CL yet, but have you considered keeping a "last
used" timestamp instead of a usage count? You could still figure out when the
password is used for the first time, but you'd also be able to sort by last used
date, as just requested in issue 238798.

[Jói, 2013-05-14 18:16:58]

Is times_used_ opaque to code in //content or will there be follow-up changes
where it gets used within //content?

[Garrett Casto, 2013-05-14 20:09:55]

On 2013/05/14 09:45:56, Mike Mammarella wrote:
> I haven't looked at the actual CL yet, but have you considered keeping a "last
> used" timestamp instead of a usage count? You could still figure out when the
> password is used for the first time, but you'd also be able to sort by last
used
> date, as just requested in issue 238798.

So both will allow us to tell if a password has been used before, but I think
that I would prefer adding times_used for now. Adding last_used would only give
extra benefit if we change the UI of the password manager to expose it in some
way, and that's not something that I can get to anytime soon. Adding times_used
will give us additional UMA data right now. I probably want that data regardless
to see the difference in usage patterns for generated and normal passwords.

[Garrett Casto, 2013-05-14 20:10:24]

On 2013/05/14 18:16:58, Jói wrote:
> Is times_used_ opaque to code in //content or will there be follow-up changes
> where it gets used within //content?

opaque, this member will only be used in //chrome

[Garrett Casto, 2013-05-14 20:17:58]

https://codereview.chromium.org/14811010/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/14811010/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_form_manager.cc:452:
pending_credentials_.times_used++;
On 2013/05/14 07:25:34, Ilya Sherman wrote:
> nit: Please use the pre-increment operator rather than the post-increment one.

Done.

https://codereview.chromium.org/14811010/diff/1/content/public/common/passwor...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/14811010/diff/1/content/public/common/passwor...
content/public/common/password_form.h:164: int times_used;
On 2013/05/14 07:25:34, Ilya Sherman wrote:
> nit: size_t?

I'd prefer not. There is some ambiguity in the chromium style guide about this,
and in general I lean more towards the Google style of avoiding unsigned
integers. I also might want to use a negative number as an indicator for not-set
instead of unused.

[Jói, 2013-05-14 20:40:16]

LGTM

Just to confirm, you don't need to update
content::InitPasswordFormFromWebPasswordForm do you?  It looks to me like the
default constructor setting times_used to 0 is sufficient.

Cheers,
Jói

[Garrett Casto, 2013-05-14 20:41:36]

On 2013/05/14 20:40:16, Jói wrote:
> LGTM
> 
> Just to confirm, you don't need to update
> content::InitPasswordFormFromWebPasswordForm do you?  It looks to me like the
> default constructor setting times_used to 0 is sufficient.
> 

Yep, should work fine as is.

> Cheers,
> Jói

[Mike Mammarella, 2013-05-15 00:08:14]

Still curious about my earlier question.

https://codereview.chromium.org/14811010/diff/8001/chrome/browser/password_ma...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/14811010/diff/8001/chrome/browser/password_ma...
chrome/browser/password_manager/login_database.cc:110: switch (current_version)
{
This seems like it might be a good place to use explicit fallthrough; each block
upgrades to the next version, and you can just fall through to the next upgrade.

[Garrett Casto, 2013-05-15 07:00:04]

On 2013/05/15 00:08:14, Mike Mammarella wrote:
> Still curious about my earlier question.
>

About save last_used_timestamp instead? I already answered that.
 
>
https://codereview.chromium.org/14811010/diff/8001/chrome/browser/password_ma...
> File chrome/browser/password_manager/login_database.cc (right):
> 
>
https://codereview.chromium.org/14811010/diff/8001/chrome/browser/password_ma...
> chrome/browser/password_manager/login_database.cc:110: switch
(current_version)
> {
> This seems like it might be a good place to use explicit fallthrough; each
block
> upgrades to the next version, and you can just fall through to the next
upgrade.

Done. Was debating about this while writing it, and couldn't make up my mind.

[Mike Mammarella, 2013-05-15 21:36:08]

Huh, not sure how I missed your comment. OK.

It occurs to me that I don't know how sync will interact with this (or the
previous change to add the type). What happens if you sync a password that has a
nonzero use count to another profile? Does it have zero use count? Does it look
non-autogenerated?

[gcasto (DO NOT USE), 2013-05-15 22:06:04]

That is a good question. It looks like they have their own representation
of a stored password in PasswordSpecificsData, which I didn't even think
about. I'll see how hard it is to add modify that representation as well.


On Wed, May 15, 2013 at 2:36 PM, <mdm@chromium.org> wrote:

> Huh, not sure how I missed your comment. OK.
>
> It occurs to me that I don't know how sync will interact with this (or the
> previous change to add the type). What happens if you sync a password that
> has a
> nonzero use count to another profile? Does it have zero use count? Does it
> look
> non-autogenerated?
>
>
https://codereview.chromium.**org/14811010/<https://codereview.chromium.org/1...
>

[Garrett Casto, 2013-05-17 18:45:41]

On 2013/05/15 22:06:04, gcasto wrote:
> That is a good question. It looks like they have their own representation
> of a stored password in PasswordSpecificsData, which I didn't even think
> about. I'll see how hard it is to add modify that representation as well.
> 
> 
> On Wed, May 15, 2013 at 2:36 PM, <mailto:mdm@chromium.org> wrote:
> 
> > Huh, not sure how I missed your comment. OK.
> >
> > It occurs to me that I don't know how sync will interact with this (or the
> > previous change to add the type). What happens if you sync a password that
> > has a
> > nonzero use count to another profile? Does it have zero use count? Does it
> > look
> > non-autogenerated?
> >
> >
>
https://codereview.chromium.**org/14811010/%3Chttps://codereview.chromium.org...>
> >

I'm actually in the middle of re-naming one of the newly added fields to
PasswordForm, so do you mind if I make the changes to the sync code in a
different CL so that I only have to do it once?

[Mike Mammarella, 2013-05-18 21:43:07]

On 2013/05/17 18:45:41, Garrett Casto wrote:

> I'm actually in the middle of re-naming one of the newly added fields to
> PasswordForm, so do you mind if I make the changes to the sync code in a
> different CL so that I only have to do it once?

OK, LGTM then.

[commit-bot: I haz the power, 2013-05-28 18:24:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/14811010/59001

[commit-bot: I haz the power, 2013-05-28 18:36:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-28 19:38:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/14811010/79001

[vadimb1, 2013-05-28 19:55:39]

https://codereview.chromium.org/14811010/diff/79001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/14811010/diff/79001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:203:
base::StringPrintf("PasswordManager.Times%sPasswordUsed",
Does this work if the histogram name changes between function call in the same
process run? From the source code of UMA_HISTOGRAM_CUSTOM_COUNTS based on
STATIC_HISTOGRAM_POINTER_BLOCK looks like it checks the name to be constant.
Even if this works it still could be better to have the names explicit when you
just have 2 possible names. This would make code analysis tools (that try to
extract histogram names from the code) work in this case.

[commit-bot: I haz the power, 2013-05-28 20:08:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Garrett Casto, 2013-05-28 21:00:20]

https://codereview.chromium.org/14811010/diff/79001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/14811010/diff/79001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:203:
base::StringPrintf("PasswordManager.Times%sPasswordUsed",
On 2013/05/28 19:55:39, vadimb1 wrote:
> Does this work if the histogram name changes between function call in the same
> process run? From the source code of UMA_HISTOGRAM_CUSTOM_COUNTS based on
> STATIC_HISTOGRAM_POINTER_BLOCK looks like it checks the name to be constant.
> Even if this works it still could be better to have the names explicit when
you
> just have 2 possible names. This would make code analysis tools (that try to
> extract histogram names from the code) work in this case.

Good catch. Changed.

[commit-bot: I haz the power, 2013-05-28 21:08:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/14811010/93001

[vadimb1, 2013-05-28 21:41:37]

lgtm

[commit-bot: I haz the power, 2013-05-28 21:51:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-28 22:51:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/14811010/93001

[commit-bot: I haz the power, 2013-05-29 06:51:58]

Change committed as 202788