Polish the keybinding implementation a bit.

chrome/common/extensions/extension.cc

Index: chrome/common/extensions/extension.cc
===================================================================
--- chrome/common/extensions/extension.cc	(revision 127690)
+++ chrome/common/extensions/extension.cc	(working copy)
@@ -246,23 +246,50 @@
                                            int index,
                                            string16* error) {
   DCHECK(!command_name.empty());
-  std::string key_binding;
-  if (!command->GetString(keys::kKey, &key_binding) ||
-      key_binding.empty()) {
+  std::string key_binding_win;
+  std::string key_binding_mac;
+  std::string key_binding_other;
+  if (!command->GetString(keys::kSuggestedKeyWin, &key_binding_win) ||

[Aaron Boodman, 2012/03/21 21:05:04]

How about setting this up like:

suggested_key: {
  default: "...",
  linux: "...",
  windows: "...",
  chromeos: "...",
  mac: "..."
}

All would be optional except default. If one is not present, it falls back to
'default'.

It would also be nice if, in the case where no specific platform is listed the
developer could do:

suggested_key: "..."

which would be equivalent to:

suggested_key: {
  other: "..."
}

[Finnur, 2012/03/21 22:42:12]

If only 'default' is provided, do we map Ctrl to Command on Mac (and Alt to
Options)?
Not sure I follow. Do you mean: 


... which would be equivalent to:
 
suggested_key: {
  default: "..."
}

+      key_binding_win.empty()) {
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
+        keys::kSuggestedKeyWin,
         "Missing");
     return false;
   }
+  if (!command->GetString(keys::kSuggestedKeyMac, &key_binding_mac) ||
+      key_binding_mac.empty()) {
+    *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
+        errors::kInvalidKeyBinding,
+        base::IntToString(index),
+        keys::kSuggestedKeyMac,
+        "Missing");
+    return false;
+  }
+  if (!command->GetString(keys::kSuggestedKeyOther, &key_binding_other) ||
+      key_binding_other.empty()) {
+    *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
+        errors::kInvalidKeyBinding,
+        base::IntToString(index),
+        keys::kSuggestedKeyOther,
+        "Missing");
+    return false;
+  }
 
-  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);
+#if defined(OS_WIN)

[Aaron Boodman, 2012/03/21 21:05:04]

Could you do this work before parsing the JSON above, that way you only try to
parse the bits that are needed.

[Finnur, 2012/03/21 22:42:12]

I'm thinking I should probably refactor this to read one key from the manifest
and send as param which key (which platform) to read. Need to think about that
some more.

However, I think I'd want (as an ext developer) to not have to test my
keybinding on all platforms to make sure I got them right. So, I'm suggesting we
parse them all on all platforms, but only use the one we need. WDYT?


On 2012/03/21 21:05:04, Aaron Boodman wrote:

+  std::string original_keybinding = key_binding_win;
+  std::string key_binding = StringToLowerASCII(key_binding_win);
+  std::string platformKey = keys::kSuggestedKeyWin;

[Aaron Boodman, 2012/03/21 21:05:04]

platform_key

+#elif defined(OS_MACOSX)
+  std::string original_keybinding = key_binding_mac;
+  std::string key_binding = StringToLowerASCII(key_binding_mac);
+  std::string platformKey = keys::kSuggestedKeyMac;
+#else
+  std::string original_keybinding = key_binding_other;
+  std::string key_binding = StringToLowerASCII(key_binding_other);
+  std::string platformKey = keys::kSuggestedKeyOther;
+#endif
 
   std::vector<std::string> tokens;
   base::SplitString(key_binding, '+', &tokens);
@@ -270,6 +297,7 @@
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
+        platformKey,
         original_keybinding);
     return false;
   }
@@ -299,6 +327,7 @@
       *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
           errors::kInvalidKeyBinding,
           base::IntToString(index),
+          platformKey,
           original_keybinding);
       return false;
     }
@@ -311,6 +340,7 @@
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
+        platformKey,
         original_keybinding);
     return false;
   }
@@ -3463,6 +3493,40 @@
   return is_app() && id() != extension_misc::kCloudPrintAppId;
 }
 
+void Extension::GetCommandByType(
+    Extension::ExtensionKeybinding::Type type,
+    std::vector<Extension::ExtensionKeybinding>* keybindings) const {
+  keybindings->clear();
+  if (!commands_.empty()) {
+    for (size_t i = 0; i < commands_.size(); ++i) {
+      switch (type) {
+       case Extension::ExtensionKeybinding::BROWSER_ACTION:
+        if (commands_[i].command_name() ==
+            extension_manifest_values::kBrowserActionKeybindingEvent) {
+          keybindings->push_back(commands_[i]);
+          return;
+        }
+        break;
+       case Extension::ExtensionKeybinding::PAGE_ACTION:
+        if (commands_[i].command_name() ==
+            extension_manifest_values::kPageActionKeybindingEvent) {
+          keybindings->push_back(commands_[i]);
+          return;
+        }
+        break;
+       case Extension::ExtensionKeybinding::COMMANDS:
+        if (commands_[i].command_name() !=
+                extension_manifest_values::kBrowserActionKeybindingEvent &&
+            commands_[i].command_name() !=
+                extension_manifest_values::kPageActionKeybindingEvent) {
+          keybindings->push_back(commands_[i]);
+        }
+        break;
+      }
+    }
+  }
+}
+
 ExtensionInfo::ExtensionInfo(const DictionaryValue* manifest,
                              const std::string& id,
                              const FilePath& path,