Conflict detection for Extension Keybinding.

chrome/browser/extensions/extension_command_service.cc

Index: chrome/browser/extensions/extension_command_service.cc
===================================================================
--- chrome/browser/extensions/extension_command_service.cc	(revision 0)
+++ chrome/browser/extensions/extension_command_service.cc	(revision 0)
@@ -0,0 +1,230 @@
+// 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 "chrome/browser/extensions/extension_command_service.h"
+
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/extensions/extension_keybinding_registry.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/prefs/scoped_user_pref_update.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/pref_names.h"
+
+namespace {
+
+const char kExtension[] = "extension";
+const char kCommandName[] = "command_name";
+
+std::string GetPlatformKeybindingKeyForAccelerator(
+    const ui::Accelerator& accelerator) {
+  return Extension::ExtensionKeybinding::KeybindingPlatform() + ":" +
+         UTF16ToUTF8(accelerator.GetShortcutText());
+}
+
+}  // namespace
+
+// static
+void ExtensionCommandService::RegisterUserPrefs(
+  PrefService* user_prefs) {
+      user_prefs->RegisterDictionaryPref(prefs::kExtensionKeybindings,
+                                         PrefService::SYNCABLE_PREF);
+}
+
+ExtensionCommandService::ExtensionCommandService(
+    Profile* profile)
+    : profile_(profile) {
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
+      content::Source<Profile>(profile->GetOriginalProfile()));

[Yoyo Zhou, 2012/05/01 01:51:30]

Since it's redirected in incognito, the profile you get built with can only be
the original profile.

