Begin whitelisting specific extensions for global key registration.

chrome/common/extensions/command_unittest.cc

Index: chrome/common/extensions/command_unittest.cc
diff --git a/chrome/common/extensions/command_unittest.cc b/chrome/common/extensions/command_unittest.cc
index b36a86addf1f09e562b4281303acea3e484e4b2b..a093ae1ee657e2051b6cae60929f69b6a8286617 100644
--- a/chrome/common/extensions/command_unittest.cc
+++ b/chrome/common/extensions/command_unittest.cc
@@ -14,6 +14,74 @@
 class CommandTest : public testing::Test {
 };
 
+typedef const struct {
+  bool expected_result;
+  ui::Accelerator accelerator;
+  const char* command_name;
+  const char* key;
+  const char* description;
+} ConstCommandsTestData;
+
+void CheckExpectations(ConstCommandsTestData data,
+                       int i,
+                       bool skip_simple_parse,
+                       bool platform_default,
+                       bool windows,
+                       bool mac,
+                       bool chromeos) {
+  scoped_ptr<base::DictionaryValue> input(new base::DictionaryValue);
+  extensions::Command command;
+  base::string16 error;
+
+  // First parse the command as a simple string.
+  if (!skip_simple_parse) {
+    input->SetString("suggested_key", data.key);
+    input->SetString("description", data.description);
+
+    SCOPED_TRACE(std::string("Command name: |") + data.command_name +
+                 "| key: |" + data.key + "| description: |" + data.description +
+                 "| index: " + base::IntToString(i));
+
+    bool result = command.Parse(input.get(), data.command_name, i, &error);
+
+    EXPECT_EQ(data.expected_result, result);
+    if (result) {
+      EXPECT_STREQ(data.description,
+                   base::UTF16ToASCII(command.description()).c_str());
+      EXPECT_STREQ(data.command_name, command.command_name().c_str());
+      EXPECT_EQ(data.accelerator, command.accelerator());
+    }
+  }
+
+  // Now parse the command as a dictionary of multiple values.
+  if (data.key[0] != '\0') {
+    input.reset(new base::DictionaryValue);
+    base::DictionaryValue* key_dict = new base::DictionaryValue();
+
+    if (platform_default)
+      key_dict->SetString("default", data.key);
+    if (windows)
+      key_dict->SetString("windows", data.key);
+    if (mac)
+      key_dict->SetString("mac", data.key);
+    if (chromeos)
+      key_dict->SetString("chromeos", data.key);
+
+    input->Set("suggested_key", key_dict);
+    input->SetString("description", data.description);
+
+    bool result = command.Parse(input.get(), data.command_name, i, &error);
+
+    EXPECT_EQ(data.expected_result, result);
+    if (result) {
+      EXPECT_STREQ(data.description,
+                   base::UTF16ToASCII(command.description()).c_str());
+      EXPECT_STREQ(data.command_name, command.command_name().c_str());
+      EXPECT_EQ(data.accelerator, command.accelerator());
+    }
+  }
+}

[Finnur, 2014/08/21 11:18:11]

Same here. Version we reviewed before is more up-to-date.

[David Tseng, 2014/08/21 16:17:56]

Acknowledged.

