Refactor GlobalShortcutListener.

chrome/browser/extensions/global_shortcut_listener.cc

Index: chrome/browser/extensions/global_shortcut_listener.cc
diff --git a/chrome/browser/extensions/global_shortcut_listener.cc b/chrome/browser/extensions/global_shortcut_listener.cc
index aad5453eca527b92e1bc426da2f8bbef700db1d7..fc8b7eda8a3195aabf5e218d04f0ae34b44d4df5 100644
--- a/chrome/browser/extensions/global_shortcut_listener.cc
+++ b/chrome/browser/extensions/global_shortcut_listener.cc
@@ -3,45 +3,59 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/global_shortcut_listener.h"
-#include "chrome/browser/profiles/profile.h"
+
+#include "base/logging.h"
+#include "content/public/browser/browser_thread.h"
 #include "ui/base/accelerators/accelerator.h"
 
+using content::BrowserThread;
+
 namespace extensions {
 
 GlobalShortcutListener::GlobalShortcutListener() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 GlobalShortcutListener::~GlobalShortcutListener() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(accelerator_map_.empty());  // Make sure we've cleaned up.
 }
 
-void GlobalShortcutListener::RegisterAccelerator(
+bool GlobalShortcutListener::RegisterAccelerator(
     const ui::Accelerator& accelerator, Observer* observer) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   AcceleratorMap::const_iterator it = accelerator_map_.find(accelerator);
-  if (it == accelerator_map_.end()) {
-    if (accelerator_map_.empty())
-      GlobalShortcutListener::GetInstance()->StartListening();
-    Observers* observers = new Observers;
-    observers->AddObserver(observer);
-    accelerator_map_[accelerator] = observers;
-  } else {
-    // Make sure we don't register the same accelerator twice.
-    DCHECK(!accelerator_map_[accelerator]->HasObserver(observer));
-    accelerator_map_[accelerator]->AddObserver(observer);
-  }
+  // The accelerator has been registered.

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

This is a personal preference, but I find comments like this that say something
about application state to be most effective when they’re written in the block
that they apply to, instead of outside of it. So in this case:

  if (it != accelerator_map_.end()) {
    // The accelerator has been registered.
    return false;
  }

since the accelerator hasn’t necessarily been registered unless it was found in
the map, and only then do you want to “return false.”

(You’d need to add the {braces} now because the conditioned block takes up more
than one line.)

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

Done.

+  if (it != accelerator_map_.end())
+    return false;
+
+  // If the platform-specific registration fails, mostly likely the shortcut has
+  // been registered by other native applications.
+  if (!RegisterAcceleratorImpl(accelerator))
+    return false;
+
+  if (accelerator_map_.empty())
+    GlobalShortcutListener::GetInstance()->StartListening();
+
+  accelerator_map_[accelerator] = observer;
+  return true;
 }
 
 void GlobalShortcutListener::UnregisterAccelerator(
     const ui::Accelerator& accelerator, Observer* observer) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   AcceleratorMap::iterator it = accelerator_map_.find(accelerator);
+  // We should never get asked to unregister something that we didn't register.
   DCHECK(it != accelerator_map_.end());

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

Good. Does DCHECK_NE work on this line, and DCHECK_EQ on line 53?

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

Actually, it will be a compile error if you use DCHECK_NE and DCHECK_EQ for
Iterator. These equality/inequality checks require values must have
operator<<(ostream, ...) defined.

-  DCHECK(it->second->HasObserver(observer));
-  it->second->RemoveObserver(observer);
-  if (!it->second->might_have_observers()) {
-    accelerator_map_.erase(it);
-    if (accelerator_map_.empty())
-      GlobalShortcutListener::GetInstance()->StopListening();
-  }
+  // The caller should call this function with the right observer.
+  DCHECK(it->second == observer);
+
+  UnregisterAcceleratorImpl(accelerator);
+  accelerator_map_.erase(it);
+  if (accelerator_map_.empty())
+    GlobalShortcutListener::GetInstance()->StopListening();
 }
 
 void GlobalShortcutListener::NotifyKeyPressed(
@@ -53,10 +67,8 @@ void GlobalShortcutListener::NotifyKeyPressed(
     NOTREACHED();
     return;  // No-one is listening to this key.
   }
-  // The observer list should not be empty.
-  DCHECK(iter->second->might_have_observers());
 
-  FOR_EACH_OBSERVER(Observer, *(iter->second), OnKeyPressed(accelerator));
+  iter->second->OnKeyPressed(accelerator);
 }
 
 }  // namespace extensions