Add Profile metric counting to new User Manager.

Add Profile metric counting to new User Manager.

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

[bcwhite, 2014-01-21 20:09:27]

Does this cover everything you were thinking of?

[noms (inactive), 2014-01-21 20:23:47]

Hmm, I don't think those are all the cases. For example, just at first glance:
- avatar_menu_bubble_view.cc uses ProfileMetrics::ADD_NEW_USER_ICON, but
profile_chooser_view.cc doesn't.
- similarly for avatar_menu_bubble_controller.mm and
profile_chooser_controller.mm on the mac
- The User Manager uses
ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::SWITCH_PROFILE_GUEST), but
you can also launch a guest user from the profile bubble, so that should
probably also show up in profile_chooser_view.cc and the mac equivalent
- I think there should be a different constant for "switching users from the
User Manager", since SWITCH_USER_XXX seems to be pretty granular (and in
particular _ICON is documented as being from the avatar bubble)

[bcwhite, 2014-01-23 16:40:52]

> Hmm, I don't think those are all the cases. For example, just at first glance:
> - avatar_menu_bubble_view.cc uses ProfileMetrics::ADD_NEW_USER_ICON, but
> profile_chooser_view.cc doesn't.

It has a new ADD_NEW_USER_MANAGER.  The description for ADD_NEW_USER_ICON didn't
seem to fit ("User adds new user from icon menu") given that we're now on a new
screen but I can use that if you prefer.

> - similarly for avatar_menu_bubble_controller.mm and
> profile_chooser_controller.mm on the mac

Yes, I've only done the Windows version so far.  I meant to mention that.

> - The User Manager uses
> ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::SWITCH_PROFILE_GUEST),
but
> you can also launch a guest user from the profile bubble, so that should
> probably also show up in profile_chooser_view.cc and the mac equivalent

File updated.

> - I think there should be a different constant for "switching users from the
> User Manager", since SWITCH_USER_XXX seems to be pretty granular (and in
> particular _ICON is documented as being from the avatar bubble)

Makes sense.  That will then match the ADD_NEW_USER reporting.

[noms (inactive), 2014-01-23 17:00:57]

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/profiles...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:27: ADD_NEW_USER_MANAGER,     // User
adds new user from profile-manager
What's the difference between USER_MANAGER and PROFILE_MANAGER? If you mean the
web ui, I think we call that the User Manager (profile manager makes me think of
the profile chooser, which is what we call the avatar bubble. Sigh.) This
comments applies to the below switch_profile_manager

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:174:
ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::OPEN_PROFILE_MANAGER);
I'm not sure whether this should be in here, or in chrome::ShowUserManager().
The user can choose to show the dialog while it's still open, in which case we'd
reuse the instance, the constructor wouldn't get hit, and we wouldn't count it.
Maybe a webui owner can comment?

[bcwhite, 2014-01-23 18:14:05]

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/profiles...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:27: ADD_NEW_USER_MANAGER,     // User
adds new user from profile-manager
Globally changed "profile-manager" to "User Manager".

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/132453004/diff/130001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:174:
ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::OPEN_PROFILE_MANAGER);
Gotcha.  Moved to UserManagerView::Show() for now.

[bcwhite, 2014-01-24 16:08:33]

asvitkine@chromium.org: Please review changes in 
chrome/browser/ui/cocoa/

msw@chromium.org: Please review changes in 
chrome/browser/ui/views/

[Alexei Svitkine (slow), 2014-01-24 16:16:33]

https://codereview.chromium.org/132453004/diff/250001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/132453004/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:205:
ProfileMetrics::LogProfileAddNewUser(ProfileMetrics::ADD_NEW_USER_ICON);
I think it would be better for these to be done within the profile APIs (i.e.
that CreateAndSwitchToNewProfile would call this).

It can be made to take an extra parameter to differentiate the different ways
its invoked.

[bcwhite, 2014-01-28 14:15:27]

