Fix extension command service to check if user has modified key before overwrite on update.

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 "base/strings/stringprintf.h"

[Finnur, 2014/10/22 09:43:03]

Remove?

 #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"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ skipping 628 matching lines @@
   CommandService* command_service = CommandService::Get(browser()->profile());
 
   // Install v1 of the extension.
   ASSERT_TRUE(InstallExtension(path_v1, 1));
   EXPECT_TRUE(registry->GetExtensionById(kId, ExtensionRegistry::ENABLED) !=
               NULL);
 
   // Verify it has a command of Alt+Shift+F.
   ui::Accelerator accelerator = command_service->FindCommandByName(
       kId, manifest_values::kBrowserActionCommandEvent).accelerator();
+

[Finnur, 2014/10/22 09:43:03]

nit: remove this linebreak.

   EXPECT_EQ(ui::VKEY_F, accelerator.key_code());
   EXPECT_FALSE(accelerator.IsCtrlDown());
   EXPECT_TRUE(accelerator.IsShiftDown());
   EXPECT_TRUE(accelerator.IsAltDown());
 
   // Simulate the user setting the keybinding to Alt+Shift+G.
   command_service->UpdateKeybindingPrefs(
       kId, manifest_values::kBrowserActionCommandEvent, kAltShiftG);
 
   // Update to version 2 with different keybinding assigned.
   EXPECT_TRUE(UpdateExtension(kId, path_v2_reassigned, 0));
   EXPECT_TRUE(registry->GetExtensionById(kId, ExtensionRegistry::ENABLED) !=
               NULL);
 
   // Verify it has a command of Alt+Shift+G.
   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());
 }
 
+// Test that Media keys do not overwrite previous settings
+IN_PROC_BROWSER_TEST_F(CommandsApiTest,
+                       MediaKeyShortcutChangedOnUpdateAfterBeingReassignedByUser) {

[Finnur, 2014/10/22 09:43:03]

nit: Adhere to the 80 cols rule.

+  base::ScopedTempDir scoped_temp_dir;
+  EXPECT_TRUE(scoped_temp_dir.CreateUniqueTempDir());
+  base::FilePath pem_path = test_data_dir_.
+      AppendASCII("keybinding").AppendASCII("keybinding.pem");
+  base::FilePath path_v1 = PackExtensionWithOptions(
+      test_data_dir_.AppendASCII("keybinding").AppendASCII("update")
+                    .AppendASCII("mk_v1"),
+      scoped_temp_dir.path().AppendASCII("mk_v1.crx"),
+      pem_path,
+      base::FilePath());
+  base::FilePath path_v2_reassigned = PackExtensionWithOptions(
+      test_data_dir_.AppendASCII("keybinding").AppendASCII("update")
+                    .AppendASCII("mk_v2"),
+      scoped_temp_dir.path().AppendASCII("mk_v2.crx"),
+      pem_path,
+      base::FilePath());
+
+  ExtensionRegistry* registry = ExtensionRegistry::Get(browser()->profile());
+  CommandService* command_service = CommandService::Get(browser()->profile());
+
+  // Install v1 of the extension.
+  ASSERT_TRUE(InstallExtension(path_v1, 1));
+  EXPECT_TRUE(registry->GetExtensionById(kId, ExtensionRegistry::ENABLED) !=
+              NULL);
+
+  // Verify it has a command of MediaPlayPause
+  ui::Accelerator accelerator = command_service->FindCommandByName(
+      kId, manifest_values::kMediaKeyTestCommand).accelerator();
+

[Finnur, 2014/10/22 09:43:03]

nit: Same here -- don't need the line break.

+  EXPECT_EQ(ui::VKEY_MEDIA_PLAY_PAUSE, accelerator.key_code());
+  EXPECT_FALSE(accelerator.IsCtrlDown());
+  EXPECT_FALSE(accelerator.IsShiftDown());
+  EXPECT_FALSE(accelerator.IsAltDown());
+
+  // Simulate the user setting the keybinding to Alt+Shift+G.
+  command_service->UpdateKeybindingPrefs(
+      kId, manifest_values::kMediaKeyTestCommand, kAltShiftG);
+
+  // Update to version 2 with different keybinding assigned.
+  EXPECT_TRUE(UpdateExtension(kId, path_v2_reassigned, 0));
+  EXPECT_TRUE(registry->GetExtensionById(kId, ExtensionRegistry::ENABLED) !=
+              NULL);
+
+  // Verify it has a command of Alt+Shift+G.
+  accelerator = command_service->FindCommandByName(
+      kId, manifest_values::kMediaKeyTestCommand).accelerator();
+
+  EXPECT_EQ(ui::VKEY_G, accelerator.key_code());
+  EXPECT_FALSE(accelerator.IsCtrlDown());
+  EXPECT_TRUE(accelerator.IsShiftDown());
+  EXPECT_TRUE(accelerator.IsAltDown());
+}
+
 IN_PROC_BROWSER_TEST_F(CommandsApiTest,
                        ShortcutRemovedOnUpdateAfterBeingReassignedByUser) {
   base::ScopedTempDir scoped_temp_dir;
   EXPECT_TRUE(scoped_temp_dir.CreateUniqueTempDir());
   base::FilePath pem_path = test_data_dir_.
       AppendASCII("keybinding").AppendASCII("keybinding.pem");
   base::FilePath path_v1 = PackExtensionWithOptions(
       test_data_dir_.AppendASCII("keybinding").AppendASCII("update")
                     .AppendASCII("v1"),
       scoped_temp_dir.path().AppendASCII("v1.crx"),
@@ skipping 85 matching lines @@
 #define MAYBE_ChromeOSConversions DISABLED_ChromeOSConversions
 #else
 #define MAYBE_ChromeOSConversions ChromeOSConversions
 #endif
 IN_PROC_BROWSER_TEST_F(CommandsApiTest, MAYBE_ChromeOSConversions) {
   RunChromeOSConversionTest("keybinding/chromeos_conversions");
 }
 #endif  // OS_CHROMEOS
 
 }  // namespace extensions