Track the active ExtensionKeybindingRegistry and make it available to EventRewriter.

chrome/browser/extensions/extension_commands_global_registry.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_EXTENSION_COMMANDS_GLOBAL_REGISTRY_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_COMMANDS_GLOBAL_REGISTRY_H_
 
 #include <map>
 #include <string>
 
@@ skipping 15 matching lines @@
 // API).
 // Note: It handles regular extension commands (not browserAction and pageAction
 // popups, which are not bindable to global shortcuts). This class registers the
 // accelerators on behalf of the extensions and routes the commands to them via
 // the BrowserEventRouter.
 class ExtensionCommandsGlobalRegistry
     : public BrowserContextKeyedAPI,
       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();
 
   // Convenience method to get the ExtensionCommandsGlobalRegistry for a
   // profile.
   static ExtensionCommandsGlobalRegistry* Get(content::BrowserContext* context);
 
   // Enables/Disables global shortcut handling in Chrome.
   static void SetShortcutHandlingSuspended(bool suspended);
@@ skipping 17 matching lines @@
       const ui::Accelerator& accelerator,
       const std::string& command_name) OVERRIDE;
 
   // Called by the GlobalShortcutListener object when a shortcut this class has
   // registered for has been pressed.
   virtual void OnKeyPressed(const ui::Accelerator& accelerator) OVERRIDE;
 
   // 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);
 };
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_COMMANDS_GLOBAL_REGISTRY_H_