Refactor GlobalShortcutListener.

chrome/browser/extensions/global_shortcut_listener.h

Index: chrome/browser/extensions/global_shortcut_listener.h
diff --git a/chrome/browser/extensions/global_shortcut_listener.h b/chrome/browser/extensions/global_shortcut_listener.h
index c61abe65f0c16b91c722c33a1c9b165583e20af3..76401efe23fa6399b3d0c45b348a76277ff27c20 100644
--- a/chrome/browser/extensions/global_shortcut_listener.h
+++ b/chrome/browser/extensions/global_shortcut_listener.h
@@ -7,9 +7,8 @@
 
 #include <map>
 
-#include "base/observer_list.h"
+#include "base/basictypes.h"
 #include "ui/events/keycodes/keyboard_codes.h"
-#include "ui/gfx/native_widget_types.h"
 
 namespace ui {
 class Accelerator;
@@ -18,7 +17,7 @@ class Accelerator;
 namespace extensions {
 
 // Platform-neutral implementation of a class that keeps track of observers and
-// monitors keystrokes. It relays messages to the appropriate observers when a
+// monitors keystrokes. It relays messages to the appropriate observer when a
 // global shortcut has been struck by the user.
 class GlobalShortcutListener {
  public:
@@ -32,31 +31,37 @@ class GlobalShortcutListener {
 
   static GlobalShortcutListener* GetInstance();
 
-  // Implemented by platform-specific implementations of this class.
-  virtual void StartListening() = 0;
-  virtual void StopListening() = 0;
-
-  // Register an observer for when a certain |accelerator| is struck.
-  virtual void RegisterAccelerator(
-      const ui::Accelerator& accelerator, Observer* observer);
+  // Register an observer for when a certain |accelerator| is struck. Return
+  // true if register successfully, false if the specificied |accelerator| has

[Mark Mentovai, 2013/12/20 17:54:47]

Minor grammar nit: “Returns true if registered successfully, or false if…”

[zhchbin, 2013/12/21 09:10:25]

Done.

+  // been registered by callers or other native applications. Note that we don’t

[Mark Mentovai, 2013/12/20 17:54:47]

Instead of “by callers,” say “by another caller”

[Mark Mentovai, 2013/12/20 17:54:47]

The apostrophe in “don’t” is a Unicode right single quote (U+2019) in UTF-8. We
typically just use ASCII in source code. You don’t have to change this unless
something breaks, but I’m mentioning it because something might break: the
PRESUBMIT line length check, for example, might not understand UTF-8.

(Now I see where you got this comment from. Most people don’t use the nice
typographic quotes, but I know someone who does… :)

[zhchbin, 2013/12/21 09:10:25]

Done.

[zhchbin, 2013/12/21 09:10:25]

Done.

+  // support recognizing that an accelerator has been registered by another
+  // application on all platforms. This is a per-platform consideration.
+  bool RegisterAccelerator(const ui::Accelerator& accelerator,
+                           Observer* observer);
   // Stop listening for the given |accelerator|.
-  virtual void UnregisterAccelerator(
-      const ui::Accelerator& accelerator, Observer* observer);
+  void UnregisterAccelerator(const ui::Accelerator& accelerator,
+                             Observer* observer);
 
  protected:
   GlobalShortcutListener();
 
   // Called by platform specific implementations of this class whenever a key
-  // is struck. Only called for keys that have observers registered.
+  // is struck. Only called for keys that have an observer registered.
   void NotifyKeyPressed(const ui::Accelerator& accelerator);
 
+ private:
+  // Implemented by platform-specific implementations of this class.
+  virtual void StartListening() = 0;

[Mark Mentovai, 2013/12/20 17:54:47]

To help people write subclass implementations, you should add a little more
guidance on how these work.

“Start/StopListening are called when transitioning between zero and nonzero
registered accelerators. StartListening will be called after
RegisterAcceleratorImpl and StopListening will be called after
UnregisterAcceleratorImpl.”

[zhchbin, 2013/12/21 09:10:25]

Done.

+  virtual void StopListening() = 0;
+  virtual bool RegisterAcceleratorImpl(const ui::Accelerator& accelerator) = 0;

[Mark Mentovai, 2013/12/20 17:54:47]

And this one can use some guidance on what the return value is about: “An
implementation should return false if registration did not complete
successfully.”

[zhchbin, 2013/12/21 09:10:25]

Done.

+  virtual void UnregisterAcceleratorImpl(
+      const ui::Accelerator& accelerator) = 0;
+
   // The map of accelerators that have been successfully registered as global
-  // shortcuts and their observer lists.
-  typedef ObserverList<Observer> Observers;
-  typedef std::map< ui::Accelerator, Observers* > AcceleratorMap;
+  // shortcuts and their observer.
+  typedef std::map<ui::Accelerator, Observer*> AcceleratorMap;
   AcceleratorMap accelerator_map_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(GlobalShortcutListener);
 };