UMA Metrics for the user menu, including vasquette-detection

UMA Metrics for the new avatar menu, including vasquette-detection and Add Account, remove Account, profile switch and locking.

For Googlers, a design doc:
https://docs.google.com/a/google.com/document/d/1ybiN1r2xAlMj0PBABH63PJIyVWhsM409xPpZI4zlmck/edit

BUG=357693
TEST=Turn on new-profile-management and optionally fast-user-switcher. Perform the actions listed above and verify results in chrome://histograms

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

[Mike Lerman, 2014-05-13 15:53:27]

Reviewers, PTAL.

noms - c/b/profiles
rogerta - c/b/signin
sky - c/b/ui/*
guohui - overall functionality

Thanks!

[Mike Lerman, 2014-05-13 16:07:00]

+asvitkine - tools/metrics/histograms

Thanks

[sky, 2014-05-13 16:34:36]

https://codereview.chromium.org/288493008/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/288493008/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:426: view_mode =
BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT;
Can you set both view_mode_ and opened_from_header_ in the member initialize
list? Early returns make late binding error prone.

[Mike Lerman, 2014-05-13 16:57:55]

Thanks sky!

https://codereview.chromium.org/288493008/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/288493008/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:426: view_mode =
BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT;
On 2014/05/13 16:34:37, sky wrote:
> Can you set both view_mode_ and opened_from_header_ in the member initialize
> list? Early returns make late binding error prone.

Done.

[sky, 2014-05-13 17:35:42]

LGTM

https://codereview.chromium.org/288493008/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/288493008/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:259: bool
opened_from_header_;
nit: make this const (and likely view_mode_ too).

[Mike Lerman, 2014-05-13 17:52:42]

https://codereview.chromium.org/288493008/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/288493008/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:259: bool
opened_from_header_;
On 2014/05/13 17:35:42, sky wrote:
> nit: make this const (and likely view_mode_ too).

Made this const.

The view_mode_ can change during the lifetime of an object (whenever ShowView()
is called).

[Roger Tawa OOO till Jul 10th, 2014-05-14 14:08:43]

lgtm

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_window.h:371:
AVATAR_BUBBLE_MODE_ACCOUNT_MANAGEMENT_FROM_HEADER
Nit: lets call this FROM_GAIA instead of FROM_HEADER.  We will likely change to
a non-header trigger later, but it will still be gaia doing the triggering.

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:258: // The view was
prompted to open from a GAIA header
Nit: end sentence with period.

[Alexei Svitkine (slow), 2014-05-14 14:15:21]

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:307: metric +
FIRST_VASQUETTE_SIGNOUT_ENUM + 1);
Instead of doing this funny business with a set of parallel enums, would it make
more sense to use a single enum and two different histograms?

e.g. Profile.DesktopSignout and Profile.DesktopSignout.Vasquette or something

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

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.h:100: // including tracking if the
vasquette was the start for user interaction.
What's a vasquette? It doesn't seem to be a real word (according to
define:vasquette in Google).

It would be good to clarify in this comment what this is (or better yet to
describe it in a way that is more clear rather than using this term).

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.h:114: // |FIRST_VASQUETTE_SIGNOUT_ENUM|
should always be the midpoint enum.
How are you going to keep this condition true when adding new entries? Remember,
you can't renumber existing entries in an enum used for UMA.

[Mike Lerman, 2014-05-14 19:46:15]

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:307: metric +
FIRST_VASQUETTE_SIGNOUT_ENUM + 1);
On 2014/05/14 14:15:22, Alexei Svitkine wrote:
> Instead of doing this funny business with a set of parallel enums, would it
make
> more sense to use a single enum and two different histograms?
> 
> e.g. Profile.DesktopSignout and Profile.DesktopSignout.Vasquette or something

Having the data points all in the same histogram will be very useful for
comparing data, so we can see proportionally how people are using the system
depending on which entry point they took.
I don't see any facility for combining/aggregating parallel histograms (that
vary only by suffix)... is there one?

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

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.h:100: // including tracking if the
vasquette was the start for user interaction.
On 2014/05/14 14:15:22, Alexei Svitkine wrote:
> What's a vasquette? It doesn't seem to be a real word (according to
> define:vasquette in Google).
> 
> It would be good to clarify in this comment what this is (or better yet to
> describe it in a way that is more clear rather than using this term).

I had thought Vasquette was a Google-wide term. Apparently not :) I'll change
the wording. Thanks!

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.h:114: // |FIRST_VASQUETTE_SIGNOUT_ENUM|
should always be the midpoint enum.
On 2014/05/14 14:15:22, Alexei Svitkine wrote:
> How are you going to keep this condition true when adding new entries?
Remember,
> you can't renumber existing entries in an enum used for UMA.

I'll take a different approach to ordering these. This is a good point.

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser_window.h:371:
AVATAR_BUBBLE_MODE_ACCOUNT_MANAGEMENT_FROM_HEADER
On 2014/05/14 14:08:44, Roger Tawa wrote:
> Nit: lets call this FROM_GAIA instead of FROM_HEADER.  We will likely change
to
> a non-header trigger later, but it will still be gaia doing the triggering.

Done.

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:258: // The view was
prompted to open from a GAIA header
On 2014/05/14 14:08:44, Roger Tawa wrote:
> Nit: end sentence with period.

Done.

[Alexei Svitkine (slow), 2014-05-14 19:55:04]

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:307: metric +
FIRST_VASQUETTE_SIGNOUT_ENUM + 1);
On 2014/05/14 19:46:15, Mike Lerman wrote:
> On 2014/05/14 14:15:22, Alexei Svitkine wrote:
> > Instead of doing this funny business with a set of parallel enums, would it
> make
> > more sense to use a single enum and two different histograms?
> > 
> > e.g. Profile.DesktopSignout and Profile.DesktopSignout.Vasquette or
something
> 
> Having the data points all in the same histogram will be very useful for
> comparing data, so we can see proportionally how people are using the system
> depending on which entry point they took.
> I don't see any facility for combining/aggregating parallel histograms (that
> vary only by suffix)... is there one?

No, it's not possible to combine them into one. But if you make them have the
same suffix, e.g. Foo.Bar.Bazz and Foo.Bar.Buzz, then you can just enter
Foo.Bar.* into the histograms dashboard and it will show load all the matching
histograms.

[Alexei Svitkine (slow), 2014-05-14 20:04:17]

lgtm % nits

https://codereview.chromium.org/288493008/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:302: DCHECK(metric % 2 == 0);
Nit: DCHECK_EQ?

https://codereview.chromium.org/288493008/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:305: if (opened_from_content) {
Nit: No {}'s

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

https://codereview.chromium.org/288493008/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:20672: +    manager, including whether
the interaction began from the vasquette or the
Nit: Either explain or don't use "vasquette" here.

[Mike Lerman, 2014-05-14 20:09:16]

https://codereview.chromium.org/288493008/diff/90001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:302: DCHECK(metric % 2 == 0);
On 2014/05/14 20:04:17, Alexei Svitkine wrote:
> Nit: DCHECK_EQ?

That would be better. Done.

https://codereview.chromium.org/288493008/diff/90001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:305: if (opened_from_content) {
On 2014/05/14 20:04:17, Alexei Svitkine wrote:
> Nit: No {}'s

Done.

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

https://codereview.chromium.org/288493008/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:20672: +    manager, including whether
the interaction began from the vasquette or the
On 2014/05/14 20:04:17, Alexei Svitkine wrote:
> Nit: Either explain or don't use "vasquette" here.

Thanks for catching this reference.

[guohui, 2014-05-15 21:46:15]

in high level, i think it is better to have two separate Enums, one for
Vasquette actions and one for user menu/manager actions, instead of combining
the two together, since the former needs to be kept in sync with gaia and the
later is solely controlled by Chrome. Also instead of reusing BubbleViewMode, i
think it is better to pass a separate parameter, otherwise either we would lose
info or we would have to make a new enum value for every possible combination of
the vasquete action and user menu/manager action.

[noms (inactive), 2014-05-16 12:52:50]

profiles lgtm

[Mike Lerman, 2014-05-16 14:36:14]

On 2014/05/16 12:52:50, Monica Dinculescu wrote:
> profiles lgtm

Hui, I've split the histograms in two, so we have one enum list which will be
logged into a different histogram depending on the starting point of the user
interaction. You can see that in the latest patch.

Alexei, can you take another look, since I've made some changes to the
histograms.xml and thus also the profile_metrics.*?

I'll take a look at separating out the second parameter to track the various
content-source actions. (GAIA service types).

[Alexei Svitkine (slow), 2014-05-16 14:40:48]

LGTM % comment

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

https://codereview.chromium.org/288493008/diff/150001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40594: +    <summary>User opened the
user menu, and clicked lock.</summary>
int tags don't have summary fields, though the description can be placed in the
int field itself (e.g. see HistoryTopSitesRecoveryEnum)

[Alexei Svitkine (slow), 2014-05-16 17:04:18]

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

https://codereview.chromium.org/288493008/diff/150001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46106: +<histogram_suffixes
name="Profile.DesktopSignout">
Sorry, one more thing - you need to set separator="." here.

[Mike Lerman, 2014-05-22 13:58:33]

Hui,

Can you review this change? I've accounted for the GAIA service types. Once
you've reviewed it, I'll alert the affected owners (alexei and sky) to take a
second look based on the new changes.

Thanks!

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

https://codereview.chromium.org/288493008/diff/150001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40594: +    <summary>User opened the
user menu, and clicked lock.</summary>
On 2014/05/16 14:40:48, Alexei Svitkine wrote:
> int tags don't have summary fields, though the description can be placed in
the
> int field itself (e.g. see HistoryTopSitesRecoveryEnum)

Done.

https://codereview.chromium.org/288493008/diff/150001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46106: +<histogram_suffixes
name="Profile.DesktopSignout">
On 2014/05/16 17:04:18, Alexei Svitkine wrote:
> Sorry, one more thing - you need to set separator="." here.

Done.

[guohui, 2014-05-22 20:20:49]

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

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:111: // User opened the user menu, and
started removing an account.
i think its more interesting to log the actual account removal than showing the
account removal view. Please also see my comment in profile_chooser_view.cc.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:116: };
its also interesting to log clicking "not you" and clicking "settings" link to
disconnect the profile.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_header_helper.cc (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_header_helper.cc:83:
Profile::FromBrowserContext(web_contents->GetBrowserContext()));
we probably want to implement similar tracking logic for android, or at least
please put a TODO here.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_header_helper.h (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_header_helper.h:34: GAIA_SERVICE_TYPE_NONE = 0,    
            // No GAIA response header
nit: missing . at the end.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:708: AvatarBubbleMode mode,
signin::GAIA_SERVICE_TYPE service_type) {
service_type is not used anywhere, please either use it as you do in
profile_chooser_view.cc or add a todo comment.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:429:
gaia_service_type_(service_type) {
gaia_service_type_ is not cleared anywhere, it means that we would log multiple
times if user executes more than one action in the lifetime of the bubble. We
probably want to do it only once.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:640:
ProfileMetrics::PROFILE_DESKTOP_MENU_REMOVE_ACCT, gaia_service_type_);
here we just show the account removal view, the account is not actually deleted,
please move the code to ProfileChooserView::RemoveAccount.

[Mike Lerman, 2014-05-23 17:15:36]

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

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:111: // User opened the user menu, and
started removing an account.
On 2014/05/22 20:20:49, guohui wrote:
> i think its more interesting to log the actual account removal than showing
the
> account removal view. Please also see my comment in profile_chooser_view.cc.

Done.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:116: };
On 2014/05/22 20:20:49, guohui wrote:
> its also interesting to log clicking "not you" and clicking "settings" link to
> disconnect the profile.

I'm planning to do so in a new CL. Should I add a TODO?

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_header_helper.cc (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_header_helper.cc:83:
Profile::FromBrowserContext(web_contents->GetBrowserContext()));
On 2014/05/22 20:20:49, guohui wrote:
> we probably want to implement similar tracking logic for android, or at least
> please put a TODO here.

Done.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_header_helper.h (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_header_helper.h:34: GAIA_SERVICE_TYPE_NONE = 0,    
            // No GAIA response header
On 2014/05/22 20:20:49, guohui wrote:
> nit: missing . at the end.

Done.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:708: AvatarBubbleMode mode,
signin::GAIA_SERVICE_TYPE service_type) {
On 2014/05/22 20:20:49, guohui wrote:
> service_type is not used anywhere, please either use it as you do in
> profile_chooser_view.cc or add a todo comment.

Done.

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:429:
gaia_service_type_(service_type) {
On 2014/05/22 20:20:49, guohui wrote:
> gaia_service_type_ is not cleared anywhere, it means that we would log
multiple
> times if user executes more than one action in the lifetime of the bubble. We
> probably want to do it only once.

There is not real default, we would need to reset it to _BUTTON or
_GAIA_DEFAULT, neither of which seems appropriate. Is it not interesting to note
that GAIA thought X action should take place, but both Y and Z were results?

https://codereview.chromium.org/288493008/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:640:
ProfileMetrics::PROFILE_DESKTOP_MENU_REMOVE_ACCT, gaia_service_type_);
On 2014/05/22 20:20:49, guohui wrote:
> here we just show the account removal view, the account is not actually
deleted,
> please move the code to ProfileChooserView::RemoveAccount.

Done.

[Mike Lerman, 2014-05-23 19:22:11]

Hui,

The service type is now cleared in the profile_chooser_view after each
action.

[guohui, 2014-05-23 19:28:45]

lgtm, thanks!

[Mike Lerman, 2014-05-23 19:31:08]

Alexei,

I redid the way the structure of the histograms. Could you take a second look?

Thanks!

[Mike Lerman, 2014-05-23 19:33:20]

Scott,

There have been a lot of changes in the /views code (and all over) since you did
the review in Patch 3. Can you take a second look?

Thanks!

[sky, 2014-05-23 19:55:21]

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/signin/s...
File chrome/browser/signin/signin_header_helper.h (right):

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/signin/s...
chrome/browser/signin/signin_header_helper.h:33: enum GAIA_SERVICE_TYPE {
This should be GaiaServiceType (or maybe GAIAServiceType). See style guide for
details.

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.h:79: enum GAIA_SERVICE_TYPE;
I didn't think you could forward declare enums.

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:266:
signin::GAIA_SERVICE_TYPE gaia_service_type_;
const

[Mike Lerman, 2014-05-23 20:13:03]

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/signin/s...
File chrome/browser/signin/signin_header_helper.h (right):

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/signin/s...
chrome/browser/signin/signin_header_helper.h:33: enum GAIA_SERVICE_TYPE {
On 2014/05/23 19:55:22, sky wrote:
> This should be GaiaServiceType (or maybe GAIAServiceType). See style guide for
> details.

Eek you're right. I had taken my example from above. I am changing both.

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.h:79: enum GAIA_SERVICE_TYPE;
On 2014/05/23 19:55:22, sky wrote:
> I didn't think you could forward declare enums.

Should I not? I would otherwise import the signin_header_helper.h file where
it's declared.

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:266:
signin::GAIA_SERVICE_TYPE gaia_service_type_;
On 2014/05/23 19:55:22, sky wrote:
> const

It originally was but I am now re-assigning the value in the PostActionPerformed
method.

[Mike Lerman, 2014-05-23 20:21:28]

On 2014/05/23 20:13:03, Mike Lerman wrote:
>
https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.h (right):
> 
>
https://codereview.chromium.org/288493008/diff/260001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.h:79: enum GAIA_SERVICE_TYPE;
> On 2014/05/23 19:55:22, sky wrote:
> > I didn't think you could forward declare enums.
> 
> Should I not? I would otherwise import the signin_header_helper.h file where
> it's declared.
> 

I've removed the forward-declarations of the enums. The internet has educated me
as to why that's a bad thing.

[sky, 2014-05-23 20:41:41]

LGTM

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2387: void
BrowserView::ShowAvatarBubbleFromAvatarButton(AvatarBubbleMode mode,
nit: wrap mode to the next line so the two parameters are aligned (same as 2435
below).

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:771:
ProfileMetrics::LogProfileDesktopMenu(action_performed, gaia_service_type_);
nit: indentation is off (should be 2 spaces, not 4).

[Mike Lerman, 2014-05-26 12:55:24]

Thanks Scott.

Alexei, can you take a look please? Thanks!

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2387: void
BrowserView::ShowAvatarBubbleFromAvatarButton(AvatarBubbleMode mode,
On 2014/05/23 20:41:42, sky wrote:
> nit: wrap mode to the next line so the two parameters are aligned (same as
2435
> below).

Done.

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/288493008/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:771:
ProfileMetrics::LogProfileDesktopMenu(action_performed, gaia_service_type_);
On 2014/05/23 20:41:42, sky wrote:
> nit: indentation is off (should be 2 spaces, not 4).

Done.

[Alexei Svitkine (slow), 2014-05-26 17:59:39]

LGTM % comments

https://codereview.chromium.org/288493008/diff/320001/chrome/browser/profiles...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/320001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:301: switch (gaia_service) {
Nit: Maybe add a warning comment mentioning that each histogram needs to be
logged separately since UMA macros cache the object (to avoid someone
refactoring this in the future).

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

https://codereview.chromium.org/288493008/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21094: +    button is noted in the
histogram suffix.
Please update the comment, since suffixes have been expanded.

[Mike Lerman, 2014-05-27 13:24:46]

Thanks Alexei, and all reviewers!

https://codereview.chromium.org/288493008/diff/320001/chrome/browser/profiles...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/288493008/diff/320001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:301: switch (gaia_service) {
On 2014/05/26 17:59:40, Alexei Svitkine wrote:
> Nit: Maybe add a warning comment mentioning that each histogram needs to be
> logged separately since UMA macros cache the object (to avoid someone
> refactoring this in the future).

Done.

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

https://codereview.chromium.org/288493008/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:21094: +    button is noted in the
histogram suffix.
On 2014/05/26 17:59:40, Alexei Svitkine wrote:
> Please update the comment, since suffixes have been expanded.

Done.

[Mike Lerman, 2014-05-27 16:51:12]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-05-27 16:52:08]

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

[commit-bot: I haz the power, 2014-05-27 18:13:33]

Change committed as 272999