[Password Manager] Add UMA stats to track interaction with synced accounts.

[Password Manager] Add UMA stats to track interaction with synced accounts.

Specifically, how often do users save these accounts and how often are they
used.

TBR=rogerta@chromium.org
BUG=386403
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282207

[Garrett Casto, 2014-06-27 07:49:45]

rogerta - Please verify the new DEPS dependency and that I'm using the GAIA and
signup apis correctly.
isherman - UMA stats
vabr - everything else

[vabr (Chromium), 2014-06-27 10:44:14]

LGTM, Thanks!
It will be interesting to learn the statistics from these.

Cheers,
Vaclav

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19079: +    Information about the users
current sync status crossed with if their synced
typo: users -> user's

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43294: +      label="User is not
syncing, but has their sync password store. This
typo: store -> stored

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43303: +      label="User is syncing
passwords but does not have their sync their
typo: "their sync their sync"

[Ilya Sherman, 2014-06-27 19:37:52]

Could you help me understand why it's interesting to separate out the case where
the user is syncing some data, but not passwords?  I believe there are very few
users in this state (I bet the Sync team could hook you up with some specific
stats, if you're interested).  What decision would this particular slice of data
help you make?

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

https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:260: enum
password_manager::SyncState sync_state) {
nit: I don't think you need the "enum" keyword here.

https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:349: default:
Optional nit: I generally prefer to enumerate all the cases explicitly, rather
than using a default stmt, so that if the enum is ever expanded, the compiler
will remind me to think about how the switch stmt should be updated.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19010: +    Number of times a users has
used the password for their sync account if they
nit: "a users" -> "a user"

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19011: +    are syncing passwords.
Please document when this is recorded.  Also, the number of times the user has
used the password since when?

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19064: +<histogram
name="PasswordManager.SyncAccountTimesUsed">
It would be nice to give this histogram (more of) a shared prefix with the one
currently named "PasswordManager.PasswordSyncAccountTimesUsed", so that the two
are adjacent in the dashboard.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19070: +    not password sync.
Same comments as above.  Also, "only histogram only" -> "histogram only".

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43295: +             value should not
happen."/>
These are very long labels, which will not be very readable on the dashboard. 
I'd recommend using briefer labels, and optionally adding description="..."
attributes -- which are shown on mouse hover -- if you think it's useful.

[Garrett Casto, 2014-06-27 21:07:35]

Adding Rachel as she was the one that suggested breaking the stats out this way
(syncing passwords vs. syncing other data). It's unclear to me how useful this
information will be, but she would have a better understanding of the breakdown
of sync users.

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

https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:260: enum
password_manager::SyncState sync_state) {
On 2014/06/27 19:37:51, Ilya Sherman wrote:
> nit: I don't think you need the "enum" keyword here.

Done.

https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:349: default:
On 2014/06/27 19:37:51, Ilya Sherman wrote:
> Optional nit: I generally prefer to enumerate all the cases explicitly, rather
> than using a default stmt, so that if the enum is ever expanded, the compiler
> will remind me to think about how the switch stmt should be updated.

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19010: +    Number of times a users has
used the password for their sync account if they
On 2014/06/27 19:37:52, Ilya Sherman wrote:
> nit: "a users" -> "a user"

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19011: +    are syncing passwords.
On 2014/06/27 19:37:51, Ilya Sherman wrote:
> Please document when this is recorded.  Also, the number of times the user has
> used the password since when?

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19064: +<histogram
name="PasswordManager.SyncAccountTimesUsed">
On 2014/06/27 19:37:51, Ilya Sherman wrote:
> It would be nice to give this histogram (more of) a shared prefix with the one
> currently named "PasswordManager.PasswordSyncAccountTimesUsed", so that the
two
> are adjacent in the dashboard.

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19070: +    not password sync.
On 2014/06/27 19:37:51, Ilya Sherman wrote:
> Same comments as above.  Also, "only histogram only" -> "histogram only".

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19079: +    Information about the users
current sync status crossed with if their synced
On 2014/06/27 10:44:14, vabr (Chromium) wrote:
> typo: users -> user's

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43294: +      label="User is not
syncing, but has their sync password store. This
On 2014/06/27 10:44:14, vabr (Chromium) wrote:
> typo: store -> stored

Done.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43295: +             value should not
happen."/>
On 2014/06/27 19:37:51, Ilya Sherman wrote:
> These are very long labels, which will not be very readable on the dashboard. 
> I'd recommend using briefer labels, and optionally adding description="..."
> attributes -- which are shown on mouse hover -- if you think it's useful.

Shortened.

https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43303: +      label="User is syncing
passwords but does not have their sync their
On 2014/06/27 10:44:14, vabr (Chromium) wrote:
> typo: "their sync their sync"

Done.

[Ilya Sherman, 2014-06-27 21:40:50]

Let me know if you think it would be useful to meet to discuss this metric in
more detail.  I just want to make sure we measure something that'll be able to
successfully guide decisions later on :)

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19059: +    Number of times a users has
used the password for their sync account. Note
nit: "a users" -> "a user"

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19063: +    different profile.
So, this tracks the number of times the user has ever used this password, across
all of time?  That seems like something of a strange thing to track, especially
since there's no accommodation for how long the user has been using Chrome. 
Note also that any metrics recorded on startup bias towards users who relaunch
Chrome more frequently.

