Record user actions for Ash accelerators.

Record user actions for Ash accelerators.

Clean up the huge switch of accelerators as well:
* Move a bunch of code into Handle helpers.
* Alphabetize the anon namespace helpers.
* Clean up the OS_CHROMEOS groupings a bit.

BUG=321695
R=jamescook@chromium.org

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

[SteveT, 2013-11-20 18:43:17]

Hey James,

Before I spend a bunch of time going through this huge list of shortcuts (there
are several more pages of accelerators to look at and log) I wanted to make sure
that the general style I am using is OK with you.

PTAL at the in-progress CL below. I have some inline comments for your
consideration/feedback.

Thanks!


Steve

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:563:
HandleCycleBackwardMRU(accelerator);
Some of these Handle...() methods do the logging internal because they have
additional logic that determines whether or not they should log the stat.

Should we be following that pattern where appropriate, or is it OK to record the
actions out here before the Handle...() call? I'm concerned that I don't fully
understand how all these accelerators work, so I might be logging some
inappropriately.

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:613:
content::RecordAction(content::UserMetricsAction("Accel_Toggle_Wifi"));
Do you know anything about this hotkey? (Ctrl+Alt+W)  It's supposed to toggle
Wifi but doesn't seem to work for me... I traced the code a bit and couldn't
figure out why.

Anyways, there seem to be a lot of special hotkeys that we don't publish to
users... such as this, and TOUCH_HUD_CLEAR below (which only works if you turn
some flag on). Do you think it's worth logging all these?

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:671: if (key_code == ui::VKEY_T)
I am trying to figure out why this does an additional check that the key_code is
"T". There are two references to NEW_TAB

This expected one in ash:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/accelerators/a...

And this other one in c/b/ui/views
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The second one is strange.. is this a keyboard accelerator, or is this command
being invoked some other way?


Similarly, LOCK_SCREEN above checks the the key is L.

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:676:
content::RecordAction(content::UserMetricsAction("Accel_New_Window"));
To follow up with the above comment, should I be checking for key_code ==
ui::VKEY_N before logging the action here?

[James Cook, 2013-11-20 19:14:28]

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:563:
HandleCycleBackwardMRU(accelerator);
On 2013/11/20 18:43:17, SteveT wrote:
> Some of these Handle...() methods do the logging internal because they have
> additional logic that determines whether or not they should log the stat.
> 
> Should we be following that pattern where appropriate, or is it OK to record
the
> actions out here before the Handle...() call? I'm concerned that I don't fully
> understand how all these accelerators work, so I might be logging some
> inappropriately.

Given that this switch statement is way way way too long I would rather the
logging be inside the Handle* utility functions. If there isn't one already, and
the existing code is more than 1 line, I would love to have a new Handle*
function created.

I think some of the Handle* functions have conditional logging is because there
are multiple keys bound to the accelerator functionality.

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:613:
content::RecordAction(content::UserMetricsAction("Accel_Toggle_Wifi"));
On 2013/11/20 18:43:17, SteveT wrote:
> Do you know anything about this hotkey? (Ctrl+Alt+W)  It's supposed to toggle
> Wifi but doesn't seem to work for me... I traced the code a bit and couldn't
> figure out why.

Ctrl+Alt+W should only exist in a Debug build. Shortcuts like this basically
exist to allow testing with a linux_chromeos build.  For example, Alt-W cycles
windows because on Linux Alt-Tab is used by the window manager.

I think we shipped a machine (parrot) that had a dedicated Wi-Fi key, which is
probably why this accelerator exists.

> Anyways, there seem to be a lot of special hotkeys that we don't publish to
> users... such as this, and TOUCH_HUD_CLEAR below (which only works if you turn
> some flag on). Do you think it's worth logging all these?

I wouldn't bother logging debugging shortcuts.

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:671: if (key_code == ui::VKEY_T)
On 2013/11/20 18:43:17, SteveT wrote:
> I am trying to figure out why this does an additional check that the key_code
is
> "T". There are two references to NEW_TAB

I have no idea why this checks for 'T'. Maybe there was some old code that
abused the AcceleratorController by calling PerformAction() directly with
NEW_TAB.

> And this other one in c/b/ui/views
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> The second one is strange.. is this a keyboard accelerator, or is this command
> being invoked some other way?

I think this is some awful code to allow the right-click menus in the browser to
show the correct keyboard shortcut label ("Ctrl-T" for new tab).

