Polish the keybinding implementation a bit.

- Make it require manifest version 2.
- Change 'key' to 'suggested_key', allow it to be a string or a dictionary with platforms and/or 'default' listed.
- Converted browser_action in manifest to _execute_browser_action, since that keyword is reserved (same for page action).
- Remove normalization of suggested_key (stop supporting Ctrl - foo, now only Ctrl+foo and make it case sensitive).
- Added methods to fetch just browser actions/just page actions (or all others), which is what you'd commonly want.

BUG=27702
TEST=Covered by automated tests.
Review URL: https://chromiumcodereview.appspot.com/9812008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@130114 0039d316-1c4b-4281-b951-d872f2087c98

16 files changed, 411 insertions, 172 deletions

diff --git a/chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc b/chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc
index 59ec645..2c3f8d8 100644
--- a/chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc
+++ b/chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc
@@ -374,21 +374,15 @@ class BrowserActionButton : public content::NotificationObserver,
 
   // Connect the accelerator for the browser action popup.
   void ConnectBrowserActionPopupAccelerator() {
-    // Iterate through all the keybindings and see if one is assigned to the
-    // browserAction.
-    const std::vector<Extension::ExtensionKeybinding>& commands =
-        extension_->keybindings();
-    for (size_t i = 0; i < commands.size(); ++i) {
-      if (commands[i].command_name() !=
-              extension_manifest_values::kBrowserActionKeybindingEvent)
-        continue;
-
+    const Extension::ExtensionKeybinding* command =
+        extension_->browser_action_command();
+    if (command) {
       // Found the browser action shortcut command, register it.
       keybinding_.reset(new ui::AcceleratorGtk(
-          commands[i].accelerator().key_code(),
-          commands[i].accelerator().IsShiftDown(),
-          commands[i].accelerator().IsCtrlDown(),
-          commands[i].accelerator().IsAltDown()));
+          command->accelerator().key_code(),
+          command->accelerator().IsShiftDown(),
+          command->accelerator().IsCtrlDown(),
+          command->accelerator().IsAltDown()));
 
       gfx::NativeWindow window =
           toolbar_->browser()->window()->GetNativeHandle();
@@ -407,7 +401,6 @@ class BrowserActionButton : public content::NotificationObserver,
       registrar_.Add(this,
                      chrome::NOTIFICATION_WINDOW_CLOSED,
                      content::Source<GtkWindow>(window));
-      break;
     }
   }
 