I think it would be good to try to clarify what your baseline for this metric
is, and then try to design the histogram to include the baseline.  For example,
perhaps you're interested in comparing the total number of Gaia logins to the
number of Gaia logins with the synced account's password (but presumably only
for users who are using Sync).  Or, perhaps you're interested in measuring a
rate of use, i.e. how many times per week was a synced account's password used? 
(Note, though, that measuring a rate is much harder to do without introducing
bias.)

Incidentally, this histogram <summary> does not currently make this clear, but I
think you're measuring the number of times the password was autofilled -- is
that true?  Perhaps what you want to compare is the frequency with which a Sync
account's password is autofilled vs. entered manually?

[rpop, 2014-06-27 21:41:47]

On 2014/06/27 21:07:35, Garrett Casto wrote:
> Adding Rachel as she was the one that suggested breaking the stats out this
way
> (syncing passwords vs. syncing other data). It's unclear to me how useful this
> information will be, but she would have a better understanding of the
breakdown
> of sync users.

The most important thing is to measure the group that is syncing passwords and
has the sync cred stored. If the user is syncing, but not syncing passwords, and
has the cred stored they are at less risk but will still be inconvenienced if we
disable autofill for the sync cred. I was curious about the size of this
population, but agree that it's not strictly necessary to know it. Removing that
group is ok with me.

