Mac global keybindings

chrome/browser/extensions/global_shortcut_listener_mac.h

Index: chrome/browser/extensions/global_shortcut_listener_mac.h
===================================================================
--- chrome/browser/extensions/global_shortcut_listener_mac.h	(revision 239014)
+++ chrome/browser/extensions/global_shortcut_listener_mac.h	(working copy)
@@ -5,27 +5,40 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_MAC_H_
 #define CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_MAC_H_
 
-#include "base/lazy_instance.h"
 #include "chrome/browser/extensions/global_shortcut_listener.h"
 
+#include <Carbon/Carbon.h>
+#include <CoreFoundation/CoreFoundation.h>
+
+#include <map>
+
+#include "base/lazy_instance.h"

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

This is obsolete now.

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

Done.

+#include "base/mac/scoped_nsobject.h"
+
 namespace extensions {
 
 // Mac-specific implementation of the GlobalShortcutListener class that
 // listens for global shortcuts. Handles basic keyboard intercepting and
 // forwards its output to the base class for processing.
-// TODO(finnur/smus): Implement this class.
+//
+// This class does two things:
+// 1. Intercepts media keys. Uses an event tap for intercepting media keys
+// (PlayPause, NextTrack, PreviousTrack).
+// 2. Binds keyboard shortcuts (hot keys). Carbon RegisterEventHotKey API for
+// binding to non-media key global hot keys (eg. Command-Shift-1).
 class GlobalShortcutListenerMac : public GlobalShortcutListener {
  public:
   virtual ~GlobalShortcutListenerMac();

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

Destructor follows constructor.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

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

Done.

+  GlobalShortcutListenerMac();
 
   virtual void StartListening() OVERRIDE;
   virtual void StopListening() OVERRIDE;
 
+  // Keyboard event callbacks.

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

These two should be private. They’re not part of the external interface and it’s
not sensible for code outside of this class to have access to them.

This means that HotKeyHandler, currently in the implementation file in an
unnamed namespace, needs to be promoted to be a private static function in this
class as well, to be able to call the private OnKeyEvent.

This follows the same pattern that you used for EventTapCallback calling
OnMediaKeyEvent, which is correct (as long as OnMediaKeyEvent becomes private
too).

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

Done.

+  void OnKeyEvent(EventHotKeyID hot_key_id);

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

On the basis of the naming that’s now fairly uniform in this class (thanks!),
this should be OnHotKeyEvent, to contrast with OnMediaKeyEvent.

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

Done.

+  bool OnMediaKeyEvent(int key_code);
+
  private:
-  friend struct base::DefaultLazyInstanceTraits<GlobalShortcutListenerMac>;
-
-  GlobalShortcutListenerMac();
-
   // Register an |accelerator| with the particular |observer|.
   virtual void RegisterAccelerator(

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

How is this ever called? It’s private, there are no “friends,” and there are no
callers within this class.

Because you’ve marked it OVERRIDE in a class that extends GlobalShortcutListener
in which this method is public, I assume that you just meant to put this (and
UnregisterAccelerator) into the public section.

       const ui::Accelerator& accelerator,
@@ -35,9 +48,49 @@
       const ui::Accelerator& accelerator,
       GlobalShortcutListener::Observer* observer) OVERRIDE;
 
+  // Mac-specific functions for registering hot keys with modifiers.
+  void 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.

+  void UnregisterHotKey(const ui::Accelerator& accelerator);
+
+  // Enable and disable the media key event tap.
+  void StartWatchingMediaKeys();
+  void StopWatchingMediaKeys();
+
+  // Whether or not any media keys are currently registered.
+  bool IsAnyMediaKeyRegistered();
+
+  // The callback for when an event tap happens.
+  static CGEventRef EventTapCallback(
+      CGEventTapProxy proxy, CGEventType type, CGEventRef event, void* refcon);
+
   // Whether this object is listening for global shortcuts.
   bool is_listening_;
 
+  // The hotkey identifier for the next global shortcut that is added.
+  typedef int HotKeyId;

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

Move all of the typedefs up to the top of the private section. Their names are
self-explanatory, they don’t need comments. You can leave the comments here on
the declarations of the member variables.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

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

Done.

+  HotKeyId hot_key_id_;
+
+  // A map of all hotkeys (media keys and shortcuts) mapping to their
+  // corresponding hotkey IDs. For quickly finding if an accelerator is
+  // registered.
+  typedef std::map<ui::Accelerator, HotKeyId> HotKeyIdMap;
+  HotKeyIdMap hot_key_ids_;
+
+  // The inverse map for quickly looking up accelerators by hotkey id.
+  typedef std::map<HotKeyId, ui::Accelerator> IdHotKeyMap;
+  IdHotKeyMap id_hot_keys_;
+
+  // Keyboard shortcut IDs to hotkeys map for unregistration.
+  typedef std::map<HotKeyId, EventHotKeyRef> IdHotKeyRefMap;
+  IdHotKeyRefMap id_hot_key_refs_;
+
+  // Event tap for intercepting mac media keys.
+  CFMachPortRef event_tap_;
+  CFRunLoopSourceRef event_tap_source_;
+
+  // Event handler for keyboard shortcut hot keys.
+  EventHandlerRef event_handler_;
+
   DISALLOW_COPY_AND_ASSIGN(GlobalShortcutListenerMac);
 };