Polish the keybinding implementation a bit.

chrome/common/extensions/extension.cc

 // 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.
 
 #include "chrome/common/extensions/extension.h"
 
 #include <algorithm>
 
 #include "base/base64.h"
 #include "base/basictypes.h"
@@ skipping 228 matching lines @@
 Extension::TtsVoice::~TtsVoice() {}
 
 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()) {
+  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);
   if (tokens.size() < 2 || tokens.size() > 3) {
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
+        platformKey,
         original_keybinding);
     return false;
   }
 
   // Now, parse it into an accelerator.
   bool ctrl = false;
   bool alt = false;
   bool shift = false;
   ui::KeyboardCode key = ui::VKEY_UNKNOWN;
   for (size_t i = 0; i < tokens.size(); i++) {
     if (tokens[i] == "ctrl") {
       ctrl = true;
     } else if (tokens[i] == "alt") {
       alt = true;
     } else if (tokens[i] == "shift") {
       shift = true;
     } else if (tokens[i].size() == 1 &&
                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'));
     } else {
       *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
           errors::kInvalidKeyBinding,
           base::IntToString(index),
+          platformKey,
           original_keybinding);
       return false;
     }
   }
 
   // We support Ctrl+foo, Alt+foo, Ctrl+Shift+foo, Alt+Shift+foo, but not
   // Ctrl+Alt+foo. For a more detailed reason why we don't support Ctrl+Alt+foo:
   // http://blogs.msdn.com/b/oldnewthing/archive/2004/03/29/101121.aspx.
   if (key == ui::VKEY_UNKNOWN || (ctrl && alt)) {
     *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
         errors::kInvalidKeyBinding,
         base::IntToString(index),
+        platformKey,
         original_keybinding);
     return false;
   }
 
   accelerator_ = ui::Accelerator(key, shift, ctrl, alt);
 
   if (command_name !=
           extension_manifest_values::kPageActionKeybindingEvent &&
       command_name !=
           extension_manifest_values::kBrowserActionKeybindingEvent) {
@@ skipping 3132 matching lines @@
   // their positions should sync.
   return location() == Extension::INTERNAL ||
       ShouldDisplayInLauncher();
 }
 
 bool Extension::ShouldDisplayInLauncher() const {
   // All apps should be displayed on the NTP except for the Cloud Print App.
   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,
                              Extension::Location location)
     : extension_id(id),
       extension_path(path),
       extension_location(location) {
   if (manifest)
     extension_manifest.reset(manifest->DeepCopy());
 }
@@ skipping 22 matching lines @@
     already_disabled(false),
     extension(extension) {}
 
 UpdatedExtensionPermissionsInfo::UpdatedExtensionPermissionsInfo(
     const Extension* extension,
     const ExtensionPermissionSet* permissions,
     Reason reason)
     : reason(reason),
       extension(extension),
       permissions(permissions) {}