> 
>
https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
> File components/password_manager/core/browser/login_database.cc (right):
> 
>
https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
> components/password_manager/core/browser/login_database.cc:260: enum
> password_manager::SyncState sync_state) {
> On 2014/06/27 19:37:51, Ilya Sherman wrote:
> > nit: I don't think you need the "enum" keyword here.
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/components/password_mana...
> components/password_manager/core/browser/login_database.cc:349: default:
> On 2014/06/27 19:37:51, Ilya Sherman wrote:
> > Optional nit: I generally prefer to enumerate all the cases explicitly,
rather
> > than using a default stmt, so that if the enum is ever expanded, the
compiler
> > will remind me to think about how the switch stmt should be updated.
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19010: +    Number of times a users
has
> used the password for their sync account if they
> On 2014/06/27 19:37:52, Ilya Sherman wrote:
> > nit: "a users" -> "a user"
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19011: +    are syncing passwords.
> On 2014/06/27 19:37:51, Ilya Sherman wrote:
> > Please document when this is recorded.  Also, the number of times the user
has
> > used the password since when?
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19064: +<histogram
> name="PasswordManager.SyncAccountTimesUsed">
> On 2014/06/27 19:37:51, Ilya Sherman wrote:
> > It would be nice to give this histogram (more of) a shared prefix with the
one
> > currently named "PasswordManager.PasswordSyncAccountTimesUsed", so that the
> two
> > are adjacent in the dashboard.
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19070: +    not password sync.
> On 2014/06/27 19:37:51, Ilya Sherman wrote:
> > Same comments as above.  Also, "only histogram only" -> "histogram only".
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19079: +    Information about the
users
> current sync status crossed with if their synced
> On 2014/06/27 10:44:14, vabr (Chromium) wrote:
> > typo: users -> user's
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:43294: +      label="User is not
> syncing, but has their sync password store. This
> On 2014/06/27 10:44:14, vabr (Chromium) wrote:
> > typo: store -> stored
> 
> Done.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:43295: +             value should not
> happen."/>
> On 2014/06/27 19:37:51, Ilya Sherman wrote:
> > These are very long labels, which will not be very readable on the
dashboard. 
> > I'd recommend using briefer labels, and optionally adding description="..."
> > attributes -- which are shown on mouse hover -- if you think it's useful.
> 
> Shortened.
> 
>
https://codereview.chromium.org/355143006/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:43303: +      label="User is syncing
> passwords but does not have their sync their
> On 2014/06/27 10:44:14, vabr (Chromium) wrote:
> > typo: "their sync their sync"
> 
> Done.

[rpop, 2014-06-27 21:44:09]

On 2014/06/27 21:40:50, Ilya Sherman wrote:
> Let me know if you think it would be useful to meet to discuss this metric in
> more detail.  I just want to make sure we measure something that'll be able to
> successfully guide decisions later on :)
> 
>
https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19059: +    Number of times a users
has
> used the password for their sync account. Note
> nit: "a users" -> "a user"
> 
>
https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:19063: +    different profile.
> So, this tracks the number of times the user has ever used this password,
across
> all of time?  That seems like something of a strange thing to track,
especially
> since there's no accommodation for how long the user has been using Chrome. 
> Note also that any metrics recorded on startup bias towards users who relaunch
> Chrome more frequently.

Number of times the password is autofilled per week would be most useful, IMO.
Number of times password is manually typed (and not autofilled) would also be
useful.

Maybe we should meet to discuss.

> 
> I think it would be good to try to clarify what your baseline for this metric
> is, and then try to design the histogram to include the baseline.  For
example,
> perhaps you're interested in comparing the total number of Gaia logins to the
> number of Gaia logins with the synced account's password (but presumably only
> for users who are using Sync).  Or, perhaps you're interested in measuring a
> rate of use, i.e. how many times per week was a synced account's password
used? 
> (Note, though, that measuring a rate is much harder to do without introducing
> bias.)
> 
> Incidentally, this histogram <summary> does not currently make this clear, but
I
> think you're measuring the number of times the password was autofilled -- is
> that true?  Perhaps what you want to compare is the frequency with which a
Sync
> account's password is autofilled vs. entered manually?

[Garrett Casto, 2014-06-27 22:33:21]

I'm fine with chatting for a few minutes if there are still questions. Probably
easier to work out that way.

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19063: +    different profile.
On 2014/06/27 21:40:49, Ilya Sherman wrote:
> So, this tracks the number of times the user has ever used this password,
across
> all of time?  That seems like something of a strange thing to track,
especially
> since there's no accommodation for how long the user has been using Chrome. 
> Note also that any metrics recorded on startup bias towards users who relaunch
> Chrome more frequently.
> 

By "using Chrome" are you talking about passwords that were imported from other
browsers? We don't start tracking this until after the password is saved in
Chrome, so that's not an issue. It's true that this doesn't take into account
how many of those usages happened when the user was syncing vs. before they were
syncing. I was trying to be conservative and possibly over count because it's
still going to be a pain point for users if they only use this password every
few months, which we might not see if we only pay attention to logins from this
point on.

As for the bias, we had talked about solving this on the server side before, but
it doesn't look like there has been any movement on that. Are there any better
ways that you can think of to remove this bias, short of trying to report only
once per day (which we've already decided is pretty painful)?

> I think it would be good to try to clarify what your baseline for this metric
> is, and then try to design the histogram to include the baseline.  For
example,
> perhaps you're interested in comparing the total number of Gaia logins to the
> number of Gaia logins with the synced account's password (but presumably only
> for users who are using Sync).  Or, perhaps you're interested in measuring a
> rate of use, i.e. how many times per week was a synced account's password
used? 
> (Note, though, that measuring a rate is much harder to do without introducing
> bias.)

