Track when and how a profile is signed out via UMA.

Track when and how a profile is signed out via UMA.

Note that this is most important for Desktop.

TBR of the several reviewers because their changes are affected broadly by the refactor. Main reviewers are rogerta@ and estade@.

BUG=357693
TBR=tim@chromium.org, noms@chromium.org, joaodasilva@chromium.org, courage@chromium.org, mek@chromium.org

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

[Mike Lerman, 2014-06-17 15:58:19]

Hi reviewers, PTAL.

For tim, estade and nyquist, can you verify that, based on the comments written
in signin_metrics.h and the name of the enumerator, I've correctly documented
what the /chrome/browser code does? Thanks!

asvitkine - histograms.xml
rogerta - /components/signin/
tim - /c/b/sync and c/b/ui/sync
estade - /c/b/ui/webui
nyquist - /c/b/android

Thanks!

[Alexei Svitkine (slow), 2014-06-17 18:13:47]

https://codereview.chromium.org/338993007/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/338993007/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:43506: +    User clicked to signout on
android.
Is there a reason this needs to be separate from "User clicked signout"?

You can already slice UMA data by platform, so why the need for a separate
entry?

[Mike Lerman, 2014-06-17 18:51:50]

https://codereview.chromium.org/338993007/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/338993007/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:43506: +    User clicked to signout on
android.
On 2014/06/17 18:13:47, Alexei Svitkine wrote:
> Is there a reason this needs to be separate from "User clicked signout"?
> 
> You can already slice UMA data by platform, so why the need for a separate
> entry?

I had been thinking that since it's a different code flow... but it's the same
intent, and we can always separate it out later. Change made.

[Alexei Svitkine (slow), 2014-06-17 18:57:46]

histograms lgtm, but I'll let others review the code changes

[Evan Stade, 2014-06-19 01:55:13]

https://codereview.chromium.org/338993007/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/338993007/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:810:
SigninManagerFactory::GetForProfile(GetProfile())->SignOut();
I think you should make SignOut take a parameter (the signout metric), which
ensures that every place SignOut is called, it logs.

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:11: enum ProfileSignout {
what about deleting a profile that's signed in? does that count as sign out?

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:19: // User clicked to signout.
clicked which control? might want to specify

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:28: TRANSFER_CREDENTIALS,
nit: insert newline

[Roger Tawa OOO till Jul 10th, 2014-06-19 14:55:13]

lgtm, agree with Evan that adding an arg to SigninManager::SignOut() would be
safer to catch all cases.

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:11: enum ProfileSignout {
On 2014/06/19 01:55:13, Evan Stade wrote:
> what about deleting a profile that's signed in? does that count as sign out?

It might be possible to track that, but not sure its worth it.  There are other
much more likely ways for a profile to "disappear", like uninstalling chrome, 
user manually deleting the folder, or a profile just never being used again.

[Mike Lerman, 2014-06-19 17:55:48]

Sorry, got a rebase mixed up in there :(
Comments addressed.

https://codereview.chromium.org/338993007/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://codereview.chromium.org/338993007/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/sync_setup_handler.cc:810:
SigninManagerFactory::GetForProfile(GetProfile())->SignOut();
On 2014/06/19 01:55:12, Evan Stade wrote:
> I think you should make SignOut take a parameter (the signout metric), which
> ensures that every place SignOut is called, it logs.

Done.

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:11: enum ProfileSignout {
On 2014/06/19 14:55:13, Roger Tawa wrote:
> On 2014/06/19 01:55:13, Evan Stade wrote:
> > what about deleting a profile that's signed in? does that count as sign out?
> 
> It might be possible to track that, but not sure its worth it.  There are
other
> much more likely ways for a profile to "disappear", like uninstalling chrome, 
> user manually deleting the folder, or a profile just never being used again.

I'm creating another CL to track profile deletions, including differentiating
between when the profile was signed in or not
(https://codereview.chromium.org/345733006/). Along with Roger, I don't think
it's an important category to track for SignOut, but if we do decide it would be
valuable it will be tracked in another histogram.

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:19: // User clicked to signout.
On 2014/06/19 01:55:13, Evan Stade wrote:
> clicked which control? might want to specify

Done.

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:28: TRANSFER_CREDENTIALS,
On 2014/06/19 01:55:13, Evan Stade wrote:
> nit: insert newline

Done... but why? Just to separate out the last counting metric?

[nyquist, 2014-06-19 18:40:25]

chrome/browser/android/ lgtm

[Evan Stade, 2014-06-19 21:33:36]

lgtm

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/338993007/diff/20001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:28: TRANSFER_CREDENTIALS,
On 2014/06/19 17:55:48, Mike Lerman wrote:
> On 2014/06/19 01:55:13, Evan Stade wrote:
> > nit: insert newline
> 
> Done... but why? Just to separate out the last counting metric?

yes

[Mike Lerman, 2014-06-26 13:03:16]

Evan and Roger,

Can I ask for a follow-up review? I've made changes to the unit tests for the
additional parameter. Thanks!

Also note the addition of reviewers in TBR (they are affected only by broad
refactor):
noms - c/b/profiles
joaodasilva - c/b/policy
courage - c/b/extensions/api/identity
mek - c/b/extensions

[Joao da Silva, 2014-06-26 13:08:31]

policy lgtm

[Evan Stade, 2014-06-26 16:17:00]

webui lgtm

[Michael Courage, 2014-06-26 17:12:41]

chrome/browser/extensions/api/identity lgtm

[Roger Tawa OOO till Jul 10th, 2014-06-27 15:45:29]

lgtm

[Mike Lerman, 2014-06-27 15:48:42]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-06-27 15:49:15]

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

[commit-bot: I haz the power, 2014-06-27 17:09:46]

Change committed as 280352