Mac global keybindings

chrome/browser/extensions/global_shortcut_listener_mac.mm

+// 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_mac.h"
+
+#include <ApplicationServices/ApplicationServices.h>
+#import <Cocoa/Cocoa.h>
+#include <IOKit/hidsystem/ev_keymap.h>
+
+#include "base/mac/foundation_util.h"

[Mark Mentovai, 2013/12/10 16:30:32]

#import

[smus, 2013/12/10 22:38:18]

Done.

+#include "chrome/browser/extensions/api/commands/command_service.h"
+#include "content/public/browser/browser_thread.h"
+#include "ui/base/accelerators/accelerator.h"
+#include "ui/events/event.h"
+#import "ui/events/keycodes/keyboard_code_conversion_mac.h"
+
+using content::BrowserThread;
+using extensions::GlobalShortcutListenerMac;
+
+namespace {
+
+// The media keys subtype. No official docs found, but widely known.
+// http://lists.apple.com/archives/cocoa-dev/2007/Aug/msg00499.html
+const int kSystemDefinedEventMediaKeys = 8;

[Mark Mentovai, 2013/12/10 16:30:32]

kSystemDefinedEventMediaKeysSubtype

to make it clear that this is used with -[NSEvent subtype]

[smus, 2013/12/10 22:38:18]

Done.

+
+OSStatus HotKeyHandler(
+    EventHandlerCallRef next_handler, EventRef event, void* user_data) {
+  // Extract the hotkey from the event.
+  EventHotKeyID hot_key_id;
+  int result = GetEventParameter(event, kEventParamDirectObject,

[Mark Mentovai, 2013/12/10 16:30:32]

OSStatus, not int.

[smus, 2013/12/10 22:38:18]

Done.

+      typeEventHotKeyID, NULL, sizeof(hot_key_id), NULL, &hot_key_id);
+  if (result != noErr)
+    return result;
+
+  GlobalShortcutListenerMac* shortcut_listener =
+      static_cast<GlobalShortcutListenerMac*>(user_data);
+  shortcut_listener->OnKeyEvent(hot_key_id);
+  return noErr;
+}
+
+ui::KeyboardCode MediaKeyCodeToKeyboardCode(int key_code) {
+  switch (key_code) {
+    case NX_KEYTYPE_PLAY:
+      return ui::VKEY_MEDIA_PLAY_PAUSE;
+    case NX_KEYTYPE_PREVIOUS:
+    case NX_KEYTYPE_REWIND:
+      return ui::VKEY_MEDIA_PREV_TRACK;
+    case NX_KEYTYPE_NEXT:
+    case NX_KEYTYPE_FAST:
+      return ui::VKEY_MEDIA_NEXT_TRACK;
+  }
+  return ui::VKEY_UNKNOWN;
+}
+
+GlobalShortcutListenerMac* g_instance = NULL;

[Mark Mentovai, 2013/12/10 16:30:32]

Move into GetInstance as a static.

[smus, 2013/12/10 22:38:18]

Done.

+
+}  // namespace
+
+namespace extensions {
+
+// static
+GlobalShortcutListener* GlobalShortcutListener::GetInstance() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[Mark Mentovai, 2013/12/10 16:30:32]

This CHECK is good, but it’s only on this function and on the constructor. This
kind of CHECK/DCHECK should also be on other “public” methods exposed by this
class, to assert that consumers of this API are doing the right thing. I’d even
put one in the destructor, although based on what you’ve told me, the destructor
in this code’s current form will never be called.

This kind of CHECK should also be on as the non-static “private” (or
should-be-“private” per my previous comment) methods of this class that are the
targets of system callbacks, On[Hot]KeyEvent and OnMediaKeyEvent, to assert that
interactions with the system libraries are working exactly as you expect. Since
this class isn’t thread-safe, I think these assertions would be a smart idea.

[smus, 2013/12/10 22:38:18]

Done.

+  if (g_instance == NULL) {
+    g_instance = new GlobalShortcutListenerMac();
+  }
+  return g_instance;
+}
+
+GlobalShortcutListenerMac::GlobalShortcutListenerMac()
+    : is_listening_(false),