I think that there are two questions we want answered.

1) What percentage of the population would be affected by not autofilling sync
credentials
2) How painful would this be for that group of users.

1) should be answered by SyncingAccountState (modulo reporting biases), so this
is trying to answer 2). My assumption was that any time a user loses this
ability it's going to be a pain point. That is, it's not clear to me that it
matters what percentage of logins are affected so much as total number.
Measuring a rate might be interesting, though that requires syncing additional
metadata about when logins occur. You could also accomplish this server side
with appropriate logs analysis I suppose.

> 
> Incidentally, this histogram <summary> does not currently make this clear, but
I
> think you're measuring the number of times the password was autofilled -- is
> that true?  Perhaps what you want to compare is the frequency with which a
Sync
> account's password is autofilled vs. entered manually?

It's actually how often it's autofilled and then successfully submitted. I can
update the histogram once we've finalized what's going on here. As for the later
point, if the user has their password saved but is still typing it, either we
have a bug somewhere or they are changing their password. It's not obvious to me
that paying attention to password change is useful for answer the questions that
we have, but I haven't thought about it to much. Maybe it's useful to know
because if the rate is high then users will be less likely to be upset if they
end up locked out because they already reset their password a lot? Not sure.

[Ilya Sherman, 2014-06-27 23:24:55]

I think one or both of us are missing what the other is trying to say.  I'll
schedule a quick meeting so we can more easily discuss :)

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19063: +    different profile.
On 2014/06/27 22:33:20, Garrett Casto wrote:
> On 2014/06/27 21:40:49, Ilya Sherman wrote:
> > So, this tracks the number of times the user has ever used this password,
> across
> > all of time?  That seems like something of a strange thing to track,
> especially
> > since there's no accommodation for how long the user has been using Chrome. 
> > Note also that any metrics recorded on startup bias towards users who
relaunch
> > Chrome more frequently.
> > 
> 
> By "using Chrome" are you talking about passwords that were imported from
other
> browsers? We don't start tracking this until after the password is saved in
> Chrome, so that's not an issue. It's true that this doesn't take into account
> how many of those usages happened when the user was syncing vs. before they
were
> syncing. I was trying to be conservative and possibly over count because it's
> still going to be a pain point for users if they only use this password every
> few months, which we might not see if we only pay attention to logins from
this
> point on.

I mean that a user who just started using Chrome last week will, on average,
have fewer Gaia logins than a user who's been using Chrome for a year, assuming
the two users log in equally often.  So, measuring total logins across all time
seems weird for this reason.

> As for the bias, we had talked about solving this on the server side before,
but
> it doesn't look like there has been any movement on that. Are there any better
> ways that you can think of to remove this bias, short of trying to report only
> once per day (which we've already decided is pretty painful)?

It's pretty hard to entirely remove bias, but you can typically reduce it by
picking a baseline for your metric, and then sampling both the metric and its
baseline at the same time.  Absolute numbers are very hard to interpret
properly.  Relative metrics are much easier to work with.

> > I think it would be good to try to clarify what your baseline for this
metric
> > is, and then try to design the histogram to include the baseline.  For
> example,
> > perhaps you're interested in comparing the total number of Gaia logins to
the
> > number of Gaia logins with the synced account's password (but presumably
only
> > for users who are using Sync).  Or, perhaps you're interested in measuring a
> > rate of use, i.e. how many times per week was a synced account's password
> used? 
> > (Note, though, that measuring a rate is much harder to do without
introducing
> > bias.)
> 
> I think that there are two questions we want answered.
> 
> 1) What percentage of the population would be affected by not autofilling sync
> credentials
> 2) How painful would this be for that group of users.
> 
> 1) should be answered by SyncingAccountState (modulo reporting biases), so
this
> is trying to answer 2). My assumption was that any time a user loses this
> ability it's going to be a pain point. That is, it's not clear to me that it
> matters what percentage of logins are affected so much as total number.
> Measuring a rate might be interesting, though that requires syncing additional
> metadata about when logins occur. You could also accomplish this server side
> with appropriate logs analysis I suppose.

