Add GTK support for global Extension Commands

chrome/browser/extensions/global_shortcut_listener_gtk.cc

+// Copyright (c) 2013 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/extensions/global_shortcut_listener_gtk.h"
+
+#include <gdk/gdkx.h>
+#include <X11/Xlib.h>
+
+#include "base/x11/x11_error_tracker.h"
+#include "content/public/browser/browser_thread.h"
+#include "ui/base/accelerators/accelerator.h"
+#include "ui/events/keycodes/keyboard_code_conversion_x.h"
+
+using content::BrowserThread;
+
+namespace {
+
+static base::LazyInstance<extensions::GlobalShortcutListenerGtk> instance =
+    LAZY_INSTANCE_INITIALIZER;
+
+static const unsigned int kModifiersMasks[] = {

[Finnur, 2013/09/27 10:35:02]

I would comment this with why we're keeping track of NUM_LOCK and friends and
why it is important to have all permutations of them.

[zhchbin, 2013/09/27 13:47:20]

Done.

+  0,                                // only specified modifiers.

[Finnur, 2013/09/27 10:35:02]

I would rephrase this: No additional modifier.

[zhchbin, 2013/09/27 13:47:20]

Done.

+  Mod2Mask,                         // NUM_LOCK
+  LockMask,                         // CAPS_LOCK
+  Mod5Mask,                         // SCROLL_LOCK
+  Mod2Mask | LockMask,
+  Mod2Mask | Mod5Mask,
+  LockMask | Mod5Mask,
+  Mod2Mask | LockMask | Mod5Mask
+};
+
+int GetNativeModifiers(const ui::Accelerator& accelerator) {
+  int modifiers = 0;
+  modifiers |= accelerator.IsShiftDown() ? ShiftMask : 0;
+  modifiers |= accelerator.IsCtrlDown() ? ControlMask : 0;
+  modifiers |= accelerator.IsAltDown() ? Mod1Mask : 0;
+
+  return modifiers;
+}
+
+}  // namespace
+
+namespace extensions {
+
+// static
+GlobalShortcutListener* GlobalShortcutListener::GetInstance() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return instance.Pointer();
+}
+
+GlobalShortcutListenerGtk::GlobalShortcutListenerGtk()
+    : is_listening_(false) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+GlobalShortcutListenerGtk::~GlobalShortcutListenerGtk() {
+  if (is_listening_)
+    StopListening();
+}
+
+void GlobalShortcutListenerGtk::StartListening() {
+  DCHECK(!is_listening_);  // Don't start twice.
+  if (!is_listening_) {

[Finnur, 2013/09/27 10:35:02]

In general, we prefer:

if (foo)
  return;
bar();

... instead of ...

if (!foo) {
  bar();
}

However, I don't think the if is needed since the DCHECK is enough. This is
basically a trade-off between gracefully handling errors everywhere vs.
minimizing the amount of code. 

The Chromium philosophy is the latter in general -- and you'll perhaps also note
(elsewhere in the codebase) that we prefer to crash on NULL pointer
de-referencing rather than add if-checks everywhere.

Same with StopListening.

[zhchbin, 2013/09/27 13:47:20]

Done.

+    gdk_window_add_filter(gdk_get_default_root_window(),
+                          &GlobalShortcutListenerGtk::OnXEventThunk,
+                          this);
+    is_listening_ = true;
+  }
+}
+
+void GlobalShortcutListenerGtk::StopListening() {
+  DCHECK(is_listening_);  // No point if we are not already listening.
+
+  if (is_listening_) {
+    gdk_window_remove_filter(NULL,
+                             &GlobalShortcutListenerGtk::OnXEventThunk,
+                             this);
+    is_listening_ = false;
+  }
+}
+
+void GlobalShortcutListenerGtk::RegisterAccelerator(
+    const ui::Accelerator& accelerator,
+    GlobalShortcutListener::Observer* observer) {
+  Display* display = GDK_WINDOW_XDISPLAY(gdk_get_default_root_window());
+  int modifiers = GetNativeModifiers(accelerator);
+  KeyCode keycode = XKeysymToKeycode(display, accelerator.key_code());
+  base::X11ErrorTracker err_tracker;
+
+  // Because XGrabKey only works on the exact modifiers mask, we should register

[Finnur, 2013/09/27 10:35:02]

Yikes. :) That's unfortunate. :)
What about mod3 and mod4? Is there a mod6?

[zhchbin, 2013/09/27 13:47:20]

Yeah, this problem has obsessed me since I working on similar feature of
node-webkit.
Mod3Mask is "right alt", and Mod4Mask is the windows key. Generally speaking,
these two keys have no state like NumLock.

