Track the active ExtensionKeybindingRegistry and make it available to EventRewriter.

chrome/browser/extensions/extension_keybinding_registry_tracker.h

Index: chrome/browser/extensions/extension_keybinding_registry_tracker.h
diff --git a/chrome/browser/extensions/extension_keybinding_registry_tracker.h b/chrome/browser/extensions/extension_keybinding_registry_tracker.h
new file mode 100644
index 0000000000000000000000000000000000000000..82594a92911e321ab4c3aac2bf1c6fcfe6cceb06
--- /dev/null
+++ b/chrome/browser/extensions/extension_keybinding_registry_tracker.h
@@ -0,0 +1,59 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[Devlin, 2014/09/11 18:41:01]

no (c), and not 2013 anymore :)

[David Tseng, 2014/09/11 19:34:20]

Done.

+// 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_KEYBINDING_REGISTRY_TRACKER_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_KEYBINDING_REGISTRY_TRACKER_H_
+
+#include "chrome/browser/extensions/extension_keybinding_registry.h"
+#include "extensions/browser/browser_context_keyed_api_factory.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+// ExtensionKeybindingRegistryTracker holds the active BrowserWindow's
+// ExtensionKeybindingRegistry.
+class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI {

[Devlin, 2014/09/11 18:41:01]

After seeing the whole of this patch, I think this is actually quite
overarchitected.  ExtensionCommandsGlobalRegistry is itself a
BrowserContextKeyedAPI, so couldn't we just expose a Get/SetActiveRegistry on
that?

[David Tseng, 2014/09/11 18:58:50]

I'd prefer to keep these separated since I think we want to keep the distinction
between global and non-global registry, but can change this if you feel
strongly. Maybe GetActiveNonglobalRegistry? Seems confusing to me.

[Devlin, 2014/09/11 19:48:48]

I don't think that making an accessor to an active instance is making the global
and non-global any less separated.  I'd say put on the accessor. (I'd also
weakly opt against saying Nonglobal in the name, since the class is named
ExtensionKeybindingRegistry vs NonGlobalExtensionKeybindingRegistry; I think
that a method comment is sufficient to differentiate.  But if you feel strongly,
I could be persuaded.)

[David Tseng, 2014/09/11 20:38:41]

Example of the confusion here...you meant ExtensionCommandsGlobalRegistry rather
than ExtensionKeybindingRegistry? The latter isn't a BrowserContextKeyedAPI

So, if we added 
ExtensionCommandsGlobalRegistry::GetActiveRegistry()

ExtensionCommandsGlobalRegistry* registry = ...;
ExtensionKeybindingRegistry* registry2 = registry->GetActiveRegistry();

it actually refers to another registry (not the instance referred to by the
caller).

Is this what you meant?


 I think
If you're not convinced by the above points, then I'll change it :).

[Devlin, 2014/09/11 20:52:45]

I think I named them all correctly in my comments :)  (I did say
ExtensionCommandsGlobalRegistry was the BCKS).  And actually, I think the
implementation is fine:
ExtensionKeybindingRegistry* active_registry =
    ExtensionCommandsGlobalRegistry::Get(browser_context)->GetActiveRegistry();

Even conceptually, this makes sense - you're asking the GlobalRegistry to return
whatever the currently-active one is.  I think that's quite a bit simpler than
trying to remember a whole new object (ExtensionKeybindingRegistryTracker).

If the argument is that you could have two "registry" variables, then there's a
larger problem - what about all our XTabHelpers? :)

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

Thanks Devlin, for weighing in. It was good to get a fresh perspective on this.

+ public:
+  // BrowserContextKeyedAPI implementation.
+  static BrowserContextKeyedAPIFactory<ExtensionKeybindingRegistryTracker>*
+      GetFactoryInstance();
+
+  // Convenience method to get the ExtensionKeybindingRegistryTracker for a
+  // profile.

[Devlin, 2014/09/11 18:41:01]

nit: s/profile/browser context.

[David Tseng, 2014/09/11 19:34:20]

Done.

+  static ExtensionKeybindingRegistryTracker* Get(
+      content::BrowserContext* context);
+
+  // Updates the active ExtensionKeybindingRegistry.
+  void Update(ExtensionKeybindingRegistry* registry);
+
+  // Gets the active ExtensionKeybindingRegistry.
+  ExtensionKeybindingRegistry* GetActiveRegistry();
+
+  explicit ExtensionKeybindingRegistryTracker(content::BrowserContext* context);

[Devlin, 2014/09/11 18:41:01]

nit: Prefer these at the top (right below public:)

[David Tseng, 2014/09/11 19:34:20]

Done.

+  virtual ~ExtensionKeybindingRegistryTracker();
+
+ private:
+  friend class BrowserContextKeyedAPIFactory<
+      ExtensionKeybindingRegistryTracker>;
+
+  // BrowserContextKeyedAPI implementation.
+  static const char* service_name() {
+    return "ExtensionKeybindingRegistryTracker";
+  }
+
+  // Weak pointer to our browser context. Not owned by us.
+  content::BrowserContext* browser_context_;
+
+  // The active ExtensionKeybindingRegistry.
+  ExtensionKeybindingRegistry* registry_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionKeybindingRegistryTracker);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_KEYBINDING_REGISTRY_TRACKER_H_