I guarantee you that the absolute number will be nonzero.  How do you evaluate
whether an absolute number is significant or not?  I claim that you'll need to
decide in the context of some baseline metric.

> > Incidentally, this histogram <summary> does not currently make this clear,
but
> I
> > think you're measuring the number of times the password was autofilled -- is
> > that true?  Perhaps what you want to compare is the frequency with which a
> Sync
> > account's password is autofilled vs. entered manually?
> 
> It's actually how often it's autofilled and then successfully submitted. I can
> update the histogram once we've finalized what's going on here. As for the
later
> point, if the user has their password saved but is still typing it, either we
> have a bug somewhere or they are changing their password. It's not obvious to
me
> that paying attention to password change is useful for answer the questions
that
> we have, but I haven't thought about it to much. Maybe it's useful to know
> because if the rate is high then users will be less likely to be upset if they
> end up locked out because they already reset their password a lot? Not sure.

Well, a Sync user might have their password saved, or they might not.  The
manual case would apply if they do not.

[Garrett Casto, 2014-07-01 07:24:09]

Updated to deal with issues that Ilya and I talked about. In particular,
autofilled sync credentials are recorded as a user action which should give more
specificity and flexibility. I'll need to run some logs analysis to get good
data, but it shouldn't be particularly complicated. 

Please take another look.

[vabr (Chromium), 2014-07-01 08:52:00]

LGTM with one question (in the 2nd comment).

Thanks,
Vaclav

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.cc:329: 2 *
sync_username.empty() + syncing_account_saved,
This type of boolean mixing arithmetic looks almost like a cryptic riddle to me,
but the C++ norm does guarantee [1] that true converts to 1, and not just to any
non-zero number, so I'm fine with that. :)