diff --git a/chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc b/chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
index 4024a7c..4ec31e7 100644
--- a/chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
+++ b/chrome/browser/ui/gtk/extensions/extension_keybinding_registry_gtk.cc
@@ -43,17 +43,17 @@ void ExtensionKeybindingRegistryGtk::AddExtensionKeybinding(
     const Extension* extension) {
   // Add all the keybindings (except pageAction and browserAction, which are
   // handled elsewhere).
-  const std::vector<Extension::ExtensionKeybinding> commands =
-      extension->keybindings();
-  for (size_t i = 0; i < commands.size(); ++i) {
-    ui::AcceleratorGtk accelerator(commands[i].accelerator().key_code(),
-                                   commands[i].accelerator().IsShiftDown(),
-                                   commands[i].accelerator().IsCtrlDown(),
-                                   commands[i].accelerator().IsAltDown());
+  const Extension::CommandMap& commands = extension->named_commands();
+  Extension::CommandMap::const_iterator iter = commands.begin();
+  for (; iter != commands.end(); ++iter) {
+    ui::AcceleratorGtk accelerator(iter->second.accelerator().key_code(),
+                                   iter->second.accelerator().IsShiftDown(),
+                                   iter->second.accelerator().IsCtrlDown(),
+                                   iter->second.accelerator().IsAltDown());
     event_targets_[accelerator] =
-        std::make_pair(extension->id(), commands[i].command_name());
+        std::make_pair(extension->id(), iter->second.command_name());
 
-    if (ShouldIgnoreCommand(commands[i].command_name()))
+    if (ShouldIgnoreCommand(iter->second.command_name()))
       continue;
 
     if (!accel_group_) {
diff --git a/chrome/browser/ui/gtk/location_bar_view_gtk.cc b/chrome/browser/ui/gtk/location_bar_view_gtk.cc
index 24451cb..79ce1c2 100644
--- a/chrome/browser/ui/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/ui/gtk/location_bar_view_gtk.cc
@@ -1709,21 +1709,15 @@ void LocationBarViewGtk::PageActionViewGtk::ConnectPageActionAccelerator() {
       extensions->GetByID(page_action_->extension_id());
   window_ = owner_->browser()->window()->GetNativeHandle();
 
-  // Iterate through all the keybindings and see if one is assigned to the
-  // pageAction.
-  const std::vector<Extension::ExtensionKeybinding>& commands =
-      extension->keybindings();
-  for (size_t i = 0; i < commands.size(); ++i) {
-    if (commands[i].command_name() !=
-        extension_manifest_values::kPageActionKeybindingEvent)
-      continue;
-
+  const Extension::ExtensionKeybinding* command =
+      extension->page_action_command();
+  if (command) {
     // Found the browser action shortcut command, register it.
     keybinding_.reset(new ui::AcceleratorGtk(
-        commands[i].accelerator().key_code(),
-        commands[i].accelerator().IsShiftDown(),
-        commands[i].accelerator().IsCtrlDown(),
-        commands[i].accelerator().IsAltDown()));
+        command->accelerator().key_code(),
+        command->accelerator().IsShiftDown(),
+        command->accelerator().IsCtrlDown(),
+        command->accelerator().IsAltDown()));
 
     accel_group_ = gtk_accel_group_new();
     gtk_window_add_accel_group(window_, accel_group_);
@@ -1740,7 +1734,6 @@ void LocationBarViewGtk::PageActionViewGtk::ConnectPageActionAccelerator() {
     registrar_.Add(this,
                    chrome::NOTIFICATION_WINDOW_CLOSED,
                    content::Source<GtkWindow>(window_));
-    break;
   }
 }
 
diff --git a/chrome/browser/ui/views/browser_actions_container.cc b/chrome/browser/ui/views/browser_actions_container.cc
index a219074..a18c24d 100644
--- a/chrome/browser/ui/views/browser_actions_container.cc
+++ b/chrome/browser/ui/views/browser_actions_container.cc
@@ -117,18 +117,13 @@ void BrowserActionButton::ViewHierarchyChanged(
       UpdateState();
     }
 
-    // Iterate through all the keybindings and see if one is assigned to the
-    // browserAction.
-    const std::vector<Extension::ExtensionKeybinding>& commands =
-        extension_->keybindings();
-    for (size_t i = 0; i < commands.size(); ++i) {
-      if (commands[i].command_name() ==
-          extension_manifest_values::kBrowserActionKeybindingEvent) {
-        keybinding_.reset(new ui::Accelerator(commands[i].accelerator()));
-        panel_->GetFocusManager()->RegisterAccelerator(
-            *keybinding_.get(), ui::AcceleratorManager::kHighPriority, this);
-        break;
-      }
+    const Extension::ExtensionKeybinding* browser_action_command =
+        extension_->browser_action_command();
+    if (browser_action_command) {
+      keybinding_.reset(new ui::Accelerator(
+          browser_action_command->accelerator()));
+      panel_->GetFocusManager()->RegisterAccelerator(
+          *keybinding_.get(), ui::AcceleratorManager::kHighPriority, this);
     }
   }
 
diff --git a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
index 50301b1..d18ee4a 100644
--- a/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
+++ b/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
@@ -28,16 +28,14 @@ void ExtensionKeybindingRegistryViews::AddExtensionKeybinding(
     const Extension* extension) {
   // Add all the keybindings (except pageAction and browserAction, which are
   // handled elsewhere).
-  const std::vector<Extension::ExtensionKeybinding> commands =
-      extension->keybindings();
-  for (size_t i = 0; i < commands.size(); ++i) {
-    if (ShouldIgnoreCommand(commands[i].command_name()))
-      continue;
-
-    event_targets_[commands[i].accelerator()] =
-        std::make_pair(extension->id(), commands[i].command_name());
+  const Extension::CommandMap& commands = extension->named_commands();
+  Extension::CommandMap::const_iterator iter = commands.begin();
+  for (; iter != commands.end(); ++iter) {
+    event_targets_[iter->second.accelerator()] =
+        std::make_pair(extension->id(), iter->second.command_name());
     focus_manager_->RegisterAccelerator(
-        commands[i].accelerator(), ui::AcceleratorManager::kHighPriority, this);
+        iter->second.accelerator(),
+        ui::AcceleratorManager::kHighPriority, this);
   }
 }
 
diff --git a/chrome/browser/ui/views/location_bar/page_action_image_view.cc b/chrome/browser/ui/views/location_bar/page_action_image_view.cc
index eefb7af..163ddd8 100644
--- a/chrome/browser/ui/views/location_bar/page_action_image_view.cc
+++ b/chrome/browser/ui/views/location_bar/page_action_image_view.cc
@@ -61,18 +61,12 @@ PageActionImageView::PageActionImageView(LocationBarView* owner,
 
   set_accessibility_focusable(true);
 
-  // Iterate through all the keybindings and see if one is assigned to the
-  // pageAction.
-  const std::vector<Extension::ExtensionKeybinding>& commands =
-      extension->keybindings();
-  for (size_t i = 0; i < commands.size(); ++i) {
-    if (commands[i].command_name() ==
-        extension_manifest_values::kPageActionKeybindingEvent) {
-      keybinding_.reset(new ui::Accelerator(commands[i].accelerator()));
-      owner_->GetFocusManager()->RegisterAccelerator(
-          *keybinding_.get(), ui::AcceleratorManager::kHighPriority, this);
-      break;
-    }
+  const Extension::ExtensionKeybinding* page_action_command =
+      extension->page_action_command();
+  if (page_action_command) {
+    keybinding_.reset(new ui::Accelerator(page_action_command->accelerator()));
+    owner_->GetFocusManager()->RegisterAccelerator(
+        *keybinding_.get(), ui::AcceleratorManager::kHighPriority, this);
   }
 }
 
diff --git a/chrome/common/extensions/api/_manifest_features.json b/chrome/common/extensions/api/_manifest_features.json
index 9823c27..7f139bb 100644
--- a/chrome/common/extensions/api/_manifest_features.json
+++ b/chrome/common/extensions/api/_manifest_features.json
@@ -24,7 +24,8 @@
     "extension_types": ["extension", "packaged_app"]
   },
   "commands": {
-    "extension_types": "all"
+    "extension_types": "all",
+    "min_manifest_version": 2
   },
   "content_security_policy": {
     "extension_types": ["extension", "packaged_app", "platform_app"]
diff --git a/chrome/common/extensions/docs/experimental.keybinding.html b/chrome/common/extensions/docs/experimental.keybinding.html
index 4cb6a1d..5c12c6d 100644
--- a/chrome/common/extensions/docs/experimental.keybinding.html
+++ b/chrome/common/extensions/docs/experimental.keybinding.html
@@ -230,7 +230,8 @@ or sending a command to the extension.
 <h2 id="manifest">Manifest</h2>
 <p>
 In addition to the "experimental" permission you must declare the "keybinding"
-permission in your extension's manifest to use this API.
+permission in your extension's manifest to use this API and set manifest_version
+to (at least) 2.
 </p>
 <pre>{
   "name": "My extension",
@@ -252,29 +253,42 @@ AltGr key.</p>
   ...
 <b>  "commands": {
     "toggle-feature-foo": {
-      "key": "Ctrl+Shift+Y",
+      "suggested_key": {
+        "default": "Ctrl+Shift+Y",
+        "mac": "Command+Shift+Y"
+      },
       "description": "Toggle feature foo"
     },
-    "browserAction": {
-      "key": "Ctrl+Shift+B"
+    "_execute_browser_action": {
+      "suggested_key": {
+        "windows": "Ctrl+Shift+Y",
+        "mac": "Command+Shift+Y",
+        "chromeos": "Ctrl+Shift+U",
+        "linux": "Ctrl+Shift+J"
+      }
     },
-    "pageAction": {
-      "key": "Alt+P"
+    "_execute_page_action": {
+      "suggested_key": {
+        "default": "Ctrl+E"
+        "windows": "Alt+P",
+        "mac": "Option+P",
+      }
     }
   }</b>,
   ...
 }</pre>
 <p>In your background page, you can bind a handler to each of the commands
-defined in the manifest (except for 'browserAction' and 'pageAction') via
-onCommand.addListener. For example:</p>
+defined in the manifest (except for '_execute_browser_action' and
+'_execute_page_action') via onCommand.addListener. For example:</p>
 <pre>chrome.experimental.keybinding.onCommand.addListener(function(command) {
   console.log('Command:', command);
 });
 </pre>
-<p>The 'browserAction' and 'pageAction' commands are reserved for the action of
-opening your extension's popups. They won't normally generate events that you
-can handle. If you need to take action based on your popup opening, consider
-listening for an 'onDomReady' event inside your popup's code.
+<p>The '_execute_browser_action' and '_execute_page_action' commands are
+reserved for the action of opening your extension's popups. They won't normally
+generate events that you can handle. If you need to take action based on your
+popup opening, consider listening for an 'onDomReady' event inside your popup's
+code.
 </p>
 <!-- END AUTHORED CONTENT -->
 </div>
diff --git a/chrome/common/extensions/docs/static/experimental.keybinding.html b/chrome/common/extensions/docs/static/experimental.keybinding.html
index 1148018..51f9cd8 100644
--- a/chrome/common/extensions/docs/static/experimental.keybinding.html
+++ b/chrome/common/extensions/docs/static/experimental.keybinding.html
@@ -8,7 +8,8 @@ or sending a command to the extension.
 <h2 id="manifest">Manifest</h2>
 <p>
 In addition to the "experimental" permission you must declare the "keybinding"
-permission in your extension's manifest to use this API.
+permission in your extension's manifest to use this API and set manifest_version
+to (at least) 2.
 </p>
 
 <pre>{
@@ -33,23 +34,34 @@ AltGr key.</p>
   ...
 <b>  "commands": {
     "toggle-feature-foo": {
-      "key": "Ctrl+Shift+Y",
+      "suggested_key": {
+        "default": "Ctrl+Shift+Y",
+        "mac": "Command+Shift+Y"
+      },
       "description": "Toggle feature foo"
     },
-    "browserAction": {
-      "key": "Ctrl+Shift+B"
+    "_execute_browser_action": {
+      "suggested_key": {
+        "windows": "Ctrl+Shift+Y",
+        "mac": "Command+Shift+Y",
+        "chromeos": "Ctrl+Shift+U",
+        "linux": "Ctrl+Shift+J"
+      }
     },
-    "pageAction": {
-      "key": "Alt+P"
+    "_execute_page_action": {
+      "suggested_key": {
+        "default": "Ctrl+E"
+        "windows": "Alt+P",
+        "mac": "Option+P",
+      }
     }
-
   }</b>,
   ...
 }</pre>
 
 <p>In your background page, you can bind a handler to each of the commands
-defined in the manifest (except for 'browserAction' and 'pageAction') via
-onCommand.addListener. For example:</p>
+defined in the manifest (except for '_execute_browser_action' and
+'_execute_page_action') via onCommand.addListener. For example:</p>
 
 <pre>
 chrome.experimental.keybinding.onCommand.addListener(function(command) {
@@ -57,9 +69,10 @@ chrome.experimental.keybinding.onCommand.addListener(function(command) {
 });
 </pre>
 
-<p>The 'browserAction' and 'pageAction' commands are reserved for the action of
-opening your extension's popups. They won't normally generate events that you
-can handle. If you need to take action based on your popup opening, consider
-listening for an 'onDomReady' event inside your popup's code.
+<p>The '_execute_browser_action' and '_execute_page_action' commands are
+reserved for the action of opening your extension's popups. They won't normally
+generate events that you can handle. If you need to take action based on your
+popup opening, consider listening for an 'onDomReady' event inside your popup's
+code.
 </p>
 <!-- END AUTHORED CONTENT -->
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index e239abe..ecf3d16 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -245,37 +245,32 @@ Extension::OAuth2Info::~OAuth2Info() {}
 Extension::ExtensionKeybinding::ExtensionKeybinding() {}
 Extension::ExtensionKeybinding::~ExtensionKeybinding() {}
 
-bool Extension::ExtensionKeybinding::Parse(DictionaryValue* command,
-                                           const std::string& command_name,
-                                           int index,
-                                           string16* error) {
-  DCHECK(!command_name.empty());
-  std::string key_binding;
-  if (!command->GetString(keys::kKey, &key_binding) ||
-      key_binding.empty()) {
+ui::Accelerator Extension::ExtensionKeybinding::ParseImpl(
+    const std::string& shortcut,
+    const std::string& platform_key,
+    int index,
+    string16* error) {
+  if (platform_key != values::kKeybindingPlatformWin &&
+      platform_key != values::kKeybindingPlatformMac &&
+      platform_key != values::kKeybindingPlatformChromeOs &&
+      platform_key != values::kKeybindingPlatformLinux &&
+      platform_key != values::kKeybindingPlatformDefault) {
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
-        errors::kInvalidKeyBinding,
+        errors::kInvalidKeyBindingUnknownPlatform,
         base::IntToString(index),
-        "Missing");
-    return false;
+        platform_key);
+    return ui::Accelerator();
   }
 
-  std::string original_keybinding = key_binding;
-  // Normalize '-' to '+'.
-  ReplaceSubstringsAfterOffset(&key_binding, 0, "-", "+");
-  // Remove all spaces.
-  ReplaceSubstringsAfterOffset(&key_binding, 0, " ", "");
-  // And finally, lower-case it.
-  key_binding = StringToLowerASCII(key_binding);
-
   std::vector<std::string> tokens;
-  base::SplitString(key_binding, '+', &tokens);
+  base::SplitString(shortcut, '+', &tokens);
   if (tokens.size() < 2 || tokens.size() > 3) {
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
-        original_keybinding);
-    return false;
+        platform_key,
+        shortcut);
+    return ui::Accelerator();
   }
 
   // Now, parse it into an accelerator.
@@ -284,27 +279,32 @@ bool Extension::ExtensionKeybinding::Parse(DictionaryValue* command,
   bool shift = false;
   ui::KeyboardCode key = ui::VKEY_UNKNOWN;
   for (size_t i = 0; i < tokens.size(); i++) {
-    if (tokens[i] == "ctrl") {
+    if (tokens[i] == "Ctrl") {
       ctrl = true;
-    } else if (tokens[i] == "alt") {
+    } else if (tokens[i] == "Alt") {
       alt = true;
-    } else if (tokens[i] == "shift") {
+    } else if (tokens[i] == "Shift") {
       shift = true;
+    } else if (tokens[i] == "Command" && platform_key == "mac") {
+      // TODO(finnur): Implement for Mac.
+    } else if (tokens[i] == "Option" && platform_key == "mac") {
+      // TODO(finnur): Implement for Mac.
     } else if (tokens[i].size() == 1 &&
-               tokens[i][0] >= 'a' && tokens[i][0] <= 'z') {
+               tokens[i][0] >= 'A' && tokens[i][0] <= 'Z') {
       if (key != ui::VKEY_UNKNOWN) {
         // Multiple key assignments.
         key = ui::VKEY_UNKNOWN;
         break;
       }
 
-      key = static_cast<ui::KeyboardCode>(ui::VKEY_A + (tokens[i][0] - 'a'));
+      key = static_cast<ui::KeyboardCode>(ui::VKEY_A + (tokens[i][0] - 'A'));
     } else {
       *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
           errors::kInvalidKeyBinding,
           base::IntToString(index),
-          original_keybinding);
-      return false;
+          platform_key,
+          shortcut);
+      return ui::Accelerator();
     }
   }
 
@@ -315,26 +315,124 @@ bool Extension::ExtensionKeybinding::Parse(DictionaryValue* command,
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
-        original_keybinding);
-    return false;
+        platform_key,
+        shortcut);
+    return ui::Accelerator();
   }
 
-  accelerator_ = ui::Accelerator(key, shift, ctrl, alt);
+  return ui::Accelerator(key, shift, ctrl, alt);
+}
+
+bool Extension::ExtensionKeybinding::Parse(DictionaryValue* command,
+                                           const std::string& command_name,
+                                           int index,
+                                           string16* error) {
+  DCHECK(!command_name.empty());
 
-  if (command_name !=
-          extension_manifest_values::kPageActionKeybindingEvent &&
-      command_name !=
-          extension_manifest_values::kBrowserActionKeybindingEvent) {
-    if (!command->GetString(keys::kDescription, &description_) ||
-        description_.empty()) {
+  // We'll build up a map of platform-to-shortcut suggestions.
+  std::map<const std::string, std::string> suggestions;
+
+  // First try to parse the |suggested_key| as a dictionary.
+  DictionaryValue* suggested_key_dict;
+  if (command->GetDictionary(keys::kSuggestedKey, &suggested_key_dict)) {
+    DictionaryValue::key_iterator iter = suggested_key_dict->begin_keys();
+    for ( ; iter != suggested_key_dict->end_keys(); ++iter) {
+      // For each item in the dictionary, extract the platforms specified.
+      std::string suggested_key_string;
+      if (suggested_key_dict->GetString(*iter, &suggested_key_string) &&
+          !suggested_key_string.empty()) {
+        // Found a platform, add it to the suggestions list.
+        suggestions[*iter] = suggested_key_string;
+      } else {
+        *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
+            errors::kInvalidKeyBinding,
+            base::IntToString(index),
+            keys::kSuggestedKey,
+            "Missing");
+        return false;
+      }
+    }
+  } else {
+    // No dictionary was found, fall back to using just a string, so developers
+    // don't have to specify a dictionary if they just want to use one default
+    // for all platforms.
+    std::string suggested_key_string;
+    if (command->GetString(keys::kSuggestedKey, &suggested_key_string) &&
+        !suggested_key_string.empty()) {
+      // If only a signle string is provided, it must be default for all.
+      suggestions["default"] = suggested_key_string;
+    } else {
       *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
-          errors::kInvalidKeyBindingDescription,
-          base::IntToString(index));
-      return false;
+          errors::kInvalidKeyBinding,
+          base::IntToString(index),
+          keys::kSuggestedKey,
+          "Missing");
+       return false;
     }
   }
 
-  command_name_ = command_name;
+  std::string platform =
+#if defined(OS_WIN)
+      values::kKeybindingPlatformWin;
+#elif defined(OS_MACOSX)
+      values::kKeybindingPlatformMac;
+#elif defined(OS_CHROMEOS)
+      values::kKeybindingPlatformChromeOs;
+#elif defined(OS_LINUX)
+      values::kKeybindingPlatformLinux;
+#else
+      "";
+#endif
+
+  std::string key = platform;
+  if (suggestions.find(key) == suggestions.end())
+    key = values::kKeybindingPlatformDefault;
+  if (suggestions.find(key) == suggestions.end()) {
+    *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
+        errors::kInvalidKeyBindingMissingPlatform,
+        base::IntToString(index),
+        keys::kSuggestedKey,
+        platform);
+    return false;  // No platform specified and no fallback. Bail.
+  }
+
+  // For developer convenience, we parse all the suggestions (and complain about
+  // errors for platforms other than the current one) but use only what we need.
+  std::map<const std::string, std::string>::const_iterator iter =
+      suggestions.begin();
+  for ( ; iter != suggestions.end(); ++iter) {
+    // Note that we pass iter->first to pretend we are on a platform we're not
+    // on.
+    ui::Accelerator accelerator =
+        ParseImpl(iter->second, iter->first, index, error);
+    if (accelerator.key_code() == ui::VKEY_UNKNOWN) {
+      *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
+          errors::kInvalidKeyBinding,
+          base::IntToString(index),
+          iter->first,
+          iter->second);
+      return false;
+    }
+
+    if (iter->first == key) {
+      // This platform is our platform, so grab this key.
+      accelerator_ = accelerator;
+      command_name_ = command_name;
+
+      if (command_name !=
+              extension_manifest_values::kPageActionKeybindingEvent &&
+          command_name !=
+              extension_manifest_values::kBrowserActionKeybindingEvent) {
+        if (!command->GetString(keys::kDescription, &description_) ||
+            description_.empty()) {
+          *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
+              errors::kInvalidKeyBindingDescription,
+              base::IntToString(index));
+          return false;
+        }
+      }
+    }
+  }
   return true;
 }
 
@@ -1542,11 +1640,20 @@ bool Extension::LoadCommands(string16* error) {
         return false;
       }
 
-      ExtensionKeybinding binding;
-      if (!binding.Parse(command, *iter, command_index, error))
+      scoped_ptr<Extension::ExtensionKeybinding> binding(
+          new ExtensionKeybinding());
+      if (!binding->Parse(command, *iter, command_index, error))
         return false;  // |error| already set.
 
-      commands_.push_back(binding);
+      std::string command_name = binding->command_name();
+      if (command_name == values::kPageActionKeybindingEvent) {
+        page_action_command_.reset(binding.release());
+      } else if (command_name == values::kBrowserActionKeybindingEvent) {
+        browser_action_command_.reset(binding.release());
+      } else {
+        if (command_name[0] != '_')  // All commands w/underscore are reserved.
+          named_commands_[command_name] = *binding.release();
+      }
     }
   }
   return true;
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index 0260bd7..28b005e2 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -167,11 +167,17 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
     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> CommandMap;
+
   struct TtsVoice {
     // Define out of line constructor/destructor to please Clang.
     TtsVoice();
@@ -564,8 +570,14 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   const std::vector<InputComponentInfo>& input_components() const {
     return input_components_;
   }
-  const std::vector<ExtensionKeybinding>& keybindings() const {
-    return commands_;
+  const ExtensionKeybinding* browser_action_command() const {
+    return browser_action_command_.get();
+  }
+  const ExtensionKeybinding* page_action_command() const {
+    return page_action_command_.get();
+  }
+  const CommandMap& named_commands() const {
+    return named_commands_;
   }
   bool has_background_page() const {
     return background_url_.is_valid() || !background_scripts_.empty();
@@ -920,7 +932,9 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   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_;
+  CommandMap named_commands_;
 
   // Optional list of web accessible extension resources.
   base::hash_set<std::string> web_accessible_resources_;
diff --git a/chrome/common/extensions/extension_manifest_constants.cc b/chrome/common/extensions/extension_manifest_constants.cc
index b5e823f..8a4b24b 100644
--- a/chrome/common/extensions/extension_manifest_constants.cc
+++ b/chrome/common/extensions/extension_manifest_constants.cc
@@ -95,6 +95,7 @@ const char kRunAt[] = "run_at";
 const char kShiftKey[] = "shiftKey";
 const char kShortcutKey[] = "shortcutKey";
 const char kSignature[] = "signature";
+const char kSuggestedKey[] = "suggested_key";
 const char kTheme[] = "theme";
 const char kThemeColors[] = "colors";
 const char kThemeDisplayProperties[] = "properties";
@@ -122,16 +123,21 @@ const char kWebURLs[] = "app.urls";
 }  // namespace extension_manifest_keys
 
 namespace extension_manifest_values {
-const char kBrowserActionKeybindingEvent[] = "browserAction";
+const char kBrowserActionKeybindingEvent[] = "_execute_browser_action";
 const char kIncognitoSplit[] = "split";
 const char kIncognitoSpanning[] = "spanning";
 const char kIntentDispositionWindow[] = "window";
 const char kIntentDispositionInline[] = "inline";
 const char kIsolatedStorage[] = "storage";
+const char kKeybindingPlatformChromeOs[] = "chromeos";
+const char kKeybindingPlatformDefault[] = "default";
+const char kKeybindingPlatformLinux[] = "linux";
+const char kKeybindingPlatformMac[] = "mac";
+const char kKeybindingPlatformWin[] = "windows";
 const char kRunAtDocumentStart[] = "document_start";
 const char kRunAtDocumentEnd[] = "document_end";
 const char kRunAtDocumentIdle[] = "document_idle";
-const char kPageActionKeybindingEvent[] = "pageAction";
+const char kPageActionKeybindingEvent[] = "_execute_page_action";
 const char kPageActionTypeTab[] = "tab";
 const char kPageActionTypePermanent[] = "permanent";
 const char kLaunchContainerPanel[] = "panel";
@@ -292,11 +298,17 @@ const char kInvalidJsList[] =
 const char kInvalidKey[] =
     "Value 'key' is missing or invalid.";
 const char kInvalidKeyBinding[] =
-    "Invalid value for 'commands[*].key':";
+     "Invalid value for 'commands[*].*': *.";
 const char kInvalidKeyBindingDescription[] =
     "Invalid value for 'commands[*].description'.";
 const char kInvalidKeyBindingDictionary[] =
     "Contents of 'commands[*]' invalid.";
+const char kInvalidKeyBindingMissingPlatform[] =
+    "Could not find key specification for 'command[*].*': Either specify a key "
+    "for '*', or specify a default key.";
+const char kInvalidKeyBindingUnknownPlatform[] =
+    "Unknown platform for 'command[*]': *. Valid values are: 'windows', 'mac'"
+    " 'chromeos', 'linux' and 'default'.";
 const char kInvalidLaunchContainer[] =
     "Invalid value for 'app.launch.container'.";
 const char kInvalidLaunchContainerForNonPlatform[] =
diff --git a/chrome/common/extensions/extension_manifest_constants.h b/chrome/common/extensions/extension_manifest_constants.h
index 779b2aa..e324600 100644
--- a/chrome/common/extensions/extension_manifest_constants.h
+++ b/chrome/common/extensions/extension_manifest_constants.h
@@ -103,6 +103,7 @@ namespace extension_manifest_keys {
   extern const char kShiftKey[];
   extern const char kShortcutKey[];
   extern const char kSignature[];
+  extern const char kSuggestedKey[];
   extern const char kTheme[];
   extern const char kThemeColors[];
   extern const char kThemeDisplayProperties[];
@@ -137,6 +138,11 @@ namespace extension_manifest_values {
   extern const char kIntentDispositionWindow[];
   extern const char kIntentDispositionInline[];
   extern const char kIsolatedStorage[];
+  extern const char kKeybindingPlatformChromeOs[];
+  extern const char kKeybindingPlatformDefault[];
+  extern const char kKeybindingPlatformLinux[];
+  extern const char kKeybindingPlatformMac[];
+  extern const char kKeybindingPlatformWin[];
   extern const char kLaunchContainerPanel[];
   extern const char kLaunchContainerShell[];
   extern const char kLaunchContainerTab[];
@@ -226,6 +232,8 @@ namespace extension_manifest_errors {
   extern const char kInvalidKeyBinding[];
   extern const char kInvalidKeyBindingDescription[];
   extern const char kInvalidKeyBindingDictionary[];
+  extern const char kInvalidKeyBindingMissingPlatform[];
+  extern const char kInvalidKeyBindingUnknownPlatform[];
   extern const char kInvalidLaunchContainer[];
   extern const char kInvalidLaunchContainerForNonPlatform[];
   extern const char kInvalidLaunchContainerForPlatform[];
diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
index 5e7f5be..1ee2bc7 100644
--- a/chrome/common/extensions/extension_unittest.cc
+++ b/chrome/common/extensions/extension_unittest.cc
@@ -945,14 +945,16 @@ TEST(ExtensionTest, ExtensionKeybindingParsing) {
     // test |command_name| being blank as it is used as a key in the manifest,
     // so it can't be blank (and we DCHECK when it is).
     { false, None, "command", "",       "" },
-    { false, None, "command", "ctrl+f", "" },
+    { false, None, "command", "Ctrl+f", "" },
     { false, None, "command", "",       "description" },
     // Ctrl+Alt is not permitted, see MSDN link in comments in Parse function.
     { false, None, "command", "Ctrl+Alt+F", "description" },
-    // Unsupported shortcuts/too many.
-    { false, None, "command", "F10",      "description" },
-    { false, None, "command", "Ctrl+1",   "description" },
-    { false, None, "command", "Ctrl+F+G", "description" },
+    // Unsupported shortcuts/too many, or missing modifier.
+    { false, None, "command", "A",                "description" },
+    { false, None, "command", "F10",              "description" },
+    { false, None, "command", "Ctrl+1",           "description" },
+    { false, None, "command", "Ctrl+F+G",         "description" },
+    { false, None, "command", "Ctrl+Alt+Shift+G", "description" },
     // Basic tests.
     { true, CtrlF,      "command", "Ctrl+F",       "description" },
     { true, ShiftF,     "command", "Shift+F",      "description" },
@@ -967,34 +969,49 @@ TEST(ExtensionTest, ExtensionKeybindingParsing) {
     { true, CtrlShiftF, "command", "F+Shift+Ctrl", "description" },
     { true, AltShiftF,  "command", "F+Alt+Shift",  "description" },
     { true, AltShiftF,  "command", "F+Shift+Alt",  "description" },
-    // Case insensitivity is OK.
-    { true, CtrlF, "command", "Ctrl+F", "description" },
-    { true, CtrlF, "command", "cTrL+f", "description" },
-    // Extra spaces are fine.
-    { true, CtrlShiftF, "command", "  Ctrl + Shift   +F", "description" },
-    // Minus is equivalent to plus.
-    { true, CtrlShiftF, "command", "Ctrl+Shift-F", "description" },
+    // Case insensitivity is not OK.
+    { false, CtrlF, "command", "Ctrl+f", "description" },
+    { false, CtrlF, "command", "cTrL+F", "description" },
     // Skipping description is OK for browser- and pageActions.
-    { true, CtrlF, "browserAction", "Ctrl+F", "" },
-    { true, CtrlF, "pageAction",    "Ctrl+F", "" },
+    { true, CtrlF, "_execute_browser_action", "Ctrl+F", "" },
+    { true, CtrlF, "_execute_page_action",   "Ctrl+F", "" },
   };
 
   // TODO(finnur): test Command/Options on Mac when implemented.
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kTests); ++i) {
+    // First parse the command as a simple string.
     scoped_ptr<DictionaryValue> command(new DictionaryValue);
-    command->SetString("key", kTests[i].key);
+    command->SetString("suggested_key", kTests[i].key);
     command->SetString("description", kTests[i].description);
 
+    SCOPED_TRACE(std::string("Command name: |") + kTests[i].command_name +
+                 "| key: |" + kTests[i].key +
+                 "| description: |" + kTests[i].description +
+                 "| index: " + base::IntToString(i));
+
     Extension::ExtensionKeybinding keybinding;
     string16 error;
     bool result =
         keybinding.Parse(command.get(), kTests[i].command_name, i, &error);
 
-    SCOPED_TRACE(std::string("Command name: |") + kTests[i].command_name +
-                 "| key: |" + kTests[i].key +
-                 "| description: |" + kTests[i].description +
-                 "| index: " + base::IntToString(i));
+    EXPECT_EQ(kTests[i].expected_result, result);
+    if (result) {
+      EXPECT_STREQ(kTests[i].description, keybinding.description().c_str());
+      EXPECT_STREQ(kTests[i].command_name, keybinding.command_name().c_str());
+      EXPECT_EQ(kTests[i].accelerator, keybinding.accelerator());
+    }
+
+    // Now parse the command as a dictionary of multiple values.
+    command.reset(new DictionaryValue);
+    DictionaryValue* key_dict = new DictionaryValue();
+    key_dict->SetString("default", kTests[i].key);
+    key_dict->SetString("windows", kTests[i].key);
+    key_dict->SetString("mac", kTests[i].key);
+    command->Set("suggested_key", key_dict);
+    command->SetString("description", kTests[i].description);
+
+    result = keybinding.Parse(command.get(), kTests[i].command_name, i, &error);
 
     EXPECT_EQ(kTests[i].expected_result, result);
     if (result) {
@@ -1005,6 +1022,78 @@ TEST(ExtensionTest, ExtensionKeybindingParsing) {
   }
 }
 
+TEST(ExtensionTest, ExtensionKeybindingParsingFallback) {
+  std::string description = "desc";
+  std::string command_name = "foo";
+
+  // Test that platform specific keys are honored on each platform, despite
+  // fallback being given.
+  scoped_ptr<DictionaryValue> command(new DictionaryValue);
+  DictionaryValue* key_dict = new DictionaryValue();
+  key_dict->SetString("default",  "Ctrl+Shift+D");
+  key_dict->SetString("windows",  "Ctrl+Shift+W");
+  key_dict->SetString("mac",      "Ctrl+Shift+M");
+  key_dict->SetString("linux",    "Ctrl+Shift+L");
+  key_dict->SetString("chromeos", "Ctrl+Shift+C");
+  command->Set("suggested_key", key_dict);
+  command->SetString("description", description);
+
+  Extension::ExtensionKeybinding keybinding;
+  string16 error;
+  EXPECT_TRUE(keybinding.Parse(command.get(), command_name, 0, &error));
+  EXPECT_STREQ(description.c_str(), keybinding.description().c_str());
+  EXPECT_STREQ(command_name.c_str(), keybinding.command_name().c_str());
+
+#if defined(OS_WIN)
+  ui::Accelerator accelerator(ui::VKEY_W, true, true, false);
+#elif defined(OS_MACOSX)
+  ui::Accelerator accelerator(ui::VKEY_M, true, true, false);
+#elif defined(OS_CHROMEOS)
+  ui::Accelerator accelerator(ui::VKEY_C, true, true, false);
+#elif defined(OS_LINUX)
+  ui::Accelerator accelerator(ui::VKEY_L, true, true, false);
+#else
+  ui::Accelerator accelerator(ui::VKEY_D, true, true, false);
+#endif
+  EXPECT_EQ(accelerator, keybinding.accelerator());
+
+  // Misspell a platform.
+  key_dict->SetString("windosw", "Ctrl+M");
+  EXPECT_FALSE(keybinding.Parse(command.get(), command_name, 0, &error));
+  EXPECT_TRUE(key_dict->Remove("windosw", NULL));
+
+  // Now remove platform specific keys (leaving just "default") and make sure
+  // every platform falls back to the default.
+  EXPECT_TRUE(key_dict->Remove("windows", NULL));
+  EXPECT_TRUE(key_dict->Remove("mac", NULL));
+  EXPECT_TRUE(key_dict->Remove("linux", NULL));
+  EXPECT_TRUE(key_dict->Remove("chromeos", NULL));
+  EXPECT_TRUE(keybinding.Parse(command.get(), command_name, 0, &error));
+  EXPECT_EQ(ui::VKEY_D, keybinding.accelerator().key_code());
+
+  // Now remove "default", leaving no option but failure. Or, in the words of
+  // the immortal Adam Savage: "Failure is always an option".
+  EXPECT_TRUE(key_dict->Remove("default", NULL));
+  EXPECT_FALSE(keybinding.Parse(command.get(), command_name, 0, &error));
+
+  // Now add only a valid platform that we are not running on to make sure devs
+  // are notified of errors on other platforms.
+#if defined(OS_WIN)
+  key_dict->SetString("mac", "Ctrl+Shift+M");
+#else
+  key_dict->SetString("windows", "Ctrl+Shift+W");
+#endif
+  EXPECT_FALSE(keybinding.Parse(command.get(), command_name, 0, &error));
+
+  // Make sure Mac specific keys are not processed on other platforms.
+#if !defined(OS_MACOSX)
+  key_dict->SetString("windows", "Command+Shift+M");
+  EXPECT_FALSE(keybinding.Parse(command.get(), command_name, 0, &error));
+  key_dict->SetString("windows", "Options+Shift+M");
+  EXPECT_FALSE(keybinding.Parse(command.get(), command_name, 0, &error));
+#endif
+}
+
 // These last 2 tests don't make sense on Chrome OS, where extension plugins
 // are not allowed.
 #if !defined(OS_CHROMEOS)
diff --git a/chrome/test/data/extensions/api_test/keybinding/basics/manifest.json b/chrome/test/data/extensions/api_test/keybinding/basics/manifest.json
index eed6bd4..7d4033c 100644
--- a/chrome/test/data/extensions/api_test/keybinding/basics/manifest.json
+++ b/chrome/test/data/extensions/api_test/keybinding/basics/manifest.json
@@ -14,12 +14,20 @@
   },
   "commands": {
     "toggle-feature": {
-      "key": "  Ctrl - Shift  - Y  ",
+      "suggested_key": {
+        "windows": "Ctrl+Shift+Y",
+        "mac": "Command+Shift+Y",
+        "linux": "Ctrl+Shift+Y",
+        "chromeos": "Ctrl+Shift+Y",
+        "default": "Ctrl+Shift+Y"
+      },
       "description": "Toggle feature foo"
     },
-    "browserAction": {
-      "key": "Ctrl+Shift+F",
-      "description": ""
+    "_execute_browser_action": {
+      "suggested_key": {
+        "mac": "Command+Shift+F",
+        "default": "Ctrl+Shift+F"
+      }
     }
   }
 }
diff --git a/chrome/test/data/extensions/api_test/keybinding/page_action/manifest.json b/chrome/test/data/extensions/api_test/keybinding/page_action/manifest.json
index d4724db..7ab4636 100644
--- a/chrome/test/data/extensions/api_test/keybinding/page_action/manifest.json
+++ b/chrome/test/data/extensions/api_test/keybinding/page_action/manifest.json
@@ -13,8 +13,8 @@
     "default_icon": "icon.png"
   },
   "commands": {
-    "pageAction": {
-      "key": "Ctrl+Shift+F",
+    "_execute_page_action": {
+      "suggested_key": "Ctrl+Shift+F",
       "description": ""
     }
   }