On 2014/01/24 16:16:33, Alexei Svitkine wrote:
>
https://codereview.chromium.org/132453004/diff/250001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):
> 
>
https://codereview.chromium.org/132453004/diff/250001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:205:
> ProfileMetrics::LogProfileAddNewUser(ProfileMetrics::ADD_NEW_USER_ICON);
> I think it would be better for these to be done within the profile APIs (i.e.
> that CreateAndSwitchToNewProfile would call this).
> 
> It can be made to take an extra parameter to differentiate the different ways
> its invoked.

Done.

[Alexei Svitkine (slow), 2014-01-28 16:25:30]

Some style comments below.

Also, it seems you forgot to include the corresponding changes to
histograms.xml?

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/profiles...
File chrome/browser/profiles/avatar_menu.cc (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/profiles...
chrome/browser/profiles/avatar_menu.cc:119: size_t index, bool always_create,
ProfileMetrics::ProfileOpen metric) {
If you wrap params, it should be 1 per line.

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:96:
menu_->SwitchToProfile(
Nit: Wrap like this:

  menu_->SwitchToProfile([sender menuIndex], always_create,
                         ProfileMetrics::SWITCH_PROFILE_ICON);

(Only function params, rather than args need to be 1 per line.)

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:212:
avatarMenu_->SwitchToProfile(
Same wrapping comment as above. Please fix throughout (for the SwitchToProfile
call sites).

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:452: bool success,
ProfileMetrics::ProfileAuth auth) {
1 param per line.

[bcwhite, 2014-01-28 17:03:02]

The histogram update is a different CL because all of these metrics are in the
internal histograms file.  I wanted to get this CL approved before I sent out
the other for review.

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/profiles...
File chrome/browser/profiles/avatar_menu.cc (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/profiles...
chrome/browser/profiles/avatar_menu.cc:119: size_t index, bool always_create,
ProfileMetrics::ProfileOpen metric) {
On 2014/01/28 16:25:31, Alexei Svitkine wrote:
> If you wrap params, it should be 1 per line.

Done.

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:96:
menu_->SwitchToProfile(
On 2014/01/28 16:25:31, Alexei Svitkine wrote:
> Nit: Wrap like this:
> 
>   menu_->SwitchToProfile([sender menuIndex], always_create,
>                          ProfileMetrics::SWITCH_PROFILE_ICON);
> 
> (Only function params, rather than args need to be 1 per line.)

Done.

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:212:
avatarMenu_->SwitchToProfile(
There is plenty of existing code (like the call to CreateAndSwitchToNewProfile
directly above) that uses this style.  Why is this one wrong?

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/132453004/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:452: bool success,
ProfileMetrics::ProfileAuth auth) {
On 2014/01/28 16:25:31, Alexei Svitkine wrote:
> 1 param per line.

Done.

[Alexei Svitkine (slow), 2014-01-28 17:26:47]

Can you move the metrics over to the open source histogram file in a separate
CL? Then, this CL can be rebased on top of that to add the metrics in the same
CL.

(Those metrics are in the internal file right now for historical reasons as the
open source file didn't exist at the time.)

[bcwhite, 2014-01-29 16:31:56]

On 2014/01/28 17:26:47, Alexei Svitkine wrote:
> Can you move the metrics over to the open source histogram file in a separate
> CL? Then, this CL can be rebased on top of that to add the metrics in the same
> CL.

Done.

[Alexei Svitkine (slow), 2014-01-29 16:45:22]

lgtm

[commit-bot: I haz the power, 2014-01-29 16:55:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/132453004/440001

[commit-bot: I haz the power, 2014-01-29 17:28:42]

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

[noms (inactive), 2014-01-29 17:33:03]

lgtm

[bcwhite, 2014-01-29 17:34:24]

msw, can you take a look at the /views/ changes?

[msw, 2014-01-29 17:51:57]

LGTM with some minor comments/nits.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/profiles...
File chrome/browser/profiles/profile_list_desktop_browsertest.cc (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/profiles...
chrome/browser/profiles/profile_list_desktop_browsertest.cc:121:
ProfileMetrics::SWITCH_PROFILE_ICON);
optional nit: you could wrap this onto the line above; ditto below.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:672:
ui::DispositionFromEventFlags(event.flags()) == NEW_WINDOW,
optional nit: you could wrap this onto the line above.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:301:
desktop_type_,
optional nit: you could wrap some of these args.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:316:
ReportAuthenticationResult(false, ProfileMetrics::AUTH_ONLINE);
Shouldn't this be AUTH_FAILED?

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:454:
ProfileMetrics::LogProfileAuthResult(auth);
If you correct the above AUTH_ONLINE to AUTH_FAILED in OnClientLoginFailure, you
could move this call outside the if(success) block and remove the separate
LogProfileAuthResult(ProfileMetrics::AUTH_FAILED) call in the else block below.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.h:55: void
ReportAuthenticationResult(bool success, ProfileMetrics::ProfileAuth);
nit: this new parameter should have an identifier.

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

https://codereview.chromium.org/132453004/diff/440001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29607: +  <int value="7" label="Switch
to locked profile via User Manager"/>
nit: make description consistent with the enum's (or vice versa):
  SWITCH_PROFILE_UNLOCK,    // User switches profiles with authentication

[bcwhite, 2014-01-29 19:50:52]

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/profiles...
File chrome/browser/profiles/profile_list_desktop_browsertest.cc (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/profiles...
chrome/browser/profiles/profile_list_desktop_browsertest.cc:121:
ProfileMetrics::SWITCH_PROFILE_ICON);
On 2014/01/29 17:51:58, msw wrote:
> optional nit: you could wrap this onto the line above; ditto below.

Done.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/views...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/views...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:672:
ui::DispositionFromEventFlags(event.flags()) == NEW_WINDOW,
On 2014/01/29 17:51:58, msw wrote:
> optional nit: you could wrap this onto the line above.

Done.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:316:
ReportAuthenticationResult(false, ProfileMetrics::AUTH_ONLINE);
It could be, yes.  Done.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:454:
ProfileMetrics::LogProfileAuthResult(auth);
On 2014/01/29 17:51:58, msw wrote:
> If you correct the above AUTH_ONLINE to AUTH_FAILED in OnClientLoginFailure,
you
> could move this call outside the if(success) block and remove the separate
> LogProfileAuthResult(ProfileMetrics::AUTH_FAILED) call in the else block
below.

Done.

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right):

https://codereview.chromium.org/132453004/diff/440001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.h:55: void
ReportAuthenticationResult(bool success, ProfileMetrics::ProfileAuth);
On 2014/01/29 17:51:58, msw wrote:
> nit: this new parameter should have an identifier.

Done.

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

https://codereview.chromium.org/132453004/diff/440001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29607: +  <int value="7" label="Switch
to locked profile via User Manager"/>
On 2014/01/29 17:51:58, msw wrote:
> nit: make description consistent with the enum's (or vice versa):
>   SWITCH_PROFILE_UNLOCK,    // User switches profiles with authentication

Done.

[msw, 2014-01-29 21:08:47]

LGTM with a nit.

https://codereview.chromium.org/132453004/diff/460001/chrome/browser/profiles...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/132453004/diff/460001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:40: SWITCH_PROFILE_UNLOCK,    // User
switches to lockd profile via User Manager
nit: "locked"

[bcwhite, 2014-01-30 13:58:03]

On 2014/01/29 21:08:47, msw wrote:
> LGTM with a nit.
> 
>
https://codereview.chromium.org/132453004/diff/460001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_metrics.h (right):
> 
>
https://codereview.chromium.org/132453004/diff/460001/chrome/browser/profiles...
> chrome/browser/profiles/profile_metrics.h:40: SWITCH_PROFILE_UNLOCK,    //
User
> switches to lockd profile via User Manager
> nit: "locked"

I tried that.  It exceeded the 80-character line limit.  :-(

[commit-bot: I haz the power, 2014-01-30 13:58:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/132453004/460001

[commit-bot: I haz the power, 2014-01-30 17:41:59]

Change committed as 247979

[commit-bot: I haz the power, 2014-01-30 17:42:03]

CQ bit was unchecked on CL. Ignoring.