Track the active ExtensionKeybindingRegistry and make it available to EventRewriter.

chrome/browser/extensions/extension_commands_global_registry.h

Index: chrome/browser/extensions/extension_commands_global_registry.h
diff --git a/chrome/browser/extensions/extension_commands_global_registry.h b/chrome/browser/extensions/extension_commands_global_registry.h
index 92a7a8e42b4073ab50a7802fea22dc99b01f798e..f0d1f862cac17fc8ad8af515d5466492168bb39b 100644
--- a/chrome/browser/extensions/extension_commands_global_registry.h
+++ b/chrome/browser/extensions/extension_commands_global_registry.h
@@ -33,6 +33,12 @@ class ExtensionCommandsGlobalRegistry
       public ExtensionKeybindingRegistry,
       public GlobalShortcutListener::Observer {
  public:
+  ExtensionKeybindingRegistry* active_registry() { return active_registry_; }

[Finnur, 2014/09/12 11:25:44]

I'd argue that documenting this function is even more important than documenting
the variable below. I'm also a little bit worried that this is ambiguous
(because there is also an active *global* commands registry, which this function
does *not* return). 

I propose a name change:


// Returns which non-global command registry is active (belonging
// to the currently active window).
registry_for_active_window()

set_registry_for_active_window()

and

ExtensionKeybindingRegistry* registry_for_active_window_;

[David Tseng, 2014/09/12 23:18:33]

Right, this was my concern as well. But, considering I didn't have a strong
opinion on the tracker strategy (though I'm now rethinking that), we went with
this somewhat confusing approach.
Apparently, trivial accessers don't require comments, but in this case, would be
helpful yes.
Ok. Made this change.

+
+  void set_active_registry(ExtensionKeybindingRegistry* registry) {
+    active_registry_ = registry;
+  }

[Finnur, 2014/09/12 11:25:44]

Also, shouldn't these two functions be below the ctor and dtor?

[David Tseng, 2014/09/12 23:18:33]

Done.

+
   // BrowserContextKeyedAPI implementation.
   static BrowserContextKeyedAPIFactory<ExtensionCommandsGlobalRegistry>*
       GetFactoryInstance();
@@ -70,6 +76,10 @@ class ExtensionCommandsGlobalRegistry
   // Weak pointer to our browser context. Not owned by us.
   content::BrowserContext* browser_context_;
 
+  // The ExtensionKeybindingRegistry belonging to the active window. Only valid

[Finnur, 2014/09/12 11:25:44]

I'd make the distinction non-global and global clear here, with something like:

// The global commands registry not only keeps track of global commands
// registered, but also of which non-global command registry is active
// (belonging to the currently active window). Only valid for TOOLKIT_VIEWS
// and NULL otherwise.

[David Tseng, 2014/09/12 23:18:33]

Done.

+  // for TOOLKIT_VIEWS and NULL otherwise.
+  ExtensionKeybindingRegistry* active_registry_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionCommandsGlobalRegistry);
 };