+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
+      content::Source<Profile>(profile->GetOriginalProfile()));
+}
+
+ExtensionCommandService::~ExtensionCommandService() {
+}
+
+const Extension::ExtensionKeybinding*
+    ExtensionCommandService::GetActiveBrowserActionCommand(
+        const std::string& extension_id) {
+  const Extension* extension =
+      profile_->GetExtensionService()->GetExtensionById(extension_id, false);

[Yoyo Zhou, 2012/05/01 01:51:30]

Nit: eventually we'd like to get rid of GetExtensionService. You can rewrite
this as
ExtensionSystem::Get(profile_)->extensions()->GetByID(extension_id)

[Finnur, 2012/05/02 13:50:45]

extensions() isn't defined in ExtensionSystem. We can do:

ExtensionSystem::Get(profile_)->extensions_service()->extensions()->GetByID(extension_id)

... but I don't see what that buys here...

On 2012/05/01 01:51:30, Yoyo Zhou wrote:

[Yoyo Zhou, 2012/05/02 19:12:49]

Eventually we'll want to deprecate profile_->GetExtensionService, so just
changing that part to
ExtensionSystem::Get(profile_)->extensions_service() is fine.

[Finnur, 2012/05/03 10:22:18]

I see.

Done!

+  CHECK(extension);
+  const Extension::ExtensionKeybinding* command =
+      extension->browser_action_command();
+  if (!command)
+    return NULL;
+  if (!IsKeybindingActive(command->accelerator(),
+                          extension_id,
+                          command->command_name())) {
+    return NULL;
+  }
+
+  return command;
+}
+
+const Extension::ExtensionKeybinding*
+    ExtensionCommandService::GetActivePageActionCommand(
+        const std::string& extension_id) {
+  const Extension* extension =
+      profile_->GetExtensionService()->GetExtensionById(extension_id, false);
+  CHECK(extension);
+  const Extension::ExtensionKeybinding* command =
+      extension->page_action_command();
+  if (!command)
+    return NULL;
+  if (!IsKeybindingActive(command->accelerator(),
+                          extension_id,
+                          command->command_name())) {
+    return NULL;
+  }
+
+  return command;
+}
+
+Extension::CommandMap ExtensionCommandService::GetActiveNamedCommands(
+    const std::string& extension_id) {
+  const Extension* extension =
+      profile_->GetExtensionService()->GetExtensionById(extension_id, false);
+  CHECK(extension);
+
+  Extension::CommandMap result;
+  const Extension::CommandMap& commands = extension->named_commands();
+  if (commands.empty())
+    return result;
+
+  Extension::CommandMap::const_iterator iter = commands.begin();
+  for (; iter != commands.end(); ++iter) {
+    if (!IsKeybindingActive(iter->second.accelerator(),
+                            extension_id,
+                            iter->second.command_name())) {
+      continue;
+    }
+
+    result[iter->second.command_name()] = iter->second;
+  }
+
+  return result;
+}
+
+bool ExtensionCommandService::IsKeybindingActive(
+    const ui::Accelerator& accelerator,
+    std::string extension_id,
+    std::string command_name) {
+  CHECK(!extension_id.empty());
+  CHECK(!command_name.empty());
+
+  std::string key = GetPlatformKeybindingKeyForAccelerator(accelerator);
+  const DictionaryValue* bindings =
+      profile_->GetPrefs()->GetDictionary(prefs::kExtensionKeybindings);
+  if (!bindings->HasKey(key))
+    return false;
+
+  DictionaryValue* value = NULL;
+  if (!bindings->GetDictionary(key, &value) || !value)

[Yoyo Zhou, 2012/05/01 01:51:30]

I don't think you need the extra null check.

+    return false;
+
+  std::string id;
+  if (!value->GetString(kExtension, &id) || id != extension_id)
+    return false;  // Already taken by another extension.

[Yoyo Zhou, 2012/05/01 01:51:30]

Comments are misleading. Actually it's "not taken by this extension", i.e. it
could also be not taken by any extension.

+
+  std::string command;
+  if (!value->GetString(kCommandName, &command) || command != command_name)
+    return false;  // Already taken by another command.
+
+  return true;  // We found a match, this one is active.
+}
+
+void ExtensionCommandService::AssignInitialKeybindings(
+    const Extension* extension) {
+  const Extension::CommandMap& commands = extension->named_commands();
+  Extension::CommandMap::const_iterator iter = commands.begin();
+  for (; iter != commands.end(); ++iter) {
+    AddKeybindingPref(iter->second.accelerator(),
+                      extension->id(),
+                      iter->second.command_name(),

[Yoyo Zhou, 2012/05/01 01:51:30]

(sorry since I haven't seen the previous keybinding CLs, but) Is it detected if
an extension uses duplicate command names?

[Finnur, 2012/05/02 13:50:45]

No, but it cannot happen because command names are keys in the manifest json
file.

On 2012/05/01 01:51:30, Yoyo Zhou wrote:

+                      false);  // Overwriting not allowed.
+  }
+
+  const Extension::ExtensionKeybinding* browser_action_command =
+      extension->browser_action_command();
+  if (browser_action_command) {
+    AddKeybindingPref(browser_action_command->accelerator(),
+                      extension->id(),
+                      browser_action_command->command_name(),
+                      false);  // Overwriting not allowed.
+  }
+
+  const Extension::ExtensionKeybinding* page_action_command =
+      extension->page_action_command();
+  if (page_action_command) {
+    AddKeybindingPref(page_action_command->accelerator(),
+                      extension->id(),
+                      page_action_command->command_name(),
+                      false);  // Overwriting not allowed.
+  }
+}
+
+bool ExtensionCommandService::AddKeybindingPref(
+    const ui::Accelerator& accelerator,
+    std::string extension_id,
+    std::string command_name,
+    bool allow_overrides) {
+  DictionaryPrefUpdate updater(profile_->GetPrefs(),
+                               prefs::kExtensionKeybindings);
+  DictionaryValue* bindings = updater.Get();
+
+  std::string key = GetPlatformKeybindingKeyForAccelerator(accelerator);
+  if (bindings->HasKey(key) && !allow_overrides)
+    return false;  // Already taken.
+
+  DictionaryValue* keybinding = new DictionaryValue();
+  keybinding->SetString(kExtension, extension_id);
+  keybinding->SetString(kCommandName, command_name);
+
+  bindings->Set(key, keybinding);
+  return true;
+}
+
+void ExtensionCommandService::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_INSTALLED:
+      AssignInitialKeybindings(
+          content::Details<const Extension>(details).ptr());
+      break;
+    case chrome::NOTIFICATION_EXTENSION_UNINSTALLED:
+      RemoveKeybindingPrefs(*content::Details<std::string>(details).ptr());

[Finnur, 2012/04/30 09:28:37]

I made an error here earlier and discovered it when I was testing the other
changes in this CL. Don't remember how it got introduced and why it didn't
surface before, since I tested this multiple times.

+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
+void ExtensionCommandService::RemoveKeybindingPrefs(std::string extension_id) {
+  DictionaryPrefUpdate updater(profile_->GetPrefs(),
+                               prefs::kExtensionKeybindings);
+  DictionaryValue* bindings = updater.Get();
+
+  typedef std::vector<std::string> KeysToRemove;
+  KeysToRemove keys_to_remove;
+  for (DictionaryValue::key_iterator it = bindings->begin_keys();
+       it != bindings->end_keys(); ++it) {
+    std::string key = *it;
+    DictionaryValue* item = NULL;
+    bindings->GetDictionary(key, &item);
+
+    std::string extension;
+    item->GetString(kExtension, &extension);
+    if (extension == extension_id)
+      keys_to_remove.push_back(key);
+  }
+
+  for (KeysToRemove::const_iterator it = keys_to_remove.begin();
+       it != keys_to_remove.end(); ++it) {
+    bindings->Remove(*it, NULL);
+  }
+}
Property changes on: chrome\browser\extensions\extension_command_service.cc
___________________________________________________________________
Added: svn:eol-style
   + LF