Record user actions for Ash accelerators.

ash/accelerators/accelerator_controller.cc

Index: ash/accelerators/accelerator_controller.cc
diff --git a/ash/accelerators/accelerator_controller.cc b/ash/accelerators/accelerator_controller.cc
index f761acb721e26ca11f9439431a32ed91097fc1cf..b34e8dde4d5730c38adb341df5995b826d11749b 100644
--- a/ash/accelerators/accelerator_controller.cc
+++ b/ash/accelerators/accelerator_controller.cc
@@ -552,8 +552,12 @@ bool AcceleratorController::PerformAction(int action,
   // below) or pull it into a HandleFoo() function above.
   switch (action) {
     case ACCESSIBLE_FOCUS_NEXT:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Accessible_Focus_Next"));
       return HandleAccessibleFocusCycle(false);
     case ACCESSIBLE_FOCUS_PREVIOUS:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Accessible_Focus_Previous"));
       return HandleAccessibleFocusCycle(true);
     case CYCLE_BACKWARD_MRU:
       HandleCycleBackwardMRU(accelerator);

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

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.

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

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.

@@ -566,9 +570,13 @@ bool AcceleratorController::PerformAction(int action,
       return true;
 #if defined(OS_CHROMEOS)
     case ADD_REMOVE_DISPLAY:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Add_Remove_Display"));
       Shell::GetInstance()->display_manager()->AddRemoveDisplay();
       return true;
     case TOGGLE_MIRROR_MODE:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Toggle_Mirror_Mode"));
       Shell::GetInstance()->display_controller()->ToggleMirrorMode();
       return true;
     case LOCK_SCREEN:
@@ -576,23 +584,38 @@ bool AcceleratorController::PerformAction(int action,
         content::RecordAction(content::UserMetricsAction("Accel_LockScreen_L"));
       return HandleLock();
     case OPEN_FILE_MANAGER:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Handle_File_Manager"));
       return HandleFileManager();
     case OPEN_CROSH:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Open_Crosh"));
       return HandleCrosh();
     case SILENCE_SPOKEN_FEEDBACK:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Silence_Spoken_Feedback"));
       HandleSilenceSpokenFeedback();
       break;
     case SWAP_PRIMARY_DISPLAY:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Swap_Primary_Display"));
       Shell::GetInstance()->display_controller()->SwapPrimaryDisplay();
       return true;
     case SWITCH_TO_NEXT_USER:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Switch_To_Next_User"));
       return SwitchToNextUser();
     case TOGGLE_SPOKEN_FEEDBACK:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Toggle_Spoken_Feedback"));
       return HandleToggleSpokenFeedback();
     case TOGGLE_WIFI:
+      content::RecordAction(content::UserMetricsAction("Accel_Toggle_Wifi"));

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

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?

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

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.
I wouldn't bother logging debugging shortcuts.

       Shell::GetInstance()->system_tray_notifier()->NotifyRequestToggleWifi();
       return true;
     case TOUCH_HUD_CLEAR: {
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Touch_Hud_Clear"));
       internal::RootWindowController* controller =
           internal::RootWindowController::ForTargetRootWindow();
       if (controller->touch_hud_debug()) {
@@ -602,6 +625,8 @@ bool AcceleratorController::PerformAction(int action,
       return false;
     }
     case TOUCH_HUD_MODE_CHANGE: {
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Touch_Hud_Mode_Change"));
       internal::RootWindowController* controller =
           internal::RootWindowController::ForTargetRootWindow();
       if (controller->touch_hud_debug()) {
@@ -611,15 +636,21 @@ bool AcceleratorController::PerformAction(int action,
       return false;
     }
     case TOUCH_HUD_PROJECTION_TOGGLE: {
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Touch_Hud_Clear"));
       bool enabled = Shell::GetInstance()->is_touch_hud_projection_enabled();
       Shell::GetInstance()->SetTouchHudProjectionEnabled(!enabled);
       return true;
     }
     case DISABLE_GPU_WATCHDOG:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Disable_Gpu_Watchdog"));
       content::GpuDataManager::GetInstance()->DisableGpuWatchdog();
       return true;
 #endif
     case OPEN_FEEDBACK_PAGE:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Open_Feedback_Page"));
       ash::Shell::GetInstance()->new_window_delegate()->OpenFeedbackPage();
       return true;
     case EXIT:
@@ -627,6 +658,8 @@ bool AcceleratorController::PerformAction(int action,
       exit_warning_handler_.HandleAccelerator();
       return true;
     case NEW_INCOGNITO_WINDOW: {
+        content::RecordAction(
+            content::UserMetricsAction("Accel_New_Incognito_Window"));
         bool incognito_allowed =
             Shell::GetInstance()->delegate()->IsIncognitoAllowed();
         if (incognito_allowed)
@@ -640,13 +673,17 @@ bool AcceleratorController::PerformAction(int action,
       Shell::GetInstance()->new_window_delegate()->NewTab();
       return true;
     case NEW_WINDOW:
+      content::RecordAction(content::UserMetricsAction("Accel_New_Window"));

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

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]

I don't think so.

       Shell::GetInstance()->new_window_delegate()->NewWindow(
           false /* is_incognito */);
       return true;
     case RESTORE_TAB:
+      content::RecordAction(content::UserMetricsAction("Accel_Restore_Tab"));
       Shell::GetInstance()->new_window_delegate()->RestoreTab();
       return true;
     case TAKE_SCREENSHOT:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Take_Screenshot"));
       if (screenshot_delegate_.get() &&
           screenshot_delegate_->CanTakeScreenshot()) {
         screenshot_delegate_->HandleTakeScreenshotForAllRootWindows();
@@ -654,6 +691,8 @@ bool AcceleratorController::PerformAction(int action,
       // Return true to prevent propagation of the key event.
       return true;
     case TAKE_PARTIAL_SCREENSHOT:
+      content::RecordAction(
+          content::UserMetricsAction("Accel_Take_Partial_Screenshot"));
       if (screenshot_delegate_) {
         ash::PartialScreenshotView::StartPartialScreenshot(
             screenshot_delegate_.get());