Refactor GlobalShortcutListener.

chrome/browser/extensions/global_shortcut_listener_x11.h

Index: chrome/browser/extensions/global_shortcut_listener_x11.h
diff --git a/chrome/browser/extensions/global_shortcut_listener_x11.h b/chrome/browser/extensions/global_shortcut_listener_x11.h
index d710206e62422e9d0e70713f9a974f4d03556a0d..2ad6c3e2621807d4b4708e304737f752fdba00a9 100644
--- a/chrome/browser/extensions/global_shortcut_listener_x11.h
+++ b/chrome/browser/extensions/global_shortcut_listener_x11.h
@@ -5,10 +5,9 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_X11_H_
 #define CHROME_BROWSER_EXTENSIONS_GLOBAL_SHORTCUT_LISTENER_X11_H_
 
-#include <set>
 #include <X11/Xlib.h>
+#include <set>
 
-#include "base/lazy_instance.h"
 #include "base/message_loop/message_pump_dispatcher.h"
 #include "chrome/browser/extensions/global_shortcut_listener.h"
 
@@ -25,28 +24,20 @@ namespace extensions {
 class GlobalShortcutListenerX11 : public GlobalShortcutListener,
                                   public base::MessagePumpDispatcher {

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

I think that MessagePumpDispatcher and the Dispatch function are only needed #if
!defined(TOOLKIT_GTK) in the same way that some of the other stuff is only used
#if defined(TOOLKIT_GTK). As it stands now, Dispatch will never be called if
TOOLKIT_GTK is true because StartListening won’t call
AddDispatcherForRootWindow.

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

This class is implemented by me. I knew the problem and asked reviewers for
style suggestion when committing it. With the ifdefs, code will become:

class GlobalShortcutListenerX11
    :
#if !defined(TOOLKIT_GTK)
     public base::MessagePumpDispatcher,
#endif
     public GlobalShortcutListener {

Personally speaking, it seems a little ugly (Do you have any good suggestion?).
Elliot told me that he wouldn't bother, so I left it behind[0]. But since you
raise it again, let 's fix it this time. :)

[0]:
https://codereview.chromium.org/26775005/diff/1/chrome/browser/extensions/glo...

[Mark Mentovai, 2014/01/02 17:42:37]

zhchbin wrote:
It may be ugly, but you had already worked #ifdefs into this code to handle the
TOOLKIT_GTK case, so it seems kind of silly to not do the same for the
!TOOLKIT_GTK case. I think it’s good now.

  public:
+  GlobalShortcutListenerX11();
   virtual ~GlobalShortcutListenerX11();
 
-  // GlobalShortcutListener implementation.
-  virtual void StartListening() OVERRIDE;
-  virtual void StopListening() OVERRIDE;
-
   // base::MessagePumpDispatcher implementation.
   virtual bool Dispatch(const base::NativeEvent& event) OVERRIDE;
 
  private:
-  friend struct base::DefaultLazyInstanceTraits<GlobalShortcutListenerX11>;
-
-  GlobalShortcutListenerX11();
-
-  // 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;
+  // GlobalShortcutListener implementation.
+  virtual void StartListening() OVERRIDE;
+  virtual void StopListening() OVERRIDE;
+  virtual bool RegisterAcceleratorImpl(
+      const ui::Accelerator& accelerator) OVERRIDE;
+  virtual void UnregisterAcceleratorImpl(
+      const ui::Accelerator& accelerator) OVERRIDE;
 
 #if defined(TOOLKIT_GTK)
   // Callback for XEvents of the default root window.