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 226 matching lines @@
 
 Extension::TtsVoice::TtsVoice() {}
 Extension::TtsVoice::~TtsVoice() {}
 
 Extension::OAuth2Info::OAuth2Info() {}
 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()) {
-    *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
-        errors::kInvalidKeyBinding,
-        base::IntToString(index),
-        "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);
+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::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.
   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] == "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) {
         // 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),
-          original_keybinding);
-      return false;
+          platform_key,
+          shortcut);
+      return ui::Accelerator();
     }
   }
 
   // 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:

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

What about ctrl+alt+shift+foo ?

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

That's just a variation of Ctrl+Alt+something, so it would not be allowed. I
don't think adding a Shift to the mix avoids the AltGr issue described in the
MSDN document linked to on line 302.

But, I've added a test to make sure we don't regress on rejecting this
combination.

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

Bummer. OK>

   // 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),
-        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));
-      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 = "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;
+        }
+      }
+    }
+  }
   return true;
 }
 
 //
 // Extension
 //
 
 // static
 scoped_refptr<Extension> Extension::Create(const FilePath& path,
                                            Location location,
@@ skipping 1165 matching lines @@
       ++command_index;
 
       DictionaryValue* command = NULL;
       if (!commands->GetDictionary(*iter, &command)) {
         *error = ExtensionErrorUtils::FormatErrorMessageUTF16(
             errors::kInvalidKeyBindingDictionary,
             base::IntToString(command_index));
         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;
 }
 
 bool Extension::LoadPlugins(string16* error) {
   if (!manifest_->HasKey(keys::kPlugins))
     return true;
   ListValue* list_value = NULL;
   if (!manifest_->GetList(keys::kPlugins, &list_value)) {
@@ skipping 1996 matching lines @@
     already_disabled(false),
     extension(extension) {}
 
 UpdatedExtensionPermissionsInfo::UpdatedExtensionPermissionsInfo(
     const Extension* extension,
     const ExtensionPermissionSet* permissions,
     Reason reason)
     : reason(reason),
       extension(extension),
       permissions(permissions) {}