Make sure keybindings removed don't come back during extension update.

BUG=234947
TBR=yoz@chromium.org

Review URL: https://codereview.chromium.org/14990002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@198873 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/api/commands/command_service.cc

@@ -23,18 +23,40 @@
 #include "content/public/browser/notification_service.h"
 
 using extensions::Extension;
+using extensions::ExtensionPrefs;
 
 namespace {
 
 const char kExtension[] = "extension";
 const char kCommandName[] = "command_name";
 
+// A preference that indicates that the initial keybindings for the given
+// extension have been set.
+const char kInitialBindingsHaveBeenAssigned[] = "initial_keybindings_set";
+
 std::string GetPlatformKeybindingKeyForAccelerator(
     const ui::Accelerator& accelerator) {
   return extensions::Command::CommandPlatform() + ":" +
          UTF16ToUTF8(accelerator.GetShortcutText());
 }
 
+void SetInitialBindingsHaveBeenAssigned(
+    ExtensionPrefs* prefs, const std::string& extension_id) {
+  prefs->UpdateExtensionPref(extension_id, kInitialBindingsHaveBeenAssigned,
+                             Value::CreateBooleanValue(true));
+}
+
+bool InitialBindingsHaveBeenAssigned(
+    const ExtensionPrefs* prefs, const std::string& extension_id) {
+  bool assigned = false;
+  if (!prefs || !prefs->ReadPrefAsBoolean(extension_id,
+                                          kInitialBindingsHaveBeenAssigned,
+                                          &assigned))
+    return false;
+
+  return assigned;
+}
+
 }  // namespace
 
 namespace extensions {
@@ -231,6 +253,13 @@ void CommandService::AssignInitialKeybindings(const Extension* extension) {
   if (!commands)
     return;
 
+  ExtensionService* extension_service =
+      ExtensionSystem::Get(profile_)->extension_service();
+  ExtensionPrefs* extension_prefs = extension_service->extension_prefs();
+  if (InitialBindingsHaveBeenAssigned(extension_prefs, extension->id()))
+    return;
+  SetInitialBindingsHaveBeenAssigned(extension_prefs, extension->id());
+
   extensions::CommandMap::const_iterator iter = commands->begin();
   for (; iter != commands->end(); ++iter) {
     if (!chrome::IsChromeAccelerator(

chrome/browser/extensions/api/commands/command_service.h

@@ -112,6 +112,12 @@ class CommandService : public ProfileKeyedAPI,
                          std::string command_name,
                          bool allow_overrides);
 
+  // Removes all keybindings for a given extension by its |extension_id|.
+  // |command_name| is optional and if specified, causes only the command with
+  // the name |command_name| to be removed.
+  void RemoveKeybindingPrefs(const std::string& extension_id,
+                             const std::string& command_name);
+
   // Update the keybinding prefs (for a command with a matching |extension_id|
   // and |command_name|) to |keystroke|. If the command had another key assigned
   // that key assignment will be removed.
@@ -154,12 +160,6 @@ class CommandService : public ProfileKeyedAPI,
   // keybinding assignment.
   void AssignInitialKeybindings(const extensions::Extension* extension);
 
-  // Removes all keybindings for a given extension by its |extension_id|.
-  // |command_name| is optional and if specified, causes only the command with
-  // the name |command_name| to be removed.
-  void RemoveKeybindingPrefs(const std::string& extension_id,
-                             const std::string& command_name);
-
   bool GetExtensionActionCommand(const std::string& extension_id,
                                  QueryType query_type,
                                  extensions::Command* command,

chrome/browser/extensions/api/commands/command_service_browsertest.cc

+// Copyright (c) 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.
+
+#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/extensions/extension_manifest_constants.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/test_utils.h"
+
+namespace extensions {
+
+typedef ExtensionApiTest CommandServiceTest;
+
+IN_PROC_BROWSER_TEST_F(CommandServiceTest, RemoveShortcutSurvivesUpdate) {
+  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"),
+      pem_path,
+      base::FilePath());
+  base::FilePath path_v2 = PackExtensionWithOptions(
+      test_data_dir_.AppendASCII("keybinding").AppendASCII("update")
+                    .AppendASCII("v2"),
+      scoped_temp_dir.path().AppendASCII("v2.crx"),
+      pem_path,
+      base::FilePath());
+
+  ExtensionService* service = ExtensionSystem::Get(browser()->profile())->
+      extension_service();
+  CommandService* command_service = CommandService::Get(browser()->profile());
+
+  const char kId[] = "pgoakhfeplldmjheffidklpoklkppipp";
+
+  // Install v1 of the extension.
+  ASSERT_TRUE(InstallExtension(path_v1, 1));
+  EXPECT_TRUE(service->GetExtensionById(kId, false) != NULL);
+
+  // Verify it has a command of Alt+Shift+F.
+  ui::Accelerator accelerator =
+      command_service->FindShortcutForCommand(
+          kId, extension_manifest_values::kBrowserActionCommandEvent);
+  EXPECT_EQ(ui::VKEY_F, accelerator.key_code());
+  EXPECT_FALSE(accelerator.IsCtrlDown());
+  EXPECT_TRUE(accelerator.IsShiftDown());
+  EXPECT_TRUE(accelerator.IsAltDown());
+
+  // Remove the keybinding.
+  command_service->RemoveKeybindingPrefs(
+      kId, extension_manifest_values::kBrowserActionCommandEvent);
+
+  // Verify it got removed.
+  accelerator = command_service->FindShortcutForCommand(
+      kId, extension_manifest_values::kBrowserActionCommandEvent);
+  EXPECT_EQ(ui::VKEY_UNKNOWN, accelerator.key_code());
+
+  // Update to version 2.
+  EXPECT_TRUE(UpdateExtension(kId, path_v2, 0));
+  EXPECT_TRUE(service->GetExtensionById(kId, false) != NULL);
+
+  // Verify it is still set to nothing.
+  accelerator = command_service->FindShortcutForCommand(
+      kId, extension_manifest_values::kBrowserActionCommandEvent);
+  EXPECT_EQ(ui::VKEY_UNKNOWN, accelerator.key_code());
+}
+
+}  // namespace extensions

chrome/browser/extensions/extension_prefs.cc

@@ -184,7 +184,7 @@ const char kPrefFromWebStore[] = "from_webstore";
 // mock App created from a bookmark.
 const char kPrefFromBookmark[] = "from_bookmark";
 
-// A prefrence that indicates whethere the extension was installed as
+// A preference that indicates whether the extension was installed as
 // default apps.
 const char kPrefWasInstalledByDefault[] = "was_installed_by_default";
 

chrome/browser/ui/views/browser_action_view.cc

@@ -407,5 +407,6 @@ void BrowserActionButton::MaybeUnregisterExtensionCommand(bool only_if_active) {
           &browser_action_command,
           NULL)) {
     GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
+    keybinding_.reset(NULL);
   }
 }

chrome/chrome_tests.gypi

@@ -1267,6 +1267,7 @@
         'browser/extensions/api/browsing_data/browsing_data_test.cc',
         'browser/extensions/api/cloud_print_private/cloud_print_private_apitest.cc',
         'browser/extensions/api/command_line_private/command_line_private_apitest.cc',
+        'browser/extensions/api/commands/command_service_browsertest.cc',
         'browser/extensions/api/content_settings/content_settings_apitest.cc',
         'browser/extensions/api/context_menus/context_menu_apitest.cc',
         'browser/extensions/api/cookies/cookies_apitest.cc',

chrome/test/data/extensions/api_test/keybinding/keybinding.pem

+-----BEGIN PRIVATE KEY-----
+MIICdwIBADANBgkqhkiG9w0BAQEFAASCAmEwggJdAgEAAoGBANbGM/QruozVbqkhP
+RkQSESpcRwmnfwjRiO6NYWJaM3sFiTaf8ZHJdf1SaewZSQ1g5JnpOmleoG5l8OHXf
+8wp6PSMJEuymkhtOmo6XKyjYGSbVq52IuyUpRVTYlni6z72SaScZ0CTQ6aqmZ/Oyu
+SnSHAK3Hi4dhYzf3+uUHHMEA9AgMBAAECgYEAitvy0zdCkbOcrHT1D7NbRilXHCBL
+nK0huA+4cvH4dMrjNkievA9cBFhumqCNg++ldY7VLMr0fdMEsEJhH5DRkQXwapaBn
+QFSd5WF2keXNwSvmCS9ZDfVY+eG6HRTkPpDiL4koRWDI1S/UwmbF0sOQEWjEYPUIf
+FlkE/jEKeUNvECQQDz0Bby7KFLHZKBpmrqPcSmsS3GOHTXGcyz+THCeCA73WOwMzH
+w7TPYLcn1ysl2bRw5MR6bBYNMt+DPbkBfkWmPAkEA4YKGhGPWXnXvseYPv7LzlkPe
+JDEJXSIjEnxvdC/E1Ivf37jgaM8W9c0c24VnLBR/4ozat/wymaKJnOTGtihbcwJAD
+R4W/f7pNqdiqIMRoYStPeKuecmzz5bdwpkXqkTYRyBEKsi2WSYJ8gmUohrE+BAqwp
+D3+pMzWz9RYxelcv7Z1QJBAJq7nOEqP+UAtz8xxIyclVb9qmba3rnwum/swezO+hd
+9AlfX2uMPdYmi+7IEjw5H4pfoXgrwGSghxprIvzAt8NECQHTbLnPnXDZ5mzhc20Le
+K90OWE9lYrI7FH9uCq9NIG9F20b9htXUarEZjaq8/8FaS4smXC7tp6E2VXQMYVwFg
+m8=
+-----END PRIVATE KEY-----

chrome/test/data/extensions/api_test/keybinding/update/v1/background.js

+// 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.
+
+// Called when the user clicks on the browser action.
+chrome.browserAction.onClicked.addListener(function(windowId) {
+  chrome.tabs.executeScript(null, { code: "document.body.bgColor='red'" });
+});
+
+chrome.test.notifyPass();

chrome/test/data/extensions/api_test/keybinding/update/v1/manifest.json

+{
+  "name": "A browser action simply for updating",
+  "version": "1.0",
+  "manifest_version": 2,
+  "background": {
+    "scripts": ["background.js"]
+  },
+  "permissions": ["activeTab"],
+  "browser_action": {
+    "default_title": "Make this page red"
+  },
+  "commands": {
+    "_execute_browser_action": {
+      "suggested_key": {
+        "default": "Alt+Shift+F"
+      }
+    }
+  }
+}

chrome/test/data/extensions/api_test/keybinding/update/v2/background.js

+// 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.
+
+// Called when the user clicks on the browser action.
+chrome.browserAction.onClicked.addListener(function(windowId) {
+  chrome.tabs.executeScript(null, { code: "document.body.bgColor='red'" });
+});
+
+chrome.test.notifyPass();

chrome/test/data/extensions/api_test/keybinding/update/v2/manifest.json

+{
+  "name": "A browser action simply for updating",
+  "version": "2.0",
+  "manifest_version": 2,
+  "background": {
+    "scripts": ["background.js"]
+  },
+  "permissions": ["activeTab"],
+  "browser_action": {
+    "default_title": "Make this page red"
+  },
+  "commands": {
+    "_execute_browser_action": {
+      "suggested_key": {
+        "default": "Alt+Shift+F"
+      }
+    }
+  }
+}