Track the active ExtensionKeybindingRegistry and make it available to EventRewriter.

chrome/browser/chromeos/events/event_rewriter.cc

 // Copyright 2014 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 "chrome/browser/chromeos/events/event_rewriter.h"
 
 #include <vector>
 
 #include "ash/sticky_keys/sticky_keys_controller.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/sys_info.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h"
+#include "chrome/browser/extensions/extension_commands_global_registry.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/ime/ime_keyboard.h"
 #include "chromeos/ime/input_method_manager.h"
 #include "components/user_manager/user_manager.h"
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/keyboard_code_conversion.h"
 #include "ui/wm/core/window_util.h"
@@ skipping 68 matching lines @@
 bool IsISOLevel5ShiftUsedByCurrentInputMethod() {
   // Since both German Neo2 XKB layout and Caps Lock depend on Mod3Mask,
   // it's not possible to make both features work. For now, we don't remap
   // Mod3Mask when Neo2 is in use.
   // TODO(yusukes): Remove the restriction.
   input_method::InputMethodManager* manager =
       input_method::InputMethodManager::Get();
   return manager->IsISOLevel5ShiftUsedByCurrentInputMethod();
 }
 
+bool IsExtensionCommandRegistered(const ui::KeyEvent& key_event) {
+  // Some keyboard events for ChromeOS get rewritten, such as:
+  // Search+Shift+Left gets converted to Shift+Home (BeginDocument).
+  // This doesn't make sense if the user has assigned that shortcut
+  // to an extension. Because:
+  // 1) The extension would, upon seeing a request for Ctrl+Shift+Home have
+  //    to register for Shift+Home, instead.
+  // 2) The conversion is unnecessary, because Shift+Home (BeginDocument) isn't
+  //    going to be executed.
+  // Therefore, we skip converting the accelerator if an extension has
+  // registered for this shortcut.
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  if (!profile)
+    return false;
+
+  extensions::ExtensionKeybindingRegistry* registry =
+      extensions::ExtensionCommandsGlobalRegistry::Get(profile)
+          ->active_registry();

[Finnur, 2014/09/12 11:25:44]

This checks the active window's registry. But what conclusion did we reach with
global shortcuts? Do we not need to do anything for those because they don't
pass through the keyboard stack in the normal way (and therefore don't get
rewritten)? 

Or should we could call IsAcceleratorRegistered on the global registry also? If
so, then one thing we could do is add an IsRegistered(accelerator) method on the
global registry and call that on line 141 instead of mucking with the
active_registry here. In that new method (IsRegistered) call
IsAcceleratorRegistered on both the active and the global registry. Then you
also don't have to make IsAcceleratorRegistered public, I believe.

In any case, would be good to add a test for global shortcut.

[David Tseng, 2014/09/12 23:18:32]

EventRewriter hooks in pretty early so it does rewrite both global and nonglobal
accelerators (which is why we check both below on ln 141).
Again, I'm kind of against the mixing of global and non-global. Do we not really
care about distinguishing between the two? Only I see EventRewriter as a special
case. It wants to deal with both global and non global commands in the same way.

If you're ok with mixing the two concepts, then I'll add the new IsRegistered,
but think it will cause all kinds of confusion down the line.

I'm actually more in favor of placing IsRegistered on ExtensionKeybinding which
would check both the global registry and the current instance. That makes more
sense to me.
I do agree with this. Can mirror the current extension but make each command
global.


Actually, just tried doing this and turns out it won't work because of the
restrictions placed on global commands being ctrl+shift+[0-9]. Punting on this
one for another cl, but will leave the helper test function in for now.

[Finnur, 2014/09/15 10:48:42]

Oh, woops. I overlooked the fact we are checking both local and global. Sorry.
:)
Unless we have a more elegant approach, I think we should go with it. The
alternatives I've seen so far just add complex elsewhere. With good function
naming and good comments we can alleviate potential confusion.
I would argue the opposite is simpler. As a consumer, I would be wanting to know
if a shortcut is in use anywhere (i.e. globally within Chrome). It would be
weirder to me to have to ask a local object for that info as opposed to the
global object. It also would complicate the code here, because instead of
accessing and querying just the global object, you'd have to start querying
which window is active, etc.
You had a CL that relaxes restrictions. Maybe it is time to extend that to
tests? I can see that being useful in the future as well. I'm ok with you doing
that in a follow-up CL to this one, if you prefer.

