diff options
6 files changed, 165 insertions, 7 deletions
diff --git a/chrome/browser/extensions/api/commands/command_service.cc b/chrome/browser/extensions/api/commands/command_service.cc index dc66682..cfcf660 100644 --- a/chrome/browser/extensions/api/commands/command_service.cc +++ b/chrome/browser/extensions/api/commands/command_service.cc @@ -230,8 +230,21 @@ bool CommandService::AddKeybindingPref( std::string key = GetPlatformKeybindingKeyForAccelerator(accelerator, extension_id); - if (!allow_overrides && bindings->HasKey(key)) - return false; // Already taken. + if (bindings->HasKey(key)) { + if (!allow_overrides) + return false; // Already taken. + + // If the shortcut has been assigned to another command, it should be + // removed before overriding, so that |ExtensionKeybindingRegistry| can get + // a chance to do clean-up. + const base::DictionaryValue* item = NULL; + bindings->GetDictionary(key, &item); + std::string old_extension_id; + std::string old_command_name; + item->GetString(kExtension, &old_extension_id); + item->GetString(kCommandName, &old_command_name); + RemoveKeybindingPrefs(old_extension_id, old_command_name); + } base::DictionaryValue* keybinding = new base::DictionaryValue(); keybinding->SetString(kExtension, extension_id); diff --git a/chrome/browser/extensions/api/commands/command_service_browsertest.cc b/chrome/browser/extensions/api/commands/command_service_browsertest.cc index 1a1a6c1..12a6e57 100644 --- a/chrome/browser/extensions/api/commands/command_service_browsertest.cc +++ b/chrome/browser/extensions/api/commands/command_service_browsertest.cc @@ -5,10 +5,31 @@ #include "chrome/browser/extensions/api/commands/command_service.h" #include "chrome/browser/extensions/extension_apitest.h" #include "chrome/browser/extensions/extension_service.h" +#include "chrome/common/pref_names.h" #include "content/public/test/browser_test.h" #include "content/public/test/test_utils.h" #include "extensions/common/manifest_constants.h" +namespace { + +// Get another command platform, whcih is used for simulating a command has been +// assigned with a shortcut on another platform. +std::string GetAnotherCommandPlatform() { +#if defined(OS_WIN) + return extensions::manifest_values::kKeybindingPlatformMac; +#elif defined(OS_MACOSX) + return extensions::manifest_values::kKeybindingPlatformChromeOs; +#elif defined(OS_CHROMEOS) + return extensions::manifest_values::kKeybindingPlatformLinux; +#elif defined(OS_LINUX) + return extensions::manifest_values::kKeybindingPlatformWin; +#else + return ""; +#endif +} + +} // namespace + namespace extensions { typedef ExtensionApiTest CommandServiceTest; @@ -68,4 +89,33 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest, RemoveShortcutSurvivesUpdate) { EXPECT_EQ(ui::VKEY_UNKNOWN, accelerator.key_code()); } +IN_PROC_BROWSER_TEST_F(CommandServiceTest, + RemoveKeybindingPrefsShouldBePlatformSpecific) { + base::FilePath extension_dir = + test_data_dir_.AppendASCII("keybinding").AppendASCII("basics"); + const Extension* extension = InstallExtension(extension_dir, 1); + ASSERT_TRUE(extension); + + DictionaryPrefUpdate updater(browser()->profile()->GetPrefs(), + prefs::kExtensionCommands); + base::DictionaryValue* bindings = updater.Get(); + + // Simulate command |toggle-feature| has been assigned with a shortcut on + // another platform. + std::string anotherPlatformKey = GetAnotherCommandPlatform() + ":Alt+G"; + const char kNamedCommandName[] = "toggle-feature"; + base::DictionaryValue* keybinding = new base::DictionaryValue(); + keybinding->SetString("extension", extension->id()); + keybinding->SetString("command_name", kNamedCommandName); + keybinding->SetBoolean("global", false); + bindings->Set(anotherPlatformKey, keybinding); + + CommandService* command_service = CommandService::Get(browser()->profile()); + command_service->RemoveKeybindingPrefs(extension->id(), kNamedCommandName); + + // Removal of keybinding preference should be platform-specific, so the key on + // another platform should always remained. + EXPECT_TRUE(bindings->HasKey(anotherPlatformKey)); +} + } // namespace extensions diff --git a/chrome/browser/extensions/extension_keybinding_apitest.cc b/chrome/browser/extensions/extension_keybinding_apitest.cc index 22e56be..7886afa 100644 --- a/chrome/browser/extensions/extension_keybinding_apitest.cc +++ b/chrome/browser/extensions/extension_keybinding_apitest.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #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" @@ -287,4 +288,55 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, MAYBE_AllowDuplicatedMediaKeys) { ASSERT_TRUE(catcher.GetNextResult()); } +// This test validates that update (including removal) of keybinding preferences +// works correctly. +IN_PROC_BROWSER_TEST_F(CommandsApiTest, UpdateKeybindingPrefsTest) { +#if defined(OS_MACOSX) + // Send "Tab" on OS X to move the focus, otherwise the omnibox will intercept + // the key presses we send below. + ASSERT_TRUE(ui_test_utils::SendKeyPressSync( + browser(), ui::VKEY_TAB, false, false, false, false)); +#endif + ResultCatcher catcher; + ASSERT_TRUE(RunExtensionTest("keybinding/command_update")); + ASSERT_TRUE(catcher.GetNextResult()); + const Extension* extension = GetSingleLoadedExtension(); + + CommandService* command_service = CommandService::Get(browser()->profile()); + extensions::CommandMap named_commands; + command_service->GetNamedCommands(extension->id(), + extensions::CommandService::ACTIVE_ONLY, + extensions::CommandService::ANY_SCOPE, + &named_commands); + EXPECT_EQ(3u, named_commands.size()); + + const char kCommandNameC[] = "command_C"; + command_service->RemoveKeybindingPrefs(extension->id(), kCommandNameC); + command_service->GetNamedCommands(extension->id(), + extensions::CommandService::ACTIVE_ONLY, + extensions::CommandService::ANY_SCOPE, + &named_commands); + EXPECT_EQ(2u, named_commands.size()); + + // Send "Alt+C", it shouldn't work because it has been removed. + ASSERT_TRUE(ui_test_utils::SendKeyPressSync( + browser(), ui::VKEY_C, false, false, true, false)); + + const char kCommandNameB[] = "command_B"; + const char kKeyStroke[] = "Alt+A"; + command_service->UpdateKeybindingPrefs(extension->id(), + kCommandNameB, + kKeyStroke); + command_service->GetNamedCommands(extension->id(), + extensions::CommandService::ACTIVE_ONLY, + extensions::CommandService::ANY_SCOPE, + &named_commands); + EXPECT_EQ(1u, named_commands.size()); + + // Activate the shortcut (Alt+A). + ASSERT_TRUE(ui_test_utils::SendKeyPressSync( + browser(), ui::VKEY_A, false, false, true, false)); + ASSERT_TRUE(catcher.GetNextResult()) << message_; +} + } // namespace extensions diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc b/chrome/browser/extensions/extension_keybinding_registry.cc index 94be6c1..cca0c28 100644 --- a/chrome/browser/extensions/extension_keybinding_registry.cc +++ b/chrome/browser/extensions/extension_keybinding_registry.cc @@ -55,12 +55,12 @@ void ExtensionKeybindingRegistry::RemoveExtensionKeybinding( // Let each platform-specific implementation get a chance to clean up. RemoveExtensionKeybindingImpl(old->first, command_name); event_targets_.erase(old); - } - // If a specific command_name was requested, it has now been deleted so no - // further work is required. - if (!command_name.empty()) - break; + // If a specific command_name was requested, it has now been deleted so no + // further work is required. + if (!command_name.empty()) + break; + } } } diff --git a/chrome/test/data/extensions/api_test/keybinding/command_update/background.js b/chrome/test/data/extensions/api_test/keybinding/command_update/background.js new file mode 100644 index 0000000..d9f9e38 --- /dev/null +++ b/chrome/test/data/extensions/api_test/keybinding/command_update/background.js @@ -0,0 +1,15 @@ +// Copyright 2013 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. + +chrome.commands.onCommand.addListener(function(command) { + if (command == "command_B") { + chrome.test.notifyPass(); + return; + } + + // Everything else is a failure case. + chrome.test.notifyFail("Unexpected command received: " + command); +}); + +chrome.test.notifyPass(); diff --git a/chrome/test/data/extensions/api_test/keybinding/command_update/manifest.json b/chrome/test/data/extensions/api_test/keybinding/command_update/manifest.json new file mode 100644 index 0000000..bac4177 --- /dev/null +++ b/chrome/test/data/extensions/api_test/keybinding/command_update/manifest.json @@ -0,0 +1,28 @@ +{ + "name": "An extension to test updating keybinding preferences", + "version": "1.0", + "manifest_version": 2, + "background": { + "scripts": ["background.js"] + }, + "commands": { + "command_A": { + "suggested_key": { + "default": "Alt+A" + }, + "description": "Test fails if called (It will be removed during testing)." + }, + "command_B": { + "suggested_key": { + "default": "Alt+B" + }, + "description": "Will be uptated to 'Alt+A' during testing." + }, + "command_C": { + "suggested_key": { + "default": "Alt+C" + }, + "description": "Test fails if called (It will be removed during testing)." + } + } +} |