Added UMA metrics for ActionBox

Added UMA metrics for ActionBox

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163146

[skare_, 2012-10-15 21:26:26]

http://codereview.chromium.org/11017042/diff/1/chrome/browser/ui/browser_comm...
File chrome/browser/ui/browser_commands.cc (right):

http://codereview.chromium.org/11017042/diff/1/chrome/browser/ui/browser_comm...
chrome/browser/ui/browser_commands.cc:699:
content::RecordAction(UserMetricsAction("ActionBox_SharePage"));
my incoming change http://codereview.chromium.org/11073009/ will remove this
function, but this functionality can be included in action_box_menu_model.cc
after that lands. The behavior changes a little bit, so I can demo it to you.

[Wei Li, 2012-10-16 23:40:13]

Thanks, updated based on your new landed change. PTAL.

On 2012/10/15 21:26:26, Travis Skare wrote:
>
http://codereview.chromium.org/11017042/diff/1/chrome/browser/ui/browser_comm...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
http://codereview.chromium.org/11017042/diff/1/chrome/browser/ui/browser_comm...
> chrome/browser/ui/browser_commands.cc:699:
> content::RecordAction(UserMetricsAction("ActionBox_SharePage"));
> my incoming change http://codereview.chromium.org/11073009/ will remove this
> function, but this functionality can be included in action_box_menu_model.cc
> after that lands. The behavior changes a little bit, so I can demo it to you.

[skare_, 2012-10-17 00:29:15]

nice -- sorry to have obsoleted your first patchset.

http://codereview.chromium.org/11017042/diff/4001/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11017042/diff/4001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:110: if
(should_send_uma_) {
logic and stat look good, perhaps call this send_uma_provider_count_ or
something. We will have stats like the previous line that are already getting
set even when "should_send_uma_" will be false.

I don't know if Chrome style allows local statics but that could allow not
making this a member.

http://codereview.chromium.org/11017042/diff/4001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:113:
SHARE_COMMAND_LAST);
I'm new to Chrome UMA but should this be kMaxShareItemsToShow?

[Wei Li, 2012-10-17 05:23:06]

http://codereview.chromium.org/11017042/diff/4001/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11017042/diff/4001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:110: if
(should_send_uma_) {
On 2012/10/17 00:29:15, Travis Skare wrote:
> logic and stat look good, perhaps call this send_uma_provider_count_ or
> something. We will have stats like the previous line that are already getting
> set even when "should_send_uma_" will be false.
> 
> I don't know if Chrome style allows local statics but that could allow not
> making this a member.

Done.

http://codereview.chromium.org/11017042/diff/4001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:113:
SHARE_COMMAND_LAST);
On 2012/10/17 00:29:15, Travis Skare wrote:
> I'm new to Chrome UMA but should this be kMaxShareItemsToShow?

Done.
Nice catch. I didn't notice the start value of the enum. According to the macro
definition, the last param should be the boundary value strictly larger than any
enum so I plus it by one.

[skare_, 2012-10-18 19:27:50]

stat functionality LGTM

http://codereview.chromium.org/11017042/diff/15001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11017042/diff/15001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:37: static bool
send_uma_share_command_count = true;
reviewers -- side question just for my curiosity:
is there a preference for this vs. a static local?

http://codereview.chromium.org/11017042/diff/15001/chrome/tools/chromeactions...
File chrome/tools/chromeactions.txt (right):

http://codereview.chromium.org/11017042/diff/15001/chrome/tools/chromeactions...
chrome/tools/chromeactions.txt:79: 0x1f17eca7af8c3394	ActionBox.FindShares
nit: s/FindShares/FindShareHandlers ?

[Wei Li, 2012-10-18 19:49:54]

https://codereview.chromium.org/11017042/diff/15001/chrome/tools/chromeaction...
File chrome/tools/chromeactions.txt (right):

https://codereview.chromium.org/11017042/diff/15001/chrome/tools/chromeaction...
chrome/tools/chromeactions.txt:79: 0x1f17eca7af8c3394	ActionBox.FindShares
On 2012/10/18 19:27:50, Travis Skare wrote:
> nit: s/FindShares/FindShareHandlers ?

Done.

[sky, 2012-10-18 21:16:55]

LGTM

[commit-bot: I haz the power, 2012-10-18 21:47:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weili@chromium.org/11017042/15002

[commit-bot: I haz the power, 2012-10-20 01:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weili@chromium.org/11017042/15002

[commit-bot: I haz the power, 2012-10-20 04:17:14]

Change committed as 163146