> Similarly, LOCK_SCREEN above checks the the key is L.

No idea here.

https://codereview.chromium.org/78763003/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:676:
content::RecordAction(content::UserMetricsAction("Accel_New_Window"));
On 2013/11/20 18:43:17, SteveT wrote:
> To follow up with the above comment, should I be checking for key_code ==
> ui::VKEY_N before logging the action here?

I don't think so.

[SteveT, 2013-11-26 20:00:41]

Hey James,

I've started moving things into their own Handle methods and creating new Handle
methods for functions that span 2+ lines. This is not done yet (a couple dozen
more to go), but I wanted to get your OK on this approach before I finish the
rest of them.

Please take a quick peek if you have a chance. No need for a full review. I'll
finish the rest tomorrow.


Steve

[James Cook, 2013-11-26 21:40:58]

I like the approach!

https://codereview.chromium.org/78763003/diff/80001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/78763003/diff/80001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:948: }
nit: I don't think you need the extra braces and it makes this too-long switch
statement even longer.

[SteveT, 2013-11-27 17:56:49]

Ooookay. Addressed everything else in this fashion. Also:

* Alphabetized the anon namespace helpers.
* Cleaned up the OS_CHROMEOS groupings a bit.

https://codereview.chromium.org/78763003/diff/80001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/78763003/diff/80001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:948: }
Done here and below.

[James Cook, 2013-11-27 21:25:28]

LGTM - Love it!  A couple optional suggestions.

https://codereview.chromium.org/78763003/diff/120001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/78763003/diff/120001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:207:
content::UserMetricsAction("Accel_Launch_App_0"));
Do you really care about whether the user hit 1, 2, 3... individually, or would
it be enough to have a single metric for "Accel_Launch_App"?

You're going to get a fall-off from 1 -> 9 I suspect, but it's unlikely we would
choose to provide shortcuts for, say, 1-5 but not 6-0.

https://codereview.chromium.org/78763003/diff/120001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:703:
Shell::GetInstance()->session_state_delegate()->LockScreen();
I'm surprised this one doesn't get a metric.

[SteveT, 2013-11-27 22:04:52]

Addressed both your comments. Thanks for the long review!

Need to comb over this to make sure the refactoring didn't trample stuff before
committing.

https://codereview.chromium.org/78763003/diff/120001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/78763003/diff/120001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:207:
content::UserMetricsAction("Accel_Launch_App_0"));
Good point.

I feel like we might not care about whether or not users use each specific
number since they could mean different apps for different users, which has no
meaning to us. I also agree with your hypothesis that the first few will be used
more than the others - not knowing *how* much more doesn't affect us.

You've convinced me - I'll switch over to just one metric (and one function) for
all of these. At the end of the day we only care if "Launch App" is used at all.

https://codereview.chromium.org/78763003/diff/120001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:703:
Shell::GetInstance()->session_state_delegate()->LockScreen();
My bad. This one used to have a metric but it got lost in the Handle...
refactoring.

Which reminds me - I need to do a (long) look-over to make sure no other
functions or metrics were trampled by this giant refactoring...

[commit-bot: I haz the power, 2013-11-29 21:40:23]

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

[commit-bot: I haz the power, 2013-11-29 23:41:10]

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

[SteveT, 2013-12-02 15:53:35]

Hey James,

I am seeing a failure on linux_chromeos (ash_unittests) which I can't seem to
reproduce on my local Linux machine running a chromeos=1 build.

Is there anything else I need to set to reproduce a linux_chromeos build on my
machine?


Steve

[do not use, 2013-12-02 17:04:38]

linux_chromeos should be equivalent to setting chromeos=1 in your local
build.  I'm not sure why you would get different results. Locale difference
on the bots, maybe?

If you have trouble figuring it out sometimes oshima knows the differences
between bots and local machines.

James


On Mon, Dec 2, 2013 at 7:53 AM, <stevet@chromium.org> wrote:

> Hey James,
>
> I am seeing a failure on linux_chromeos (ash_unittests) which I can't seem
> to
> reproduce on my local Linux machine running a chromeos=1 build.
>
> Is there anything else I need to set to reproduce a linux_chromeos build
> on my
> machine?
>
>
> Steve
>
> https://codereview.chromium.org/78763003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-12-02 18:49:11]

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

[commit-bot: I haz the power, 2013-12-02 22:37:41]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-03 13:17:38]

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

[commit-bot: I haz the power, 2013-12-03 14:28:17]

Change committed as 238391