Polish the keybinding implementation a bit.

chrome/common/extensions/extension.h

 // Copyright (c) 2012 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_COMMON_EXTENSIONS_EXTENSION_H_
 #define CHROME_COMMON_EXTENSIONS_EXTENSION_H_
 #pragma once
 
 #include <map>
 #include <set>
@@ skipping 149 matching lines @@
                const std::string& command_name,
                int index,
                string16* error);
 
     // Accessors:
     const std::string& command_name() const { return command_name_; }
     const ui::Accelerator& accelerator() const { return accelerator_; }
     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.
     TtsVoice();
     ~TtsVoice();
 
     std::string voice_name;
     std::string lang;
     std::string gender;
     std::set<std::string> event_types;
@@ skipping 360 matching lines @@
   const FileBrowserHandlerList* file_browser_handlers() const {
     return file_browser_handlers_.get();
   }
   const std::vector<PluginInfo>& plugins() const { return plugins_; }
   const std::vector<NaClModuleInfo>& nacl_modules() const {
     return nacl_modules_;
   }
   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();
   }
   bool allow_background_js_access() const {
     return allow_background_js_access_;
   }
   const std::vector<std::string>& background_scripts() const {
     return background_scripts_;
   }
@@ skipping 322 matching lines @@
   // Optional list of NPAPI plugins and associated properties.
   std::vector<PluginInfo> plugins_;
 
   // Optional list of NaCl modules and associated properties.
   std::vector<NaClModuleInfo> nacl_modules_;
 
   // Optional list of input components and associated properties.
   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_;
 
   // Optional URL to a master page of which a single instance should be always
   // loaded in the background.
   GURL background_url_;
 
   // Optional list of scripts to use to generate a background page. If this is
   // present, background_url_ will be empty and generated by GetBackgroundURL().
@@ skipping 160 matching lines @@
   // only contain the removed permissions.
   const ExtensionPermissionSet* permissions;
 
   UpdatedExtensionPermissionsInfo(
       const Extension* extension,
       const ExtensionPermissionSet* permissions,
       Reason reason);
 };
 
 #endif  // CHROME_COMMON_EXTENSIONS_EXTENSION_H_