Mac global keybindings

chrome/browser/extensions/global_shortcut_listener_mac.mm

Index: chrome/browser/extensions/global_shortcut_listener_mac.mm
===================================================================
--- chrome/browser/extensions/global_shortcut_listener_mac.mm	(revision 0)
+++ chrome/browser/extensions/global_shortcut_listener_mac.mm	(working copy)
@@ -0,0 +1,365 @@
+// 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"
+
+#import <Cocoa/Cocoa.h>
+
+#include "content/public/browser/browser_thread.h"
+#include "ui/events/event.h"
+#include "ui/base/accelerators/accelerator.h"
+#import "ui/events/keycodes/keyboard_code_conversion_mac.h"
+
+#define SystemDefinedEventMediaKeys 8

[Robert Sesek, 2013/11/18 15:25:03]

Use type-safe C++ constants. Do not use #define for constants.

[smus, 2013/11/18 23:13:41]

Done.

+#define EVENT_KEY @"event"
+#define HANDLED_KEY @"handled"
+
+typedef extensions::GlobalShortcutListenerMac GSL;

[Robert Sesek, 2013/11/18 15:25:03]

This is not an appropriate typedef. Use |using
extensions::GlobalShortcutListener| and refer to it by its actual typename.

[smus, 2013/11/18 23:13:41]

Done.

+
+@interface GlobalShortcutListenerTap : NSObject {
+  @public

[Robert Sesek, 2013/11/18 15:25:03]

nit: one space before @public. But, I'm not sure you should have public ivars at
all.

[smus, 2013/11/18 23:13:41]

Removed @public member.

+  CFMachPortRef eventTap_;
+
+  @private
+  CFRunLoopSourceRef eventTapSource_;
+  CFRunLoopRef tapThreadRunLoop_;
+  GSL* gsl_;

[Robert Sesek, 2013/11/18 15:25:03]

Similarly, this is not an appropriate variable name. |shortcutListener_|?

[smus, 2013/11/18 23:13:41]

Done.

+}
+
+- (id)initWithGSL:(GSL*)gsl;
+- (void)startWatchingMediaKeys;
+- (void)stopWatchingMediaKeys;
+- (void)handleMediaKeyEvent:(NSEvent*)event;
+- (BOOL)performEventHandlerOnMainThread:(SEL)selector withEvent:(NSEvent*)event;
+@end
+
+// Processed events should propagate if they aren't handled by any listeners.
+// Returning event causes the event to propagate to other applications.
+// Returning NULL prevents the event from propagating.
+static CGEventRef tapEventCallback(

[Robert Sesek, 2013/11/18 15:25:03]

naming: TapEventCallback

You mix and match anonymous namespace and static. The preference is generally
for anonymous namespace.

[smus, 2013/11/18 23:13:41]

Done.

+    CGEventTapProxy proxy, CGEventType type, CGEventRef event, void* refcon) {
+  NSAutoreleasePool* pool = [NSAutoreleasePool new];
+  CGEventRef ret = event;

[Robert Sesek, 2013/11/18 15:25:03]

Naming: |ret|?

[smus, 2013/11/18 23:13:41]

Done.

+
+  GlobalShortcutListenerTap* self = (GlobalShortcutListenerTap*) refcon;

[Robert Sesek, 2013/11/18 15:25:03]

C-style casts are banned.

[smus, 2013/11/18 23:13:41]

Changed to static_cast.

+
+  // Handle the timeout case by re-enabling the tap.
+  if (type == kCGEventTapDisabledByTimeout) {
+    LOG(ERROR) << "Event tap was disabled by a timeout.";
+    CGEventTapEnable(self->eventTap_, TRUE);
+    // Release the event as soon as possible.
+    return ret;
+  }
+
+  // TODO(smus): do some error handling since eventWithCGEvent can fail.
+  NSEvent* nsEvent = [NSEvent eventWithCGEvent:event];

[Robert Sesek, 2013/11/18 15:25:03]

Why do you need to convert this to a NSEvent? Can't you just use the CGEvent
functions to get this exact same information?

[Robert Sesek, 2013/11/18 15:25:03]

Naming: in C/C++, use under_scores for variables. Only in @interface ivars and
@implementation methods do you use camelCase. Here and throughout this file.

[smus, 2013/11/18 23:13:41]

I looked for a way of doing this from the CGEvent, but could not find a good way
of getting at the illusive data1 field that NSEvent exposes. Any ideas how to do
this?

[smus, 2013/11/18 23:13:41]

Done.

[Robert Sesek, 2013/11/19 18:34:58]

int64_t subtype = CGEventGetIntegerValueField(event, kCGMouseEventSubtype);

https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz...

[smus, 2013/11/20 04:28:51]

Not sure I follow. Are you suggesting that CGEventGetIntegerValueField(event,
kCGMouseEventSubtype); corresponds to [nsEvent data1]?

[Robert Sesek, 2013/11/20 16:23:57]

What about kCGEventSourceUserData?

[smus, 2013/11/20 17:50:53]

No, unfortunately that field is always zero.

  int data1Carbon = CGEventGetIntegerValueField(event, kCGEventSourceUserData);
  int data1Cocoa = [ns_event data1];
  VLOG(0) << "Data1. carbon: " << data1Carbon << " cocoa: " << data1Cocoa;

Yields

[47820:1287:1120/083631:INFO:global_shortcut_listener_mac.mm(71)] Data1. carbon:
0 cocoa: 1313536

I asked around in #macdev on freenode as well, and was told to go the NSEvent
route. Any other ideas? 

+
+  // Handle media keys (PlayPause, NextTrack, PreviousTrack).
+  if (type != NX_SYSDEFINED ||
+      [nsEvent subtype] != SystemDefinedEventMediaKeys) {
+    int keyCode = (([nsEvent data1] & 0xFFFF0000) >> 16);
+    if (keyCode != NX_KEYTYPE_PLAY && keyCode != NX_KEYTYPE_NEXT &&
+        keyCode != NX_KEYTYPE_PREVIOUS && keyCode != NX_KEYTYPE_FAST &&
+        keyCode != NX_KEYTYPE_REWIND) {
+      // Release the event as soon as possible.
+      return ret;
+    }
+  }
+
+  // If we got here, we are dealing with a real media key event.
+  BOOL wasHandled = [self
+    performEventHandlerOnMainThread:@selector(handleMediaKeyEvent:)
+    withEvent:nsEvent];

[Finnur, 2013/11/18 11:55:57]

As I recall, from my brief exposure to Mac, the colons need to align... Maybe
I'm wrong.

[smus, 2013/11/18 23:13:41]

Couldn't confirm that from
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml

+  // Prevent the event from proagating to other mac applications if it was
+  // handled by Chrome.
+  if (wasHandled) {
+    ret = NULL;
+  }

[Finnur, 2013/11/18 11:55:57]

Yup, you guessed it (single-line if, no braces) :)

