Polish the keybinding implementation a bit.

chrome/common/extensions/extension.h

Index: chrome/common/extensions/extension.h
===================================================================
--- chrome/common/extensions/extension.h	(revision 127690)
+++ chrome/common/extensions/extension.h	(working copy)
@@ -167,10 +167,15 @@
     const std::string& description() const { return description_; }
 
    private:
+    ui::Accelerator ParseImpl(const std::string& shortcut,
+                              const std::string& platform_key,
+                              int index,
+                              string16* error);
     std::string command_name_;
     ui::Accelerator accelerator_;
     std::string description_;
   };
+  typedef std::map<std::string, ExtensionKeybinding> Commands;

[Aaron Boodman, 2012/03/26 21:44:57]

Naming nit: CommandMap? Just commands seems too ambiguous.

 
   struct TtsVoice {
     // Define out of line constructor/destructor to please Clang.
@@ -551,9 +556,15 @@
   const std::vector<InputComponentInfo>& input_components() const {
     return input_components_;
   }
-  const std::vector<ExtensionKeybinding>& keybindings() const {
-    return commands_;
+  const ExtensionKeybinding* browser_action_command() const {

[Aaron Boodman, 2012/03/26 21:44:57]

I think it would be more traditional to make these const references.

[Finnur, 2012/03/29 14:57:27]

Traditional perhaps, but one of those (ba/pa) is guaranteed to always be zero
and perhaps much of the time the will both be null.

Also, I think it would complicate the call sites, going from:
if (extension->browser_action_command())
... to something like ...
if (extension->browser_action_command().accelerator().key_code() ==
ui::VKEY_UNKNOWN)

On 2012/03/26 21:44:57, Aaron Boodman wrote:

[Aaron Boodman, 2012/03/29 20:57:28]

Oh, duh, you are totally right. Sorry.

+    return browser_action_command_.get();
   }
+  const ExtensionKeybinding* page_action_command() const {
+    return page_action_command_.get();
+  }
+  const Commands& named_commands() const {
+    return named_commands_;
+  }
   bool has_background_page() const {
     return background_url_.is_valid() || !background_scripts_.empty();
   }
@@ -895,7 +906,9 @@
   std::vector<InputComponentInfo> input_components_;
 
   // Optional list of commands (keyboard shortcuts).
-  std::vector<ExtensionKeybinding> commands_;
+  scoped_ptr<ExtensionKeybinding> browser_action_command_;
+  scoped_ptr<ExtensionKeybinding> page_action_command_;
+  Commands named_commands_;
 
   // Optional list of web accessible extension resources.
   base::hash_set<std::string> web_accessible_resources_;