[David Tseng, 2014/09/15 17:25:12]

Added IsRegisteredForActiveWindow to GlobalCommandsRegistry as suggested.
Unfortunately, we still need to make IsAcceleratorRegistered public since we
need to call it on the registry for active window.

 Will add a test for the global case in a follow up due to the relaxation piece.

+  if (!registry)
+    return false;
+
+  int modifiers = ui::EF_NONE;
+  if (key_event.IsShiftDown())
+    modifiers |= ui::EF_SHIFT_DOWN;
+  if (key_event.IsControlDown())
+    modifiers |= ui::EF_CONTROL_DOWN;
+  if (key_event.IsAltDown())
+    modifiers |= ui::EF_ALT_DOWN;
+  if (key_event.IsCommandDown())
+    modifiers |= ui::EF_COMMAND_DOWN;
+  ui::Accelerator accelerator(key_event.key_code(), modifiers);
+  if (registry->IsAcceleratorRegistered(accelerator) ||
+      extensions::ExtensionCommandsGlobalRegistry::Get(profile)
+          ->IsAcceleratorRegistered(accelerator))
+    return true;
+  return false;

[Finnur, 2014/09/12 11:25:44]

nit: prefer: 

return foo || bar;

over 

if (foo || bar)
  return true;
return false;

[David Tseng, 2014/09/12 23:18:32]

Done.

+}
+
 EventRewriter::DeviceType GetDeviceType(const std::string& device_name) {
   std::vector<std::string> tokens;
   Tokenize(device_name, " .", &tokens);
 
   // If the |device_name| contains the two words, "apple" and "keyboard", treat
   // it as an Apple keyboard.
   bool found_apple = false;
   bool found_keyboard = false;
   for (size_t i = 0; i < tokens.size(); ++i) {
     if (!found_apple && LowerCaseEqualsASCII(tokens[i], "apple"))
@@ skipping 236 matching lines @@
     remapped_state->key_code = map.output_key_code;
     remapped_state->flags = (input.flags & ~map.input_flags) | map.output_flags;
     return true;
   }
   return false;
 }
 
 ui::EventRewriteStatus EventRewriter::RewriteKeyEvent(
     const ui::KeyEvent& key_event,
     scoped_ptr<ui::Event>* rewritten_event) {
+  if (IsExtensionCommandRegistered(key_event))
+    return ui::EVENT_REWRITE_CONTINUE;
   if (key_event.source_device_id() != ui::ED_UNKNOWN_DEVICE)
     DeviceKeyPressedOrReleased(key_event.source_device_id());
   MutableKeyState state = {key_event.flags(), key_event.key_code()};
   // Do not rewrite an event sent by ui_controls::SendKeyPress(). See
   // crbug.com/136465.
   if (!(key_event.flags() & ui::EF_FINAL)) {
     RewriteModifierKeys(key_event, &state);
     RewriteNumPadKeys(key_event, &state);
   }
   ui::EventRewriteStatus status = ui::EVENT_REWRITE_CONTINUE;
@@ skipping 488 matching lines @@
     KeyboardDeviceAddedInternal(device_info[i].deviceid, device_info[i].name);
   }
 
   XIFreeDeviceInfo(device_info);
 #else
   KeyboardDeviceAddedInternal(device_id, "keyboard");
 #endif
 }
 
 }  // namespace chromeos