[Password Manager] Add UMA stats for custom passphrase users.

[Password Manager] Add UMA stats for custom passphrase users.

We are trying to determine if the usage pattern of these users is
significantly different than other users.

BUG=417126
Committed: https://crrev.com/e015fd92a519f7c9cbc12ae341e18b650eaab1d3
Cr-Commit-Position: refs/heads/master@{#301167}

[Garrett Casto, 2014-10-10 23:27:51]

gcasto@chromium.org changed reviewers:
+ jww@chromium.org, vabr@chromium.org

[Garrett Casto, 2014-10-10 23:27:55]

vabr@ - Please review
jww@ - Can you verify that these are the type of stats that we need.

[vabr (Chromium), 2014-10-13 09:24:19]

LGTM, with some comments.
Cheers,
Vaclav

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:260: if
(!((state == password_manager::ONLY_CUSTOM_PASSPHRASE_USERS) ^
This is very hard to read, and ^ is a bitwise operator, not a logical one.
Please consider coding this as:

if ((state == password_manager::ONLY_CUSTOM_PASSPHRASE_USERS) ==
sync_service->IsUsingSecondaryPassphrase())

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:377:
EXPECT_FALSE(client->IsPasswordSyncEnabled(
Should you also mock the two possible values of IsUsingSecondaryPassphrase()?

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/login_database.cc:307: std::string
custom_passphrase;
Should we have a non-empty value describing the situation without custom
passphrase value, to avoid mixing the old and the new semantics of the involved
histograms? What I mean: before, AccountsPerSite was reported for all users, now
it will only be reported for users without custom passphrase.

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:23:
EXCLUDE_CUSTOM_PASSPHRASE_USERS,
What about renaming those to "WITHOUT_CUSTOM_PASSPHRASE" and
"WITH_CUSTOM_PASSPHRASE"?

Just from reading the code, I struggle to understand
"IsPasswordSyncEnabled(ONLY_CUSTOM_PASSPHRASE_USERS)" -- there is only one user
in the context. Whereas "IsPasswordSyncEnabled(WITH_CUSTOM_PASSPHRASE)" seems to
be exactly the question answered by the call, if I understand the implementation
correctly.

https://codereview.chromium.org/644053003/diff/300001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/644053003/diff/300001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21773: + 
<owner>dubroy@chromium.org</owner>
I would not add Patrick as an owner, since he appears no longer active in the
password manager development. You can consider replacing his e-mail with yours,
or just dropping this line completely.

The same in the other histograms below.

https://codereview.chromium.org/644053003/diff/300001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21776: +    Same as AccountsPerSite
except distribution only includes users that are
If I understood correctly, AccountsPerSite (the original one) now excludes
custom passphrase users. Should that be mentioned in it's description?
Alternatively, if you address my comment above in the code, about having two new
histogram names to replace AccountsPerSite, feel free to ignore this comment.

The same in the other histograms below.

[jww, 2014-10-13 18:25:35]

Stats lgtm, one comment below.

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/password_generation_manager.cc:53:
DVLOG(2) << "Generation disabled because passwords are not being synced";
Shouldn't this log message actually be "Generation disabled because passwords
are not being synced or this is a custom passphrase user"?

[Garrett Casto, 2014-10-13 20:57:57]

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:260: if
(!((state == password_manager::ONLY_CUSTOM_PASSPHRASE_USERS) ^
On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> This is very hard to read, and ^ is a bitwise operator, not a logical one.
> Please consider coding this as:
> 
> if ((state == password_manager::ONLY_CUSTOM_PASSPHRASE_USERS) ==
> sync_service->IsUsingSecondaryPassphrase())

I actually decided to be even more readable here. and split this out into two
different statements. Hopefully this addresses your comment.

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(right):

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:377:
EXPECT_FALSE(client->IsPasswordSyncEnabled(
On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> Should you also mock the two possible values of IsUsingSecondaryPassphrase()?

Sure, done.

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/login_database.cc:307: std::string
custom_passphrase;
On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> Should we have a non-empty value describing the situation without custom
> passphrase value, to avoid mixing the old and the new semantics of the
involved
> histograms? What I mean: before, AccountsPerSite was reported for all users,
now
> it will only be reported for users without custom passphrase.

Yeah, seems reasonable. I was assuming that there would be a small enough
percentage of these users that it wouldn't appreciably affect the other stats,
but might as well be specific.

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/password_generation_manager.cc:53:
DVLOG(2) << "Generation disabled because passwords are not being synced";
On 2014/10/13 18:25:34, jww wrote:
> Shouldn't this log message actually be "Generation disabled because passwords
> are not being synced or this is a custom passphrase user"?

Done.

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:23:
EXCLUDE_CUSTOM_PASSPHRASE_USERS,
On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> What about renaming those to "WITHOUT_CUSTOM_PASSPHRASE" and
> "WITH_CUSTOM_PASSPHRASE"?
> 
> Just from reading the code, I struggle to understand
> "IsPasswordSyncEnabled(ONLY_CUSTOM_PASSPHRASE_USERS)" -- there is only one
user
> in the context. Whereas "IsPasswordSyncEnabled(WITH_CUSTOM_PASSPHRASE)" seems
to
> be exactly the question answered by the call, if I understand the
implementation
> correctly.

WITH_CUSTOM_PASSPHRASE sounds like it's inclusive (normal + custom passphrase)
which isn't right. Dropping _USERS seems reasonable, so I can imagine either
EXCLUDE_CUSTOM_PASSPHRASE and ONLY_CUSTOM_PASSPHRASE or
WITHOUT_CUSTOM_PASSPHRASE and ONLY_CUSTOM_PASSPHRASE. Preference?

https://codereview.chromium.org/644053003/diff/300001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/644053003/diff/300001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21773: + 
<owner>dubroy@chromium.org</owner>
On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> I would not add Patrick as an owner, since he appears no longer active in the
> password manager development. You can consider replacing his e-mail with
yours,
> or just dropping this line completely.
> 
> The same in the other histograms below.

Done.

https://codereview.chromium.org/644053003/diff/300001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21776: +    Same as AccountsPerSite
except distribution only includes users that are
On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> If I understood correctly, AccountsPerSite (the original one) now excludes
> custom passphrase users. Should that be mentioned in it's description?
> Alternatively, if you address my comment above in the code, about having two
new
> histogram names to replace AccountsPerSite, feel free to ignore this comment.
> 
> The same in the other histograms below.

N/A.

[vabr (Chromium), 2014-10-14 07:32:51]

Hi Garrett,
Seems like the TestPasswordStore::ReportMetricsImpl needs its signature fixed,
but otherwise LGTM.
Thanks!
Vaclav

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/644053003/diff/300001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.cc:260: if
(!((state == password_manager::ONLY_CUSTOM_PASSPHRASE_USERS) ^
On 2014/10/13 20:57:56, Garrett Casto wrote:
> On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> > This is very hard to read, and ^ is a bitwise operator, not a logical one.
> > Please consider coding this as:
> > 
> > if ((state == password_manager::ONLY_CUSTOM_PASSPHRASE_USERS) ==
> > sync_service->IsUsingSecondaryPassphrase())
> 
> I actually decided to be even more readable here. and split this out into two
> different statements. Hopefully this addresses your comment.

The new version looks great. Thanks!

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/644053003/diff/300001/components/password_man...
components/password_manager/core/browser/password_manager_client.h:23:
EXCLUDE_CUSTOM_PASSPHRASE_USERS,
On 2014/10/13 20:57:56, Garrett Casto wrote:
> On 2014/10/13 09:24:18, vabr (Chromium) wrote:
> > What about renaming those to "WITHOUT_CUSTOM_PASSPHRASE" and
> > "WITH_CUSTOM_PASSPHRASE"?
> > 
> > Just from reading the code, I struggle to understand
> > "IsPasswordSyncEnabled(ONLY_CUSTOM_PASSPHRASE_USERS)" -- there is only one
> user
> > in the context. Whereas "IsPasswordSyncEnabled(WITH_CUSTOM_PASSPHRASE)"
seems
> to
> > be exactly the question answered by the call, if I understand the
> implementation
> > correctly.
> 
> WITH_CUSTOM_PASSPHRASE sounds like it's inclusive (normal + custom passphrase)
> which isn't right. Dropping _USERS seems reasonable, so I can imagine either
> EXCLUDE_CUSTOM_PASSPHRASE and ONLY_CUSTOM_PASSPHRASE or
> WITHOUT_CUSTOM_PASSPHRASE and ONLY_CUSTOM_PASSPHRASE. Preference?

Good point with WITH_CUSTOM_PASSPHRASE sounding like it's inclusive.
From the two suggestions you made I prefer WITHOUT_CUSTOM_PASSPHRASE and
ONLY_CUSTOM_PASSPHRASE, simply because
IsPasswordSyncEnabled(WITHOUT_CUSTOM_PASSPHRASE) reads more naturally than
IsPasswordSyncEnabled(EXCLUDE_CUSTOM_PASSPHRASE). But both are acceptable to me.

[Will Harris, 2014-10-14 15:57:08]

see also https://code.google.com/p/chromium/issues/detail?id=421623

the sync team were also going to add an UMA for encryption status.

[Garrett Casto, 2014-10-14 23:43:45]

gcasto@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Garrett Casto, 2014-10-14 23:43:46]

asvitkine@ for histograms changes.

[Alexei Svitkine (slow), 2014-10-15 15:15:31]

https://codereview.chromium.org/644053003/diff/840001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/644053003/diff/840001/components/password_man...
components/password_manager/core/browser/login_database.cc:372: 10);
Instead of repeating these params, how about making a function just to log this
histogram variant.

i.e. LogTimesPasswordUsedStat(name, sample) - that way, the params won't be
repeated and the calling code is simpler.

Also, make another function for the 0,32,6 params above.

https://codereview.chromium.org/644053003/diff/840001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/644053003/diff/840001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22152: +<histogram
name="PasswordManager.TimesPasswordUsedWithCustomPassphrase">
Nit: I suggest delimiting the suffix with a "." to get better grouping on the
dashboard, so this can be:

PasswordManager.TimesPasswordUsed.WithCustomPassphrase

Same for the one below.

Then, you can use the <histogram_suffixes> construct in this file (see docs at
the top of the file) to avoid some repetition.

[Garrett Casto, 2014-10-16 22:52:11]

https://codereview.chromium.org/644053003/diff/840001/components/password_man...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/644053003/diff/840001/components/password_man...
components/password_manager/core/browser/login_database.cc:372: 10);
On 2014/10/15 15:15:31, Alexei Svitkine wrote:
> Instead of repeating these params, how about making a function just to log
this
> histogram variant.
> 
> i.e. LogTimesPasswordUsedStat(name, sample) - that way, the params won't be
> repeated and the calling code is simpler.
> 
> Also, make another function for the 0,32,6 params above.

Done.

https://codereview.chromium.org/644053003/diff/840001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/644053003/diff/840001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22152: +<histogram
name="PasswordManager.TimesPasswordUsedWithCustomPassphrase">
On 2014/10/15 15:15:31, Alexei Svitkine wrote:
> Nit: I suggest delimiting the suffix with a "." to get better grouping on the
> dashboard, so this can be:
> 
> PasswordManager.TimesPasswordUsed.WithCustomPassphrase
> 
> Same for the one below.
> 
> Then, you can use the <histogram_suffixes> construct in this file (see docs at
> the top of the file) to avoid some repetition.

Neato. Done.

[Alexei Svitkine (slow), 2014-10-17 15:55:48]

lgtm, thanks!

[Garrett Casto, 2014-10-17 17:13:40]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-17 17:13:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644053003/880001

[commit-bot: I haz the power, 2014-10-17 17:41:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 17:41:23]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[jww, 2014-10-22 17:18:10]

On 2014/10/17 17:41:23, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

Hey Garrett. What's the status on this CL? I think it's pretty important to get
this in ASAP.

[Garrett Casto, 2014-10-22 17:22:17]

There are some threading issues that I still need to work out. I'll send out an
update later today.

[jww, 2014-10-22 17:33:39]

Great, thanks!

On Wed, Oct 22, 2014 at 10:22 AM, <gcasto@chromium.org> wrote:

> There are some threading issues that I still need to work out. I'll send
> out an
> update later today.
>
> https://codereview.chromium.org/644053003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Garrett Casto, 2014-10-24 00:46:49]

On 2014/10/22 17:33:39, jww wrote:
> Great, thanks!
> 
> On Wed, Oct 22, 2014 at 10:22 AM, <mailto:gcasto@chromium.org> wrote:
> 
> > There are some threading issues that I still need to work out. I'll send
> > out an
> > update later today.
> >
> > https://codereview.chromium.org/644053003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Vaclav, can you take another look now? The previous implementation had threading
issues, so I had to move calls that touched ProfileSyncService from the DB
thread to the UI thread.

[vabr (Chromium), 2014-10-24 06:30:28]

Still LGTM, with one comment.

Cheers,
Vaclav

https://codereview.chromium.org/644053003/diff/980001/components/password_man...
File
components/password_manager/core/browser/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/644053003/diff/980001/components/password_man...
components/password_manager/core/browser/password_generation_manager_unittest.cc:79:
~TestPasswordManagerClient() {
Should this also be 'override'?

[Garrett Casto, 2014-10-24 18:25:03]

https://codereview.chromium.org/644053003/diff/980001/components/password_man...
File
components/password_manager/core/browser/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/644053003/diff/980001/components/password_man...
components/password_manager/core/browser/password_generation_manager_unittest.cc:79:
~TestPasswordManagerClient() {
On 2014/10/24 06:30:28, vabr (Chromium) wrote:
> Should this also be 'override'?

Good catch. Done.

[Garrett Casto, 2014-10-24 18:25:17]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-10-24 18:26:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644053003/1000001

[commit-bot: I haz the power, 2014-10-24 19:20:43]

Committed patchset #16 (id:1000001)

[commit-bot: I haz the power, 2014-10-24 19:21:28]

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