+
 TEST(CommandTest, ExtensionCommandParsing) {
   const ui::Accelerator none = ui::Accelerator();
   const ui::Accelerator shift_f = ui::Accelerator(ui::VKEY_F,
@@ -52,116 +120,68 @@ TEST(CommandTest, ExtensionCommandParsing) {
   const ui::Accelerator stop =
       ui::Accelerator(ui::VKEY_MEDIA_STOP, ui::EF_NONE);
 
-  const struct {
-    bool expected_result;
-    ui::Accelerator accelerator;
-    const char* command_name;
-    const char* key;
-    const char* description;
-  } kTests[] = {
-    // Negative test (one or more missing required fields). We don't need to
-    // test |command_name| being blank as it is used as a key in the manifest,
-    // so it can't be blank (and we CHECK() when it is). A blank shortcut is
-    // permitted.
-    { false, none, "command", "",       "" },
-    { false, none, "command", "Ctrl+f", "" },
-    // Ctrl+Alt is not permitted, see MSDN link in comments in Parse function.
-    { false, none, "command", "Ctrl+Alt+F", "description" },
-    // Unsupported shortcuts/too many, or missing modifier.
-    { false, none, "command", "A",                "description" },
-    { false, none, "command", "F10",              "description" },
-    { false, none, "command", "Ctrl+F+G",         "description" },
-    { false, none, "command", "Ctrl+Alt+Shift+G", "description" },
-    // Shift on its own is not supported.
-    { false, shift_f, "command", "Shift+F",       "description" },
-    { false, shift_f, "command", "F+Shift",       "description" },
-    // Basic tests.
-    { true, none,         "command", "",             "description" },
-    { true, ctrl_f,       "command", "Ctrl+F",       "description" },
-    { true, alt_f,        "command", "Alt+F",        "description" },
-    { true, ctrl_shift_f, "command", "Ctrl+Shift+F", "description" },
-    { true, alt_shift_f,  "command", "Alt+Shift+F",  "description" },
-    { true, ctrl_1,       "command", "Ctrl+1",       "description" },
-    // Shortcut token order tests.
-    { true, ctrl_f,       "command", "F+Ctrl",       "description" },
-    { true, alt_f,        "command", "F+Alt",        "description" },
-    { true, ctrl_shift_f, "command", "F+Ctrl+Shift", "description" },
-    { true, ctrl_shift_f, "command", "F+Shift+Ctrl", "description" },
-    { true, alt_shift_f,  "command", "F+Alt+Shift",  "description" },
-    { true, alt_shift_f,  "command", "F+Shift+Alt",  "description" },
-    // Case insensitivity is not OK.
-    { false, ctrl_f, "command", "Ctrl+f", "description" },
-    { false, ctrl_f, "command", "cTrL+F", "description" },
-    // Skipping description is OK for browser- and pageActions.
-    { true, ctrl_f, "_execute_browser_action", "Ctrl+F", "" },
-    { true, ctrl_f, "_execute_page_action",    "Ctrl+F", "" },
-    // Home, End, Arrow keys, etc.
-    { true, ctrl_comma, "_execute_browser_action", "Ctrl+Comma", "" },
-    { true, ctrl_dot, "_execute_browser_action", "Ctrl+Period", "" },
-    { true, ctrl_left, "_execute_browser_action", "Ctrl+Left", "" },
-    { true, ctrl_right, "_execute_browser_action", "Ctrl+Right", "" },
-    { true, ctrl_up, "_execute_browser_action", "Ctrl+Up", "" },
-    { true, ctrl_down, "_execute_browser_action", "Ctrl+Down", "" },
-    { true, ctrl_ins, "_execute_browser_action", "Ctrl+Insert", "" },
-    { true, ctrl_del, "_execute_browser_action", "Ctrl+Delete", "" },
-    { true, ctrl_home, "_execute_browser_action", "Ctrl+Home", "" },
-    { true, ctrl_end, "_execute_browser_action", "Ctrl+End", "" },
-    { true, ctrl_pgup, "_execute_browser_action", "Ctrl+PageUp", "" },
-    { true, ctrl_pgdwn, "_execute_browser_action", "Ctrl+PageDown", "" },
-    // Media keys.
-    { true, next_track, "command", "MediaNextTrack", "description" },
-    { true, play_pause, "command", "MediaPlayPause", "description" },
-    { true, prev_track, "command", "MediaPrevTrack", "description" },
-    { true, stop, "command", "MediaStop", "description" },
-    { false, none, "_execute_browser_action", "MediaNextTrack", "" },
-    { false, none, "_execute_page_action", "MediaPrevTrack", "" },
-    { false, none, "command", "Ctrl+Shift+MediaPrevTrack", "description" },
+  ConstCommandsTestData kTests[] = {
+      // Negative test (one or more missing required fields). We don't need to
+      // test |command_name| being blank as it is used as a key in the manifest,
+      // so it can't be blank (and we CHECK() when it is). A blank shortcut is
+      // permitted.
+      {false, none, "command", "", ""},
+      {false, none, "command", "Ctrl+f", ""},
+      // Ctrl+Alt is not permitted, see MSDN link in comments in Parse function.
+      {false, none, "command", "Ctrl+Alt+F", "description"},
+      // Unsupported shortcuts/too many, or missing modifier.
+      {false, none, "command", "A", "description"},
+      {false, none, "command", "F10", "description"},
+      {false, none, "command", "Ctrl+F+G", "description"},
+      {false, none, "command", "Ctrl+Alt+Shift+G", "description"},
+      // Shift on its own is not supported.
+      {false, shift_f, "command", "Shift+F", "description"},
+      {false, shift_f, "command", "F+Shift", "description"},
+      // Basic tests.
+      {true, none, "command", "", "description"},
+      {true, ctrl_f, "command", "Ctrl+F", "description"},
+      {true, alt_f, "command", "Alt+F", "description"},
+      {true, ctrl_shift_f, "command", "Ctrl+Shift+F", "description"},
+      {true, alt_shift_f, "command", "Alt+Shift+F", "description"},
+      {true, ctrl_1, "command", "Ctrl+1", "description"},
+      // Shortcut token order tests.
+      {true, ctrl_f, "command", "F+Ctrl", "description"},
+      {true, alt_f, "command", "F+Alt", "description"},
+      {true, ctrl_shift_f, "command", "F+Ctrl+Shift", "description"},
+      {true, ctrl_shift_f, "command", "F+Shift+Ctrl", "description"},
+      {true, alt_shift_f, "command", "F+Alt+Shift", "description"},
+      {true, alt_shift_f, "command", "F+Shift+Alt", "description"},
+      // Case insensitivity is not OK.
+      {false, ctrl_f, "command", "Ctrl+f", "description"},
+      {false, ctrl_f, "command", "cTrL+F", "description"},
+      // Skipping description is OK for browser- and pageActions.
+      {true, ctrl_f, "_execute_browser_action", "Ctrl+F", ""},
+      {true, ctrl_f, "_execute_page_action", "Ctrl+F", ""},
+      // Home, End, Arrow keys, etc.
+      {true, ctrl_comma, "_execute_browser_action", "Ctrl+Comma", ""},
+      {true, ctrl_dot, "_execute_browser_action", "Ctrl+Period", ""},
+      {true, ctrl_left, "_execute_browser_action", "Ctrl+Left", ""},
+      {true, ctrl_right, "_execute_browser_action", "Ctrl+Right", ""},
+      {true, ctrl_up, "_execute_browser_action", "Ctrl+Up", ""},
+      {true, ctrl_down, "_execute_browser_action", "Ctrl+Down", ""},
+      {true, ctrl_ins, "_execute_browser_action", "Ctrl+Insert", ""},
+      {true, ctrl_del, "_execute_browser_action", "Ctrl+Delete", ""},
+      {true, ctrl_home, "_execute_browser_action", "Ctrl+Home", ""},
+      {true, ctrl_end, "_execute_browser_action", "Ctrl+End", ""},
+      {true, ctrl_pgup, "_execute_browser_action", "Ctrl+PageUp", ""},
+      {true, ctrl_pgdwn, "_execute_browser_action", "Ctrl+PageDown", ""},
+      // Media keys.
+      {true, next_track, "command", "MediaNextTrack", "description"},
+      {true, play_pause, "command", "MediaPlayPause", "description"},
+      {true, prev_track, "command", "MediaPrevTrack", "description"},
+      {true, stop, "command", "MediaStop", "description"},
+      {false, none, "_execute_browser_action", "MediaNextTrack", ""},
+      {false, none, "_execute_page_action", "MediaPrevTrack", ""},
+      {false, none, "command", "Ctrl+Shift+MediaPrevTrack", "description"},
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kTests); ++i) {
-    // First parse the command as a simple string.
-    scoped_ptr<base::DictionaryValue> input(new base::DictionaryValue);
-    input->SetString("suggested_key", kTests[i].key);
-    input->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));
-
-    extensions::Command command;
-    base::string16 error;
-    bool result =
-        command.Parse(input.get(), kTests[i].command_name, i, &error);
-
-    EXPECT_EQ(kTests[i].expected_result, result);
-    if (result) {
-      EXPECT_STREQ(kTests[i].description,
-                   base::UTF16ToASCII(command.description()).c_str());
-      EXPECT_STREQ(kTests[i].command_name, command.command_name().c_str());
-      EXPECT_EQ(kTests[i].accelerator, command.accelerator());
-    }
-
-    // Now parse the command as a dictionary of multiple values.
-    if (kTests[i].key[0] != '\0') {
-      input.reset(new base::DictionaryValue);
-      base::DictionaryValue* key_dict = new base::DictionaryValue();
-      key_dict->SetString("default", kTests[i].key);
-      key_dict->SetString("windows", kTests[i].key);
-      key_dict->SetString("mac", kTests[i].key);
-      input->Set("suggested_key", key_dict);
-      input->SetString("description", kTests[i].description);
-
-      result = command.Parse(input.get(), kTests[i].command_name, i, &error);
-
-      EXPECT_EQ(kTests[i].expected_result, result);
-      if (result) {
-        EXPECT_STREQ(kTests[i].description,
-                     base::UTF16ToASCII(command.description()).c_str());
-        EXPECT_STREQ(kTests[i].command_name, command.command_name().c_str());
-        EXPECT_EQ(kTests[i].accelerator, command.accelerator());
-      }
-    }
+    CheckExpectations(kTests[i], false, i, true, true, true, true);
   }
 }
 
@@ -248,3 +268,28 @@ TEST(CommandTest, ExtensionCommandParsingFallback) {
   EXPECT_FALSE(command.Parse(input.get(), command_name, 0, &error));
 #endif
 }
+
+TEST(CommandTest, ExtensionCommandParsingPlatformSpecific) {
+  ui::Accelerator search_shift_z(ui::VKEY_Z,
+                                 ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN);
+
+  ConstCommandsTestData kChromeOsTests[] = {
+      {true, search_shift_z, "command", "Search+Shift+Z", "description"},
+      {false, search_shift_z, "command", "Command+Shift+Z", "description"},
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kChromeOsTests); ++i) {
+    // Search is only valid on Chrome OS and default platforms.
+    CheckExpectations(kChromeOsTests[i], i, false, true, false, false, true);
+  }
+
+  ConstCommandsTestData kNonChromeOsSearchTests[] = {
+      {false, search_shift_z, "command", "Search+Shift+Z", "description"},
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kNonChromeOsSearchTests); ++i) {
+    // Make sure dictionary parsing fails on Windows and Mac.
+    CheckExpectations(
+        kNonChromeOsSearchTests[i], i, true, false, true, true, false);
+  }
+}