+  // our hot keys with modifiers that we want to ignore, including NUM_LOCK,
+  // CAPS_LOCK, SCROLL_LOCK.
+  for (size_t i = 0; i < arraysize(kModifiersMasks); ++i) {
+    XGrabKey(display, keycode, modifiers | kModifiersMasks[i],
+             gdk_x11_get_default_root_xwindow(), False,
+             GrabModeAsync, GrabModeAsync);
+  }
+
+  if (err_tracker.FoundNewError()) {

[Finnur, 2013/09/27 10:35:02]

So, what are the possible error scenarios here?

I think you already mentioned this will not fail if another app has registered
for the same shortcut, correct? No, wait -- maybe I'm misremembering, since this
doc (http://tronche.com/gui/x/xlib/input/XGrabKey.html) seems to indicate that
we'll get an error if there is a conflict.

In that case we have to think about partial success and how to handle that. For
example, if I ask for Ctrl+Shift+E and it fully succeeds in registering, except
for Ctrl+Shift+E+NumLock (because someone else registered for just that
shortcut). Are we better off not registering for any Ctrl+Shift+E combination at
all (current approach) or ignoring Ctrl+Shift+E+NumLock? I'm leaning towards the
current approach, but I'm not sure.

[zhchbin, 2013/09/27 13:47:20]

Yes, we will get BadAccess if there is a conflict. But the err_tracker didn't
expose this error code to us. It seems that we only concern about the result
here.
The reason why I choose current approach: keep consistence with "RegisterHotKey"
on windows. And if we ignore Ctrl+Shift+E+NumLock and continue, something wired
happen: we are reporting we register hot key successfully, but it works only
when the NumLock is on.

+    LOG(ERROR) << "X failed to grab global hotkey: "
+               << accelerator.GetShortcutText();

[Finnur, 2013/09/27 10:35:02]

Add the error number?

[zhchbin, 2013/09/27 13:47:20]

It seems that we couldn't get the error number if using base::X11ErrorTracker
here. But we can change the X11ErrorTracker to get the X error code, we really
need it? For what? :)

[Finnur, 2013/09/27 14:01:27]

I thought we had the error, but I guess not. That's fine. No worries.

+
+    // We may have part of the hotkeys registered, clean up.
+    for (size_t i = 0; i < arraysize(kModifiersMasks); ++i) {
+      XUngrabKey(display, keycode, modifiers | kModifiersMasks[i],
+                 gdk_x11_get_default_root_xwindow());
+    }
+  } else {
+    registered_hot_keys_.insert(accelerator);
+    GlobalShortcutListener::RegisterAccelerator(accelerator, observer);
+  }
+}
+
+void GlobalShortcutListenerGtk::UnregisterAccelerator(
+    const ui::Accelerator& accelerator,
+    GlobalShortcutListener::Observer* observer) {
+  if (registered_hot_keys_.find(accelerator) == registered_hot_keys_.end())
+    return;
+
+  Display* display = GDK_WINDOW_XDISPLAY(gdk_get_default_root_window());
+  int modifiers = GetNativeModifiers(accelerator);
+  KeyCode keycode = XKeysymToKeycode(display, accelerator.key_code());
+
+  for (size_t i = 0; i < arraysize(kModifiersMasks); ++i) {
+    XUngrabKey(display, keycode, modifiers | kModifiersMasks[i],
+               gdk_x11_get_default_root_xwindow());
+  }
+  registered_hot_keys_.erase(accelerator);
+  GlobalShortcutListener::UnregisterAccelerator(accelerator, observer);
+}
+
+GdkFilterReturn GlobalShortcutListenerGtk::OnXEvent(GdkXEvent* gdk_x_event,
+                                                    GdkEvent* gdk_event) {
+  XEvent* x_event = static_cast<XEvent*>(gdk_x_event);
+  if (x_event->type == KeyPress) {
+    int modifiers = 0;
+    modifiers |= (x_event->xkey.state & ShiftMask) ? ui::EF_SHIFT_DOWN : 0;
+    modifiers |= (x_event->xkey.state & ControlMask) ? ui::EF_CONTROL_DOWN : 0;
+    modifiers |= (x_event->xkey.state & Mod1Mask) ? ui::EF_ALT_DOWN : 0;
+
+    ui::Accelerator accelerator(
+        ui::KeyboardCodeFromXKeyEvent(x_event), modifiers);
+    instance.Get().NotifyKeyPressed(accelerator);
+  }
+
+  return GDK_FILTER_CONTINUE;
+}
+
+}  // namespace extensions