[1] §4.7/4 from the C++ Standard
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

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

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:538: if
(client_->IsPasswordSyncAccountCredential(
Should this signal also be in PasswordFormManager::SaveAsNewLogin?

[Ilya Sherman, 2014-07-01 19:46:21]

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19076: +</histogram>
Hmm, do you still need these two metrics, now that you've added the action and
are planning to write custom server-side scripts?  I'm concerned that these
metrics will be very hard to use, and hence either of ver low value, or possibly
even of negative value if somebody tries to use them without deeply
understanding what biases can contribute here.

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19083: +    Information about the user's
current sync status crossed with if their
nit: IMO "crossed with if" -> "crossed with whether" (and you could probably
drop the "or not" at the end of the sentence).

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43296: +  <int value="0" label="User is
not syncing."/>
nit: I'd write this label as "Not Syncing" to match the format of the other
labels.

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43298: +      label="Not Syncing/Pasword
saved. This value should not happen."/>
nit: I'd use either "Sync password" or "Password" consistently throughout.

[Garrett Casto, 2014-07-04 07:27:49]

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
components/password_manager/core/browser/login_database.cc:329: 2 *
sync_username.empty() + syncing_account_saved,
On 2014/07/01 08:52:00, vabr (Chromium) wrote:
> This type of boolean mixing arithmetic looks almost like a cryptic riddle to
me,
> but the C++ norm does guarantee [1] that true converts to 1, and not just to
any
> non-zero number, so I'm fine with that. :)
> 
> [1] §4.7/4 from the C++ Standard
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

Yeah, verified this with the standard before I used it :). I do this in testing
code all the time but I wasn't actually certain it was guaranteed.

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

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:538: if
(client_->IsPasswordSyncAccountCredential(
On 2014/07/01 08:52:00, vabr (Chromium) wrote:
> Should this signal also be in PasswordFormManager::SaveAsNewLogin?

If we are saving a new password then the credential wasn't autofilled. So I
think that we just want to record this in the update case.

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19076: +</histogram>
On 2014/07/01 19:46:21, Ilya Sherman wrote:
> Hmm, do you still need these two metrics, now that you've added the action and
> are planning to write custom server-side scripts?  I'm concerned that these
> metrics will be very hard to use, and hence either of ver low value, or
possibly
> even of negative value if somebody tries to use them without deeply
> understanding what biases can contribute here.

Whoops, removed from the code but forgot to remove here. Done.

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19083: +    Information about the user's
current sync status crossed with if their
On 2014/07/01 19:46:21, Ilya Sherman wrote:
> nit: IMO "crossed with if" -> "crossed with whether" (and you could probably
> drop the "or not" at the end of the sentence).

Done.

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43296: +  <int value="0" label="User is
not syncing."/>
On 2014/07/01 19:46:21, Ilya Sherman wrote:
> nit: I'd write this label as "Not Syncing" to match the format of the other
> labels.

Done.

https://codereview.chromium.org/355143006/diff/80001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:43298: +      label="Not Syncing/Pasword
saved. This value should not happen."/>
On 2014/07/01 19:46:21, Ilya Sherman wrote:
> nit: I'd use either "Sync password" or "Password" consistently throughout.

Done.

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

Still LGTM.

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

https://codereview.chromium.org/355143006/diff/80001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:538: if
(client_->IsPasswordSyncAccountCredential(
On 2014/07/04 07:27:49, Garrett Casto wrote:
> On 2014/07/01 08:52:00, vabr (Chromium) wrote:
> > Should this signal also be in PasswordFormManager::SaveAsNewLogin?
> 
> If we are saving a new password then the credential wasn't autofilled. So I
> think that we just want to record this in the update case.

Ah, right, thanks for explaining that to me.

[Ilya Sherman, 2014-07-07 21:57:17]

LGTM, thanks.

[Ilya Sherman, 2014-07-07 21:57:22]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-07-07 21:58:54]

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

[Garrett Casto, 2014-07-07 22:04:42]

On 2014/07/07 21:58:54, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/gcasto@chromium.org/355143006/100001

Unfortunately Roger is the only OWNER for google_apis but it looks like he's OOO
for the next few weeks. I looked at the dependencies for google_apis and they
look safe (can't depend on content or chrome) so I'm fairly certain it's safe to
depend on here. I'm going to TBR him for now.

[commit-bot: I haz the power, 2014-07-07 23:37:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25594)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30046)

[commit-bot: I haz the power, 2014-07-07 23:51:55]

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

[commit-bot: I haz the power, 2014-07-07 23:51:56]

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

[Garrett Casto, 2014-07-08 06:20:46]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-08 06:22:01]

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

[commit-bot: I haz the power, 2014-07-08 07:43:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/25778)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30227)

[commit-bot: I haz the power, 2014-07-08 07:53:38]

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

[commit-bot: I haz the power, 2014-07-08 07:53:40]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Garrett Casto, 2014-07-08 18:34:23]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-08 18:35:57]

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

[commit-bot: I haz the power, 2014-07-08 21:35:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30547)

[commit-bot: I haz the power, 2014-07-08 21:46:36]

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

[commit-bot: I haz the power, 2014-07-08 21:46:37]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Garrett Casto, 2014-07-08 21:55:58]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-08 21:57:54]

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

[commit-bot: I haz the power, 2014-07-09 01:21:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-09 01:24:38]

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

[commit-bot: I haz the power, 2014-07-09 01:24:39]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Garrett Casto, 2014-07-09 07:32:08]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-09 07:34:19]

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

[commit-bot: I haz the power, 2014-07-09 09:29:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-09 09:42:15]

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

[commit-bot: I haz the power, 2014-07-09 09:42:17]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Garrett Casto, 2014-07-09 17:40:07]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-09 17:41:41]

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

[commit-bot: I haz the power, 2014-07-09 21:29:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Garrett Casto, 2014-07-09 21:41:58]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:44:09]

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

[commit-bot: I haz the power, 2014-07-10 01:18:21]

Change committed as 282207