[smus, 2013/11/18 23:13:41]

Done.

+
+  [pool drain];
+  // By default, pass the event through.
+  return ret;
+}
+
+@implementation GlobalShortcutListenerTap
+
+- (id)initWithGSL:(GSL*)gsl {
+  gsl_ = gsl;

[Robert Sesek, 2013/11/18 15:25:03]

if ((self = [super init])) {
  shortcutListener_ = shortcutListener;
}
return self;

[smus, 2013/11/18 23:13:41]

Done.

+  return self;
+}
+
+- (void)eventTapThread {

[Robert Sesek, 2013/11/18 15:25:03]

Yikes – an entire thread just to listen for global keyboard shortcuts! Threads
aren't free; is this really necessary?

[smus, 2013/11/18 23:13:41]

I think this is the best practice. Doing this on the main thread risks blocking
it.

[Robert Sesek, 2013/11/19 18:34:58]

What are you worried about blocking? You already have a cross-thread
synchronization (waitUntilDone:YES below) to run something on the main thread,
so you're still doing work there. I don't see how that's any different than
installing the event tap on the main thread to begin with.

[smus, 2013/11/20 04:28:51]

Good point. Initially the thread was not synchronized (waitUntilDone:NO). I
changed to the synchronized model after realizing that we need to know if a
media key was handled by Chrome, in order to prevent the event from propagating
to other apps, but only in this case.

One possibility is to try to do waitUntilDone:YES, and then rebroadcast the
event (not sure if this is possible).

The other possibility is somehow doing this on the main thread (which seems to
be what you are suggesting). Unfortunately I don't know how to do that. The
event tap seems to need its own run loop, unless you know of some other way?

[Robert Sesek, 2013/11/20 16:23:57]

Rebroadcasting the event would post it out-of-order relative to the event
stream. It's certainly doable (CGEventTapPostEvent, e.g.), but I'm not sure it's
the right solution.
Why do you think it needs its own run loop? The main/UI thread will return a
CFRunLoopRef if you call CFRunLoopGetCurrent() in that thread.

Once that happens, I think you can fold all of the ObjC class into the C++
class.

[smus, 2013/11/20 17:50:53]

Okay, I've gone this route. I had a (big!) bug before where I was running
CFRunLoopRun() on the main thread again, which understandably caused some
undesired behavior. New approach seems to work pretty well, and definitely
simplifies the code. Folded the Obj-C class.

+  tapThreadRunLoop_ = CFRunLoopGetCurrent();
+  CFRunLoopAddSource(tapThreadRunLoop_, eventTapSource_,
+      kCFRunLoopCommonModes);
+  CFRunLoopRun();
+}
+
+- (BOOL)performEventHandlerOnMainThread:(SEL)selector
+      withEvent:(NSEvent*)event {
+  NSMutableDictionary* dict = [[NSMutableDictionary alloc] init];
+  [dict setObject:event forKey:EVENT_KEY];
+  [self performSelectorOnMainThread:selector
+      withObject:dict waitUntilDone:YES];
+  // Keep track of the result from the main thread to know if the event has
+  // been handled.
+  BOOL wasHandled = [[dict objectForKey:HANDLED_KEY] boolValue];
+  [dict release];
+  return wasHandled;
+}
+
+- (ui::KeyboardCode)mediaKeyCodeToKeyboardCode:(int)keyCode {
+  switch (keyCode) {
+    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;

[Finnur, 2013/11/18 11:55:57]

Is there no 'Stop' event?

For example, see Chaobin's patch:
https://codereview.chromium.org/64273008/diff/620001/chrome/browser/extension...

[smus, 2013/11/18 23:13:41]

Mac keyboards don't have this button.

+  }
+  return ui::VKEY_UNKNOWN;
+}
+
+- (void)startWatchingMediaKeys {
+  // Make sure there's no existing event tap.
+  assert(eventTap_ == NULL);

[Robert Sesek, 2013/11/18 15:25:03]

No assert(). Use Chromium's logging facilities for this.

[smus, 2013/11/18 23:13:41]

Done.

+
+  // Add an event tap to intercept the system defined media key events.
+  eventTap_ = CGEventTapCreate(kCGSessionEventTap,

[Robert Sesek, 2013/11/18 15:25:03]

Per
https://developer.apple.com/library/mac/documentation/Carbon/Reference/Quartz...:

This will only work if Chrome is running as root (it shouldn't be) or if
Universal Access/Assistive Devices is turned on.

[Robert Sesek, 2013/11/18 18:36:20]

Sorry, this isn't accurate. This code isn't tapping raw key events, but rather
system events. FWICT, this should be fine.

[smus, 2013/11/18 23:13:41]

Self-addressed.

[smus, 2013/11/18 23:13:41]

Done.

+      kCGHeadInsertEventTap,
+      kCGEventTapOptionDefault,
+      CGEventMaskBit(NX_SYSDEFINED),
+      tapEventCallback,
+      self);
+  assert(eventTap_ != NULL);
+
+  eventTapSource_ = CFMachPortCreateRunLoopSource(kCFAllocatorSystemDefault,
+      eventTap_, 0);
+  assert(eventTapSource_ != NULL);
+
+  // Run the event tap in separate thread to prevent blocking UI.
+  [NSThread detachNewThreadSelector:@selector(eventTapThread)
+      toTarget:self withObject:nil];
+}
+
+- (void)stopWatchingMediaKeys {
+  if (tapThreadRunLoop_) {
+    CFRunLoopStop(tapThreadRunLoop_);
+    tapThreadRunLoop_ = nil;
+  }
+
+  if (eventTap_) {
+    CFMachPortInvalidate(eventTap_);
+    CFRelease(eventTap_);
+    eventTap_ = nil;
+  }
+
+  if (eventTapSource_) {
+    CFRelease(eventTapSource_);
+    eventTapSource_ = nil;
+  }
+}
+
+// Event will have been retained in the other thread.
+- (void)handleMediaKeyEvent:(NSMutableDictionary*)dict {
+  NSEvent* event = [dict objectForKey:EVENT_KEY];
+
+  int keyCode = (([event data1] & 0xFFFF0000) >> 16);
+  int keyFlags = ([event data1] & 0x0000FFFF);
+  BOOL keyIsPressed = (((keyFlags & 0xFF00) >> 8)) == 0xA;
+
+  bool result = false;
+  if (keyIsPressed)
+    result = gsl_->OnMediaKeyEvent([self mediaKeyCodeToKeyboardCode:keyCode]);
+
+  [dict setObject:[NSNumber numberWithBool:result] forKey:HANDLED_KEY];
+}
+
+@end
+
+using content::BrowserThread;

[Robert Sesek, 2013/11/18 15:25:03]

This belongs after the #includes, not here.

[smus, 2013/11/18 23:13:41]

Done.

+
+namespace {
+
+static base::LazyInstance<extensions::GlobalShortcutListenerMac> instance =

[Robert Sesek, 2013/11/18 15:25:03]

Naming: g_instance

[smus, 2013/11/18 23:13:41]

Done.

+    LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+namespace extensions {
+
+// static
+GlobalShortcutListener* GlobalShortcutListener::GetInstance() {
+  LOG(ERROR) << "GlobalShortcutListener GetInstance";
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  return instance.Pointer();
+}
+
+GlobalShortcutListenerMac::GlobalShortcutListenerMac()
+    : is_listening_(false) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // TODO(implementor): Remove this.
+  LOG(ERROR) << "GlobalShortcutListenerMac object created";

[Finnur, 2013/11/18 11:55:57]

You can remove this and other LOG(ERROR) statements added.

[smus, 2013/11/18 23:13:41]

Done.

+
+  tap_.reset([[GlobalShortcutListenerTap alloc] initWithGSL:this]);
+}
+
+GlobalShortcutListenerMac::~GlobalShortcutListenerMac() {

[Robert Sesek, 2013/11/18 15:25:03]

What about all the registered hotkeys? Won't those listeners be leaked?

[smus, 2013/11/18 23:13:41]

Following other implementations (X11 and Win). I think the way it works is all
registered accelerators should be unregistered first (via
UnregisterAccelerator), and only then does this class get destroyed. cc: finnur@

[Finnur, 2013/11/19 13:10:05]

Yes, we'll get calls to Unregister before the object is destroyed and the
StopListening call DCHECKs if there's anything remaining on shutdown.

+  if (is_listening_)
+    StopListening();
+}
+
+void GlobalShortcutListenerMac::StartListening() {

[Robert Sesek, 2013/11/18 15:25:03]

The Start/StopListeneing pair of methods only affect media keys, not general
hotkeys. Should they? If not, this deserves a comment.

[smus, 2013/11/18 23:13:41]

Added clarifying comment.

[Finnur, 2013/11/19 13:10:05]

So, this Start/StopListening pair is meant as a convenience for platforms that
can turn the faucet on (when getting the first accelerator) and off (when
removing the last one). For example, Windows Chrome uses these events to
register for an observer on SingletonHWND on Start and remove it on Stop. In
other words, the Start/Stop pair avoids the need to check specifically whenever
you add a shortcut whether it is the first shortcut to be added (so that the
faucet needs to be turned on).

This isn't quite as clean on Mac because you have a split system (one for
MediaKeys and another for the rest). If you start watching Media Keys here then
you end up in a situation where you might have extensions register only
Non-MediaKey global shortcuts but you fire up the thread and start listening for
MediaKeys anyway.

I think you probably need to keep track of whether you've started the media key
loop and start it on seeing the first MediaKey shortcut (and stop it when the
last is removed).

[smus, 2013/11/20 04:28:51]

Ok, made this change.

+  DCHECK(!is_listening_);  // Don't start twice.
+  DCHECK(!hotkey_ids_.empty());  // Don't start if no hotkey registered.
+  DCHECK(!id_hotkeys_.empty());
+  DCHECK(!id_hotkey_refs_.empty());
+  LOG(ERROR) << "GlobalShortcutListenerMac StartListening";
+  is_listening_ = true;
+
+  [tap_ startWatchingMediaKeys];
+}
+
+void GlobalShortcutListenerMac::StopListening() {
+  DCHECK(is_listening_);  // No point if we are not already listening.
+  DCHECK(hotkey_ids_.empty());  // Make sure the set is clean.
+  DCHECK(id_hotkeys_.empty());
+  DCHECK(id_hotkey_refs_.empty());
+  LOG(ERROR) << "GlobalShortcutListenerMac StopListening";
+  is_listening_ = false;
+
+  [tap_ stopWatchingMediaKeys];
+}
+
+void GlobalShortcutListenerMac::RegisterAccelerator(
+    const ui::Accelerator& accelerator,
+    GlobalShortcutListener::Observer* observer) {
+  LOG(ERROR) << "GlobalShortcutListenerMac RegisterAccelerator";
+  VLOG(0) << "Registered keyCode: " << accelerator.key_code()
+      << ", modifiers: " << accelerator.modifiers();
+  // To implement:
+  // 1) Convert modifiers to platform specific modifiers.
+  bool isMediaKey = (accelerator.modifiers() == 0);
+  VLOG(0) << "isMediaKey: " << isMediaKey;
+  if (!isMediaKey) {
+    RegisterHotKey(accelerator);
+  }

[Finnur, 2013/11/18 11:55:57]

nit: Single line. :)

[smus, 2013/11/18 23:13:41]

Done.

+  // 2) Register for the hotkey.
+  // 3) If not successful, log why.
+  // 4) Else, call base class RegisterAccelerator.

[Finnur, 2013/11/18 11:55:57]

You can remove these comments, they were only meant as convenience for the
implementor.

[smus, 2013/11/18 23:13:41]

Done.

+  id_hotkeys_[hotkey_id] = accelerator;
+  hotkey_ids_[accelerator] = hotkey_id;
+  hotkey_id += 1;
+  GlobalShortcutListener::RegisterAccelerator(accelerator, observer);
+}
+
+void GlobalShortcutListenerMac::UnregisterAccelerator(
+    const ui::Accelerator& accelerator,
+    GlobalShortcutListener::Observer* observer) {
+  LOG(ERROR) << "GlobalShortcutListenerMac UnregisterAccelerator";
+  // To implement:
+  // 1) Unregister for the hotkey.
+  // 2) Call base class UnregisterAccelerator.

[Finnur, 2013/11/18 11:55:57]

Same here.

[smus, 2013/11/18 23:13:41]

Done.

+  bool isMediaKey = (accelerator.modifiers() == 0);

[Finnur, 2013/11/18 11:55:57]

Chaobin is adding a static IsMediaKey function to the CommandService, we should
probably use that?

[smus, 2013/11/18 23:13:41]

Oh excellent, I added my own for now.

+  VLOG(0) << "isMediaKey: " << isMediaKey;
+  if (!isMediaKey) {
+    UnregisterHotKey(accelerator);
+  }

[Finnur, 2013/11/18 11:55:57]

nit: Single line, your favorite. :)

