Polish the keybinding implementation a bit.

chrome/common/extensions/extension.cc

Index: chrome/common/extensions/extension.cc
===================================================================
--- chrome/common/extensions/extension.cc	(revision 128819)
+++ chrome/common/extensions/extension.cc	(working copy)
@@ -244,37 +244,22 @@
 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()) {
-    *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
-        errors::kInvalidKeyBinding,
-        base::IntToString(index),
-        "Missing");
-    return false;
-  }
+ui::Accelerator Extension::ExtensionKeybinding::ParseImpl(
+    const std::string& shortcut,
+    const std::string& platform_key,
+    int index,
+    string16* error) {
+  std::string shortcut_normalized = StringToLowerASCII(shortcut);

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

Do we even need to do this part? I kinda prefer we make things as unambiguous as
possible. Just define the correct capitalization and require that.

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

Need? No. 

I changed it everywhere to allow only |Ctrl|, not |ctrl| (to take one example)
and required the platform to be lowercase and one word (so |chromeos| and not
|Chrome OS| as I think people will have problems remembering whether it is
|ChromeOS|, |ChromeOs| or |Chromeos|).

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

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

OK, let's try it out this way for awhile and see how hard it is to remember the
correct casing. If it turns out to be super annoying, we can relax it.

 
-  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_normalized, '+', &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.
@@ -289,6 +274,10 @@
       alt = true;
     } 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') {
       if (key != ui::VKEY_UNKNOWN) {
@@ -302,8 +291,9 @@
       *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
           errors::kInvalidKeyBinding,
           base::IntToString(index),
-          original_keybinding);
-      return false;
+          platform_key,
+          shortcut);
+      return ui::Accelerator();
     }
   }
 
@@ -314,26 +304,124 @@
     *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);
+}
 
-  if (command_name !=
-          extension_manifest_values::kPageActionKeybindingEvent &&
-      command_name !=
-          extension_manifest_values::kBrowserActionKeybindingEvent) {
-    if (!command->GetString(keys::kDescription, &description_) ||
-        description_.empty()) {
+bool Extension::ExtensionKeybinding::Parse(DictionaryValue* command,
+                                           const std::string& command_name,
+                                           int index,
+                                           string16* error) {
+  DCHECK(!command_name.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 |commands| section as a dictionary.

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

s/commands/suggested_key/ ?

+  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[StringToLowerASCII(*iter)] = suggested_key_string;

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

Again, I would just require the case to be correct.

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

Do we handle the case where the platform is typo'd?

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

We did not. But we do now.

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

+      } 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));
+          errors::kInvalidKeyBinding,
+          base::IntToString(index),
+          keys::kSuggestedKey,
+          "Missing");
+       return false;
+    }
+  }
+
+  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 = "default";
+  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;
+        }
+      }
+    }
   }
-
-  command_name_ = command_name;
   return true;
 }
 
@@ -1519,11 +1607,18 @@
         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

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

You should skip keys beginning with "_" if they aren't one of the two we know
about. That way we reserve it for future uses.

+        named_commands_[command_name] = *binding.release();
     }
   }
   return true;