Record user actions for Ash accelerators.

ash/accelerators/accelerator_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/accelerators/accelerator_controller.h"
 
 #include <algorithm>
 #include <cmath>
 #include <iostream>
 #include <string>
@@ skipping 534 matching lines @@
 
   // You *MUST* return true when some action is performed. Otherwise, this
   // function might be called *twice*, via BrowserView::PreHandleKeyboardEvent
   // and BrowserView::HandleKeyboardEvent, for a single accelerator press.
   //
   // If your accelerator invokes more than one line of code, please either
   // implement it in your module's controller code (like TOGGLE_MIRROR_MODE
   // 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.

       return true;
     case CYCLE_FORWARD_MRU:
       HandleCycleForwardMRU(accelerator);
       return true;
     case CYCLE_LINEAR:
       HandleCycleLinear(accelerator);
       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:
       if (key_code == ui::VKEY_L)
         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()) {
         controller->touch_hud_debug()->Clear();
         return true;
       }
       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()) {
         controller->touch_hud_debug()->ChangeToNextMode();
         return true;
       }
       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:
       // UMA metrics are recorded in the handler.
       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)
           Shell::GetInstance()->new_window_delegate()->NewWindow(
               true /* is_incognito */);
         return incognito_allowed;
     }
     case NEW_TAB:
       if (key_code == ui::VKEY_T)

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

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.

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

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.
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).
No idea here.

         content::RecordAction(content::UserMetricsAction("Accel_NewTab_T"));
       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();
       }
       // 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());
       }
       // Return true to prevent propagation of the key event because
       // this key combination is reserved for partial screenshot.
       return true;
     case TOGGLE_APP_LIST:
       // If something else was pressed between the Search key (LWIN)
       // being pressed and released, then ignore the release of the
@@ skipping 329 matching lines @@
     keyboard_brightness_control_delegate) {
   keyboard_brightness_control_delegate_ =
       keyboard_brightness_control_delegate.Pass();
 }
 
 bool AcceleratorController::CanHandleAccelerators() const {
   return true;
 }
 
 }  // namespace ash