This adds a webui overlay on the extensions page for showing what Extension keybindings are active.…

chrome/browser/extensions/api/commands/extension_command_service.h

Index: chrome/browser/extensions/api/commands/extension_command_service.h
===================================================================
--- chrome/browser/extensions/api/commands/extension_command_service.h	(revision 137495)
+++ chrome/browser/extensions/api/commands/extension_command_service.h	(working copy)
@@ -20,6 +20,10 @@
 class PrefService;
 class Profile;
 
+namespace base {
+  class DictionaryValue;
+}
+
 namespace ui {
   class Accelerator;
 }
@@ -30,6 +34,13 @@
 class ExtensionCommandService : public ProfileKeyedService,

[Aaron Boodman, 2012/05/19 05:33:26]

I may have already mentioned this, and it's a low priority, but:

extensions::CommandService - then you rename the file to be shorter, and whoever
owns this can call it just command_service.

[Finnur, 2012/05/21 21:28:43]

How about a separate changelist, so as to not mess up the review diffs?

On 2012/05/19 05:33:26, Aaron Boodman wrote:

                                 public content::NotificationObserver {
  public:
+  // An enum specifying whether to fetch all extension commands or only active
+  // ones.
+  enum ExtensionCommandQueryType {

[Aaron Boodman, 2012/05/19 05:33:26]

Seems like QueryType is sufficient.

[Finnur, 2012/05/21 21:28:43]

Done.

+    ALL,
+    ACTIVE_ONLY,
+  };
+
   // Register prefs for keybinding.
   static void RegisterUserPrefs(PrefService* user_prefs);
 
@@ -37,27 +48,34 @@
   explicit ExtensionCommandService(Profile* profile);
   virtual ~ExtensionCommandService();
 
-  // Gets the active keybinding (if any) for the browser action of an extension
-  // given its |extension_id|. The function consults the master list to see if
+  // Gets the keybinding (if any) for the browser action of an extension given
+  // its |extension_id|. The function consults the master list to see if
   // the keybinding is active. Returns NULL if the extension has no browser
-  // action or no active keybinding for it.
-  const extensions::Command* GetActiveBrowserActionCommand(
-      const std::string& extension_id);
+  // action. Returns NULL if the keybinding is not active and |type| requested
+  // is ACTIVE_ONLY.
+  const extensions::Command* GetBrowserActionCommand(
+      const std::string& extension_id, ExtensionCommandQueryType type);
 
-  // Gets the active keybinding (if any) for the page action of an extension
-  // given its |extension_id|. The function consults the master list to see if
-  // the keybinding is active. Returns NULL if the extension has no page action
-  // or no active keybinding for it.
-  const extensions::Command* GetActivePageActionCommand(
-      const std::string& extension_id);
+  // Gets the keybinding (if any) for the browser action of an extension given

[Aaron Boodman, 2012/05/19 05:33:26]

Did you mean to change this? Also, we're killing page actions. It probably makes
sense to skip supporting them with keybindings. Sorry I didn't realize this
earlier.

[Finnur, 2012/05/21 21:28:43]

Did not mean to change it. I'm hesitant to remove the code on the basis that we
might remove support for page actions at some point in the future. It would be
kind of weird to only support browser actions in the interim.

On 2012/05/19 05:33:26, Aaron Boodman wrote:

+  // its |extension_id|. The function consults the master list to see if
+  // the keybinding is active. Returns NULL if the extension has no page
+  // action. Returns NULL if the keybinding is not active and |type| requested
+  // is ACTIVE_ONLY.
+  const extensions::Command* GetPageActionCommand(
+      const std::string& extension_id, ExtensionCommandQueryType type);
 
   // Gets the active keybinding (if any) for the named commands of an extension
   // given its |extension_id|. The function consults the master list to see if
   // the keybinding is active. Returns an empty map if the extension has no
-  // named commands or no active keybinding for the commands.
-  extensions::CommandMap GetActiveNamedCommands(
-      const std::string& extension_id);
+  // named commands or no active named commands when |type| requested is
+  // ACTIVE_ONLY.
+  extensions::CommandMap GetNamedCommands(
+      const std::string& extension_id, ExtensionCommandQueryType type);
 
+  // Fetches all known commands, active and inactive and returns them through
+  // |commands|.
+  void GetAllCommands(base::DictionaryValue* commands);

[Aaron Boodman, 2012/05/19 05:33:26]

Weird to have an interface that deals in Values here. Seems like the UI layer
should just call GetNamedCommands() and GetBrowserActionCommand() directly, and
handle the construction of the DictionaryValue itself.

[Finnur, 2012/05/21 21:28:43]

Done.

+
   // Checks to see if a keybinding |accelerator| for a given |command_name| in
   // an extension with id |extension_id| is registered as active (by consulting
   // the master list in |user_prefs|).
@@ -93,6 +111,13 @@
   // Removes all keybindings for a given extension by its |extension_id|.
   void RemoveKeybindingPrefs(std::string extension_id);
 
+  // Creates a DictionaryValue containing the details of the extension and
+  // its commands. |extension| and |command| are pointers to the objects this
+  // function should describe.
+  base::DictionaryValue* CreateExtensionDetailValue(

[Aaron Boodman, 2012/05/19 05:33:26]

Seems like this should be Command::ToValue().

[Finnur, 2012/05/21 21:28:43]

Done.

+      const Extension* extension,
+      const extensions::Command* command);
+
   // The content notification registrar for listening to extension events.
   content::NotificationRegistrar registrar_;