mac: Allow WebContents key handling to supplant extension overrides of the bookmark shortcut.

chrome/browser/extensions/extension_keybinding_apitest.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 "base/command_line.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
@@ skipping 264 matching lines @@
   bool result = false;
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
       tab,
       "setInterval(function() {"
       "  if (document.body.bgColor == 'green') {"
       "    window.domAutomationController.send(true)}}, 100)",
       &result));
   ASSERT_TRUE(result);
 }
 
-// Behavior to be implemented on Mac. See http://crbug.com/389340.
-#if defined(OS_MACOSX)
-#define MAYBE_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding DISABLED_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding
-#else
-#define MAYBE_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding
-#endif
 // This test validates that an extension override of the Chrome bookmark
 // shortcut does not supersede the same keybinding by web pages.
-IN_PROC_BROWSER_TEST_F(
-    CommandsApiTest,
-    MAYBE_OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding) {
+IN_PROC_BROWSER_TEST_F(CommandsApiTest,
+                       OverwriteBookmarkShortcutDoesNotOverrideWebKeybinding) {
   ASSERT_TRUE(test_server()->Start());
 
   ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
 
   // This functionality requires a feature flag.
   CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       "--enable-override-bookmarks-ui",
       "1");
 
   ASSERT_TRUE(RunExtensionTest("keybinding/overwrite_bookmark_shortcut"))
@@ skipping 19 matching lines @@
   bool result = false;
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
       tab,
       "setInterval(function() {"
       "  if (document.body.bgColor == 'magenta') {"
       "    window.domAutomationController.send(true)}}, 100)",
       &result));
   ASSERT_TRUE(result);
 }
 
-// Behavior to be implemented on Mac. See http://crbug.com/389340.
-#if defined(OS_MACOSX)
-#define MAYBE_OverwriteBookmarkShortcutByUserOverridesWebKeybinding DISABLED_OverwriteBookmarkShortcutByUserOverridesWebKeybinding
-#else
-#define MAYBE_OverwriteBookmarkShortcutByUserOverridesWebKeybinding OverwriteBookmarkShortcutByUserOverridesWebKeybinding
-#endif
 // This test validates that user-set override of the Chrome bookmark shortcut in
 // an extension that does not request it does supersede the same keybinding by
 // web pages.
-IN_PROC_BROWSER_TEST_F(
-    CommandsApiTest,
-    MAYBE_OverwriteBookmarkShortcutByUserOverridesWebKeybinding) {
+IN_PROC_BROWSER_TEST_F(CommandsApiTest,
+                       OverwriteBookmarkShortcutByUserOverridesWebKeybinding) {
   ASSERT_TRUE(test_server()->Start());
 
   ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
 
   // This functionality requires a feature flag.
   CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       "--enable-override-bookmarks-ui",
       "1");
 
   ASSERT_TRUE(RunExtensionTest("keybinding/basics"))
       << message_;
 
   CommandService* command_service = CommandService::Get(browser()->profile());
 
   const Extension* extension = GetSingleLoadedExtension();
   // Simulate the user setting the keybinding to Ctrl+D.
+#if defined(OS_MACOSX)
+  command_service->UpdateKeybindingPrefs(
+      extension->id(),
+      manifest_values::kBrowserActionCommandEvent,
+      "Command+D");
+#else
   command_service->UpdateKeybindingPrefs(
       extension->id(), manifest_values::kBrowserActionCommandEvent, "Ctrl+D");
+#endif  // defined(OS_MACOSX)

[Scott Hess - ex-Googler, 2014/07/15 19:20:59]

AFAICT, these only differ on "Command+D" vs "Ctrl+D".  Maybe better to have the
#ifdef around a local constant with a single function call.  [Mostly my
complaint is around having the two bits of code look very different.]

[Yoyo Zhou, 2014/07/15 19:46:37]

Agreed.

[erikchen, 2014/07/15 20:48:40]

Done.

 
   ui_test_utils::NavigateToURL(browser(),
       test_server()->GetURL(
           "files/extensions/test_file_with_ctrl-d_keybinding.html"));
 
   WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(tab);
 
   // Activate the shortcut (Ctrl+D) which should be handled by the extension and
   // make the background color red.
@@ skipping 329 matching lines @@
   // Verify the keybinding is still set.
   accelerator = command_service->FindCommandByName(
       kId, manifest_values::kBrowserActionCommandEvent).accelerator();
   EXPECT_EQ(ui::VKEY_G, accelerator.key_code());
   EXPECT_FALSE(accelerator.IsCtrlDown());
   EXPECT_TRUE(accelerator.IsShiftDown());
   EXPECT_TRUE(accelerator.IsAltDown());
 }
 
 }  // namespace extensions