Polish the keybinding implementation a bit.

chrome/common/extensions/extension_unittest.cc

Index: chrome/common/extensions/extension_unittest.cc
===================================================================
--- chrome/common/extensions/extension_unittest.cc	(revision 129600)
+++ chrome/common/extensions/extension_unittest.cc	(working copy)
@@ -945,14 +945,16 @@
     // 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,35 +969,50 @@
     { 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) {
       EXPECT_STREQ(kTests[i].description, keybinding.description().c_str());
@@ -1005,6 +1022,78 @@
   }
 }
 
+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)