[smus, 2013/11/18 23:13:41]

Done.

+
+  int id = hotkey_ids_[accelerator];
+  id_hotkeys_.erase(id);
+  hotkey_ids_.erase(accelerator);
+  GlobalShortcutListener::UnregisterAccelerator(accelerator, observer);
+}
+
+bool GlobalShortcutListenerMac::OnKeyEvent(EventHotKeyID hotKeyID) {
+  // Look up the accelerator based on this hot key ID.
+  VLOG(0) << "OnKeyEvent! hotKeyID: " << hotKeyID.id;
+  ui::Accelerator accelerator = id_hotkeys_[hotKeyID.id];
+  VLOG(0) << "Key code: " << accelerator.key_code() <<
+      " modifiers: " << accelerator.modifiers();
+  instance.Get().NotifyKeyPressed(accelerator);

[Robert Sesek, 2013/11/18 15:25:03]

Why are you not using the |this| pointer? This method isn't static.

[smus, 2013/11/18 23:13:41]

Fair point (I think). If so, same should apply to _win and _x11: 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[Finnur, 2013/11/19 13:10:05]

Yeah, looks like.

On 2013/11/18 23:13:41, smus wrote:

+  return true;
+}
+
+// Returns true iff event was handled.
+bool GlobalShortcutListenerMac::OnMediaKeyEvent(ui::KeyboardCode keyCode) {
+  VLOG(0) << "OnMediaKeyEvent! keyCode: " << keyCode;
+  // Create an accelerator corresponding to the keyCode.
+  ui::Accelerator accelerator(keyCode, 0);
+  // Look for a match with a bound hotkey.
+  if (hotkey_ids_.find(accelerator) != hotkey_ids_.end()) {
+    // If matched, callback to the event handling system.
+    instance.Get().NotifyKeyPressed(accelerator);
+    return true;
+  }
+  return false;
+}
+
+OSStatus HotKeyHandler(EventHandlerCallRef nextHandler, EventRef theEvent,

[Robert Sesek, 2013/11/18 15:25:03]

This doesn't belong here. This belongs with your other callback functions in the
anonymous namespace below the @interface.

[Robert Sesek, 2013/11/18 15:25:03]

naming: You use HotKey here but Hotkey elsewhere. Please pick one and be
consistent.

[smus, 2013/11/18 23:13:41]

Done.

[smus, 2013/11/18 23:13:41]

Done.

+    void *userData) {
+  VLOG(0) << "HotKeyHandler fired with event: " << theEvent;
+  // Extract the hotkey from the event.
+  EventHotKeyID hotKeyID;
+  int result = GetEventParameter(theEvent, kEventParamDirectObject,
+      typeEventHotKeyID, NULL, sizeof(hotKeyID), NULL, &hotKeyID);
+  assert(result == noErr);
+
+  // Callback to the parent class.
+  GlobalShortcutListenerMac* gsl = (GlobalShortcutListenerMac*) userData;
+  gsl->OnKeyEvent(hotKeyID);
+  return noErr;
+}
+
+void GlobalShortcutListenerMac::RegisterHotKey(ui::Accelerator accelerator) {
+  VLOG(0) << "Registering hotkey. Windows keycode: " << accelerator.key_code();
+  EventHotKeyRef hotKeyRef;
+  EventHotKeyID hotKeyID;
+  EventHandlerUPP hotKeyFunction = NewEventHandlerUPP(HotKeyHandler);
+
+  EventTypeSpec eventType;
+  eventType.eventClass = kEventClassKeyboard;
+  eventType.eventKind = kEventHotKeyPressed;
+  InstallApplicationEventHandler(hotKeyFunction, 1, &eventType, this, NULL);
+
+  // TODO: Understand what signature means.

[Robert Sesek, 2013/11/18 15:25:03]

I think this is important to understand before committing.

[smus, 2013/11/18 23:13:41]

Agreed. Added a comment explaining what it is.

+  hotKeyID.signature = 31337;
+  hotKeyID.id = hotkey_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;
+  unichar characterIgnoringModifiers;
+  int keyCode = ui::MacKeyCodeForWindowsKeyCode(accelerator.key_code(), 0,
+      &character, &characterIgnoringModifiers);
+  VLOG(0) << "RegisterHotKey. Code: " << keyCode << " modifier: " << modifiers;
+
+  // TODO: Move away from hardcoded CMD + Enter event.

[Finnur, 2013/11/18 11:55:57]

What does this mean?

[smus, 2013/11/18 23:13:41]

Stale comment. Removed.

+  RegisterEventHotKey(keyCode, modifiers, hotKeyID,
+      GetApplicationEventTarget(), 0, &hotKeyRef);
+
+  id_hotkey_refs_[hotkey_id] = hotKeyRef;

[Finnur, 2013/11/18 11:55:57]

Don't you need to advance hotkey_id now?

[smus, 2013/11/18 23:13:41]

Added a comment explaining (it happens in the caller -- kinda ugly, not sure
what to do about it).

+}
+
+void GlobalShortcutListenerMac::UnregisterHotKey(ui::Accelerator accelerator) {
+  // Get the ref corresponding to this accelerator.
+  int id = hotkey_ids_[accelerator];
+  EventHotKeyRef ref = id_hotkey_refs_[id];
+  // Unregister the event hot key.
+  UnregisterEventHotKey(ref);
+
+  // Remove the event from the mapping.
+  id_hotkey_refs_.erase(id);
+}
+
+

[Finnur, 2013/11/18 11:55:57]

nit: Extra line break.

[smus, 2013/11/18 23:13:41]

Done.

+}  // namespace extensions