Mac global keybindings

chrome/browser/extensions/global_shortcut_listener_mac.h

 // 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.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_MAC_H_
 #define CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_MAC_H_
 
+#include "chrome/browser/extensions/global_shortcut_listener.h"
+
+#include <set>

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

#include <map> since you use it.

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

Done.

+#include <IOKit/hidsystem/ev_keymap.h>

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

IOKit/IOKit.h

Does this need to be in the .h?

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

Moved to .mm, but unfortunately IOKit/IOKit.h doesn't exist. Nor does
IOKit/IOKitLib.h include the NX_ constants I need.

+#include <Carbon/Carbon.h>

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

Same. Does this need to be in the .h?

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

Yes, this provides EventHotKeyRef

+
+#include "ApplicationServices/ApplicationServices.h"

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

This is a system header, so it should be in <>. Does this need to be in the .h
though, or can it be in the .mm?

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

Moved to .mm and changed to <> syntax.

 #include "base/lazy_instance.h"
-#include "chrome/browser/extensions/global_shortcut_listener.h"
+#include "base/mac/scoped_nsobject.h"
+
+@class GlobalShortcutListenerTap;
 
 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.

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

This comment should be revised. It should also detail how to use this class.

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

Done.

 class GlobalShortcutListenerMac : public GlobalShortcutListener {
  public:
   virtual ~GlobalShortcutListenerMac();
 
   virtual void StartListening() OVERRIDE;
   virtual void StopListening() OVERRIDE;
 
+  // Keyboard event callbacks.
+  bool OnKeyEvent(EventHotKeyID hotKeyID);
+  bool OnMediaKeyEvent(ui::KeyboardCode keyCode);
+
+

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

nit: Remove line break.

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

Done.

  private:
   friend struct base::DefaultLazyInstanceTraits<GlobalShortcutListenerMac>;
 
   GlobalShortcutListenerMac();
 
   // Register an |accelerator| with the particular |observer|.
   virtual void RegisterAccelerator(
       const ui::Accelerator& accelerator,
       GlobalShortcutListener::Observer* observer) OVERRIDE;
   // Unregister an |accelerator| with the particular |observer|.
   virtual void UnregisterAccelerator(
       const ui::Accelerator& accelerator,
       GlobalShortcutListener::Observer* observer) OVERRIDE;
 
+  // Mac-specific function for registering a hotkey.
+  void RegisterHotKey(ui::Accelerator accelerator);

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

Make these be const& parameters.

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

Done.

+  void UnregisterHotKey(ui::Accelerator accelerator);
+
   // Whether this object is listening for global shortcuts.
   bool is_listening_;
+  int hotkey_id = 0;

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

nit: Add line break and comment.
Also, why not trailing underscore?

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

Done.

+
+  // A map of registered accelerators and their registration ids.
+  // TODO: Consider using a bimap structure instead.

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

style: All TODOs need an actor, e.g.:
// TODO(smus): Lorum ipsus.

(same goes for .mm file)

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

Done.

+  typedef std::map< ui::Accelerator, int > HotkeyIdMap;
+  HotkeyIdMap hotkey_ids_;
+  typedef std::map< int, ui::Accelerator > IdHotkeyMap;
+  IdHotkeyMap id_hotkeys_;
+
+  // A map of hot key refs so they can be unregistered later.

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

nit: hot key? or hotkey? :)

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

hotkey!!!

+  typedef std::map< int, EventHotKeyRef > IdHotkeyRefMap;
+  IdHotkeyRefMap id_hotkey_refs_;

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

That's a lot of maps, Mr. Magellan. It would be good if you could document why
we need so many (and what each is used for, media keys only? all keys?).

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

Ok, documented. I could have used one fewer map but that would have meant doing
linear search.

+
+

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

nit: Remove line break.

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

Done.

+  // A pointer to the global shortcut listener tap.

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

This is kind of self-evident from the code. It would be more apt to describe
what you use it for (media keys? other keys? all keys?)

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

Done.

+  base::scoped_nsobject<GlobalShortcutListenerTap> tap_;
 
   DISALLOW_COPY_AND_ASSIGN(GlobalShortcutListenerMac);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_MAC_H_