[Mark Mentovai, 2013/12/10 16:30:32]

Now that you’re using event_handler_ properly, its value (as a boolean) has
exactly the same meaning as is_listening_. You don’t need two member variables
to track the same thing. Save some memory, reduce complexity. Get rid of
is_listening_, add event_handler_(NULL) to the initializer list here, and fix up
the uses of is_listening_ to use event_handler_ in the destructor,
StartListening, and StopListening.

[smus, 2013/12/10 22:38:18]

Done.

+      hot_key_id_(0),
+      event_tap_(NULL),
+      event_tap_source_(NULL) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+GlobalShortcutListenerMac::~GlobalShortcutListenerMac() {
+  // By this point, UnregisterAccelerator should have been called for all
+  // keyboard shortcuts.
+  if (is_listening_)
+    StopListening();
+
+  // If media keys are still registered, make sure we stop the tap.
+  if (IsAnyMediaKeyRegistered())
+    StopWatchingMediaKeys();
+}
+
+void GlobalShortcutListenerMac::StartListening() {
+  DCHECK(!is_listening_);  // Don't start twice.
+  DCHECK(!hot_key_ids_.empty());  // Don't start if no hot key registered.
+  DCHECK(!id_hot_keys_.empty());
+  is_listening_ = true;
+
+  // Install an event handler for hot keys.

[Mark Mentovai, 2013/12/10 16:30:32]

Personally, I would do this only when it’s needed, on demand from RegisterHotKey
(and do the inverse operation presently in StopListening in UnregisterHotKey).
That’s the way that you made Start/StopWatchingMediaKeys work, and in general, I
like it better.

The existing API already has this Start/StopListening feature, and mixing the
desired Start/StopListening semantics, which I assume to be “don’t respond to a
global shortcut unless StartListening has been called” with doing on-demand
event handler installation would get a little complicated. RegisterHotKey would
need to call a new InstallEventHandlerIfListeningAndKeysAreRegistered function,
and StartListening would have to be {listening_ = true;
InstallEventHandlerIfListeningAndKeysRegistered(); }. Maybe that kind of
complexity isn’t justified.

The reason I’m mentioning it is that you actually have different semantics for
media keys than you have for hot keys. Media keys don’t care at all whether
StartListening or StopListening has been called. If any media key is registered,
it’s alive and active. That doesn’t match the behavior for hot keys.

I think that the behaviors of the two classes of keys should definitely match.
And if honoring Start/StopListening is correct, then that’s what they should do.
On the other hand, if Start/StopListening are merely intended to be advisory, to
do system-specific setup and teardown, then the media keys’ behavior is not
incorrect. In that case, I still think that the behaviors of the hot keys and
media keys should match, but it’d be at your option to decide which semantics
you’d prefer to implement.

[smus, 2013/12/10 22:38:18]

RegisterAccelerator can be called at any point before or after StartListening.
I've now moved on to this lazy event handler installation model you suggested,
but would rather hold on to the is_listening_ private member just for sanity
checking.

+  EventHandlerUPP hot_key_function = NewEventHandlerUPP(HotKeyHandler);
+  EventTypeSpec event_type;
+  event_type.eventClass = kEventClassKeyboard;
+  event_type.eventKind = kEventHotKeyPressed;
+  InstallApplicationEventHandler(
+      hot_key_function, 1, &event_type, this, &event_handler_);
+}
+
+void GlobalShortcutListenerMac::StopListening() {
+  DCHECK(is_listening_);  // No point if we are not already listening.
+  DCHECK(hot_key_ids_.empty());  // Make sure the set is clean.
+  DCHECK(id_hot_keys_.empty());
+  is_listening_ = false;
+
+  // Remove the hot key event handler.
+  RemoveEventHandler(event_handler_);
+}
+
+void GlobalShortcutListenerMac::OnKeyEvent(EventHotKeyID hot_key_id) {
+  // Look up the accelerator based on this hot key ID.
+  const ui::Accelerator& accelerator = id_hot_keys_[hot_key_id.id];

[Mark Mentovai, 2013/12/10 16:30:32]

At
https://codereview.chromium.org/60353008/diff/790001/chrome/browser/extension...
, Robert said “What if it's not found?” and you responded “Done.” You did answer
a similar question of mine, but nothing here was actually done.

I read the implied second half of Robert’s question as “If it’s a possibility,
you should handle that case, but if it should never happen, do you want to
assert so with a DCHECK?”

[smus, 2013/12/10 22:38:18]

Oops, my mistake. Added a DCHECK.

+  NotifyKeyPressed(accelerator);
+}
+
+bool GlobalShortcutListenerMac::OnMediaKeyEvent(int media_key_code) {
+  ui::KeyboardCode key_code = MediaKeyCodeToKeyboardCode(media_key_code);
+  // Create an accelerator corresponding to the keyCode.
+  ui::Accelerator accelerator(key_code, 0);
+  // Look for a match with a bound hot_key.
+  if (hot_key_ids_.find(accelerator) != hot_key_ids_.end()) {
+    // If matched, callback to the event handling system.
+    NotifyKeyPressed(accelerator);
+    return true;
+  }
+  return false;
+}
+
+void GlobalShortcutListenerMac::RegisterAccelerator(
+    const ui::Accelerator& accelerator,
+    GlobalShortcutListener::Observer* observer) {
+  if (hot_key_ids_.find(accelerator) != hot_key_ids_.end()) {
+    // The shortcut has already been registered. Some shortcuts, such as
+    // MediaKeys can have multiple targets, all keyed off of the same
+    // accelerator.
+    return;
+  }
+
+  // If this is the first media key registered, start the event tap.
+  if (CommandService::IsMediaKey(accelerator) && !IsAnyMediaKeyRegistered()) {
+    StartWatchingMediaKeys();
+  }
+
+  // Register hot_key if they are non-media keyboard shortcuts.
+  if (!CommandService::IsMediaKey(accelerator))

[Mark Mentovai, 2013/12/10 16:30:32]

Rather than calling IsMediaKey multiple times, how about

  if (CommandService::IsMediaKey(accelerator)) {
    if (!IsAnyMediaKeyRegistered()) {
      StartWatchingMediaKeys();
    }
  } else {
    RegisterHotKey(accelerator, hot_key_id_);
  }

[smus, 2013/12/10 22:38:18]

Done.

+    RegisterHotKey(accelerator, hot_key_id_);
+
+  // Store the hotkey-ID mappings we will need for lookup later.
+  id_hot_keys_[hot_key_id_] = accelerator;
+  hot_key_ids_[accelerator] = hot_key_id_;
+  ++hot_key_id_;
+  GlobalShortcutListener::RegisterAccelerator(accelerator, observer);
+}
+
+void GlobalShortcutListenerMac::UnregisterAccelerator(
+    const ui::Accelerator& accelerator,
+    GlobalShortcutListener::Observer* observer) {
+  // Unregister the hot_key if it's a keyboard shortcut.
+  if (!CommandService::IsMediaKey(accelerator))
+    UnregisterHotKey(accelerator);
+
+  // Remove hot_key from the mappings.
+  int id = hot_key_ids_[accelerator];

[Mark Mentovai, 2013/12/10 16:30:32]

What if wasn’t registered to begin with?

[Mark Mentovai, 2013/12/10 16:30:32]

There’s a typedef for this now, HotKeyId.

[smus, 2013/12/10 22:38:18]

Done.

[smus, 2013/12/10 22:38:18]

Added a DCHECK

+  id_hot_keys_.erase(id);
+  hot_key_ids_.erase(accelerator);
+  GlobalShortcutListener::UnregisterAccelerator(accelerator, observer);
+
+  // If we unregistered a media key, and now if no media keys are registered,
+  // stop the media key tap.
+  if (CommandService::IsMediaKey(accelerator) && !IsAnyMediaKeyRegistered()) {
+    StopWatchingMediaKeys();
+  }
+}
+
+void GlobalShortcutListenerMac::RegisterHotKey(
+    const ui::Accelerator& accelerator, int hot_key_id) {

[Mark Mentovai, 2013/12/10 16:30:32]

int → HotKeyId?

[smus, 2013/12/10 22:38:18]

Done.

+  EventHotKeyRef hot_key_ref;

[Mark Mentovai, 2013/12/10 16:30:32]

Don’t declare until use, down where you call RegisterEventHotKey.

[smus, 2013/12/10 22:38:18]

Done.

+  EventHotKeyID event_hot_key_id;
+
+  // Signature uniquely identifies the application that owns this hot_key.
+  event_hot_key_id.signature = base::mac::CreatorCodeForApplication();
+  event_hot_key_id.id = hot_key_id;
+
+  // Translate ui::Accelerator modifiers to cmdKey, altKey, etc.
+  int modifiers = 0;
+  modifiers |= (accelerator.IsShiftDown() ? shiftKey : 0);
+  modifiers |= (accelerator.IsCtrlDown() ? controlKey : 0);
+  modifiers |= (accelerator.IsAltDown() ? optionKey : 0);
+  modifiers |= (accelerator.IsCmdDown() ? cmdKey : 0);
+
+  unichar character;

[Mark Mentovai, 2013/12/10 16:30:32]

You don’t use either of these. You can eliminate them and pass NULL as the last
two parameters to ui::MacKeyCodeForWindowsKeyCode.

[smus, 2013/12/10 22:38:18]

Done.

+  unichar character_nomods;
+  int key_code = ui::MacKeyCodeForWindowsKeyCode(accelerator.key_code(), 0,
+      &character, &character_nomods);
+
+  // Register the event hot key.
+  RegisterEventHotKey(key_code, modifiers, event_hot_key_id,
+      GetApplicationEventTarget(), 0, &hot_key_ref);
+
+  id_hot_key_refs_[hot_key_id] = hot_key_ref;
+}
+
+void GlobalShortcutListenerMac::UnregisterHotKey(
+    const ui::Accelerator& accelerator) {
+  // Get the ref corresponding to this accelerator.
+  int id = hot_key_ids_[accelerator];

[Mark Mentovai, 2013/12/10 16:30:32]

What if it wasn’t registered to begin with?

I asked the same question in UnregisterAccelerator on line 168, but
UnregisterAccelerator calls this function before it manipulates hot_key_ids_ or
the other maps, so this will actually be the first place in the current code
that would become a problem if a caller passed in an accelerator that wasn’t
registered to the public API UnregisterAccelerator.

[Mark Mentovai, 2013/12/10 16:30:32]

int → HotKeyId?

[smus, 2013/12/10 22:38:18]

Done.

[smus, 2013/12/10 22:38:18]

Done.

+  EventHotKeyRef ref = id_hot_key_refs_[id];
+  // Unregister the event hot key.
+  UnregisterEventHotKey(ref);
+
+  // Remove the event from the mapping.
+  id_hot_key_refs_.erase(id);
+}
+
+void GlobalShortcutListenerMac::StartWatchingMediaKeys() {
+  // Make sure there's no existing event tap.
+  DCHECK(event_tap_ == NULL);

[Mark Mentovai, 2013/12/10 16:30:32]

DCHECK(event_tap_source_ == NULL) too?

[smus, 2013/12/10 22:38:18]

Done.

+
+  // Add an event tap to intercept the system defined media key events.
+  event_tap_ = CGEventTapCreate(kCGSessionEventTap,
+      kCGHeadInsertEventTap,
+      kCGEventTapOptionDefault,
+      CGEventMaskBit(NX_SYSDEFINED),
+      EventTapCallback,
+      this);
+  if (event_tap_ == NULL) {
+    LOG(ERROR) << "Error: failed to create event tap.";
+    return;
+  }
+
+  event_tap_source_ = CFMachPortCreateRunLoopSource(kCFAllocatorSystemDefault,
+      event_tap_, 0);
+  if (event_tap_source_ == NULL) {
+    LOG(ERROR) << "Error: failed to create new run loop source.";
+    return;
+  }
+
+  CFRunLoopAddSource(CFRunLoopGetCurrent(), event_tap_source_,
+      kCFRunLoopCommonModes);
+}
+
+void GlobalShortcutListenerMac::StopWatchingMediaKeys() {
+  CFRunLoopRemoveSource(CFRunLoopGetCurrent(), event_tap_source_,

[Mark Mentovai, 2013/12/10 16:30:32]

DCHECK(event_tap_) and DCHECK(event_tap_source_) ?

[smus, 2013/12/10 22:38:18]

These are checked below, but I restructured to be clearer.

+      kCFRunLoopCommonModes);
+
+  // Invalidate the event tap.
+  DCHECK(event_tap_ != NULL);
+  CFMachPortInvalidate(event_tap_);
+  CFRelease(event_tap_);
+  event_tap_ = NULL;
+
+  // Release the event tap source.
+  DCHECK(event_tap_source_ != NULL);
+  CFRelease(event_tap_source_);
+  event_tap_source_ = NULL;
+}
+
+bool GlobalShortcutListenerMac::IsAnyMediaKeyRegistered() {
+  // Iterate through registered accelerators, looking for media keys.
+  HotKeyIdMap::iterator iter;
+  for (iter = hot_key_ids_.begin(); iter != hot_key_ids_.end(); ++iter) {
+    if (CommandService::IsMediaKey(iter->first))
+      return true;
+  }
+  return false;
+}
+
+// Processed events should propagate if they aren't handled by any listeners.
+// For events that don't matter, this handler should return as quickly as
+// possible.
+// Returning event causes the event to propagate to other applications.
+// Returning NULL prevents the event from propagating.
+// static
+CGEventRef GlobalShortcutListenerMac::EventTapCallback(
+    CGEventTapProxy proxy, CGEventType type, CGEventRef event, void* refcon) {
+  GlobalShortcutListenerMac* shortcut_listener =
+      static_cast<GlobalShortcutListenerMac*>(refcon);
+
+  // Handle the timeout case by re-enabling the tap.
+  if (type == kCGEventTapDisabledByTimeout) {
+    CGEventTapEnable(shortcut_listener->event_tap_, TRUE);
+    return event;
+  }
+
+  // Convert the CGEvent to an NSEvent for access to the data1 field.
+  NSEvent* ns_event = [NSEvent eventWithCGEvent:event];
+  if (ns_event == nil) {
+    return event;
+  }
+
+  // Ignore events that are not system defined media keys.
+  if (type != NX_SYSDEFINED ||
+      [ns_event type] != NSSystemDefined ||
+      [ns_event subtype] != kSystemDefinedEventMediaKeys) {
+    return event;
+  }
+
+  NSInteger data1 = [ns_event data1];
+  // Ignore media keys that aren't previous, next and play/pause.
+  // Magical constants are from http://weblog.rogueamoeba.com/2007/09/29/
+  int key_code = ((data1 & 0xFFFF0000) >> 16);

[Mark Mentovai, 2013/12/10 16:30:32]

Outer (parentheses) unnecessary here and on line 313.

[smus, 2013/12/10 22:38:18]

Done.

+  if (key_code != NX_KEYTYPE_PLAY && key_code != NX_KEYTYPE_NEXT &&
+      key_code != NX_KEYTYPE_PREVIOUS && key_code != NX_KEYTYPE_FAST &&
+      key_code != NX_KEYTYPE_REWIND) {
+    return event;
+  }
+
+  int key_flags = (data1 & 0x0000FFFF);
+  bool is_key_pressed = (((key_flags & 0xFF00) >> 8)) == 0xA;
+
+  // If the key wasn't pressed (eg. was released), ignore this event.
+  if (!is_key_pressed)
+    return event;
+
+  // Now we have a media key that we care about. Send it to the caller.
+  bool was_handled = shortcut_listener->OnMediaKeyEvent(key_code);
+
+  // Prevent event from proagating to other apps if handled by Chrome.
+  if (was_handled)
+    return NULL;
+
+  // By default, pass the event through.
+  return event;
+}
+
+}  // namespace extensions