Conflict detection for Extension Keybinding.

chrome/browser/extensions/extension_keybinding_registry.cc

Index: chrome/browser/extensions/extension_keybinding_registry.cc
===================================================================
--- chrome/browser/extensions/extension_keybinding_registry.cc	(revision 133458)
+++ chrome/browser/extensions/extension_keybinding_registry.cc	(working copy)
@@ -4,12 +4,23 @@
 
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/prefs/pref_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/extensions/extension_manifest_constants.h"
 #include "chrome/common/extensions/extension_set.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/browser/extensions/extension_service.h"
 
+namespace {
+
+const char kExtension[] = "extension";
+const char kCommandName[] = "command_name";
+
+}  // namespace
+
 ExtensionKeybindingRegistry::ExtensionKeybindingRegistry(Profile* profile)
     : profile_(profile) {
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
@@ -38,6 +49,189 @@
          command == extension_manifest_values::kBrowserActionKeybindingEvent;
 }
 
+// static
+void ExtensionKeybindingRegistry::RegisterUserPrefs(
+    PrefService* user_prefs) {
+  user_prefs->RegisterDictionaryPref(prefs::kExtensionKeybindings,
+                                     PrefService::UNSYNCABLE_PREF);

[Aaron Boodman, 2012/04/25 19:14:38]

This seems like something we'd want to sync.

[Finnur, 2012/04/26 13:12:35]

Yes. I've now changed this to SYNCABLE_PREF. I was initially planning on
delaying it a bit because we need to sync this value with care so as to not
clobber keybindings on one platform with the keybindings with another. However,
I've made it so that the keybinding key we store it under takes platform into
account, so that should no longer be an issue.

On 2012/04/25 19:14:38, Aaron Boodman wrote:

+}
+
+// static
+const Extension::ExtensionKeybinding*
+    ExtensionKeybindingRegistry::GetActiveBrowserActionCommand(
+        Profile* profile,
+        const std::string& extension_id) {
+  const Extension::ExtensionKeybinding* command =

[Aaron Boodman, 2012/04/25 19:14:38]

In a separate change, uou should move ExtensionKeybindingRegistry to
extensions::KeybindingRegistry. Sorry for not noticing this earlier.

[Finnur, 2012/04/26 13:12:35]

Yup. Sounds good.

On 2012/04/25 19:14:38, Aaron Boodman wrote:

+      profile->GetExtensionService()->GetExtensionById(

[Aaron Boodman, 2012/04/25 19:14:38]

If there is no such extension_id, we crash. Is that OK? Do we know for sure at
the times this is called that the extension ID is legit?

If the assumption is that only enabled extension_id's are passed, store the
returned const Extension* and CHECK that it is non-null.

Otherwise when someone inevitably calls with with a bad extension ID we will go
off into lala land for a little while before crashing because
extension->browser_action_command() and command->accelerator() are both
non-virtual.

[Finnur, 2012/04/26 13:12:35]

Callers are always holding what I assume to be valid Extension pointers when
they call these functions. If they are not then we have a problem (elsewhere) I
think. So, I've added a CHECK.

On 2012/04/25 19:14:38, Aaron Boodman wrote:

+          extension_id, false)->browser_action_command();
+  if (!command)
+    return NULL;
+  return ExtensionKeybindingRegistry::KeybindingActive(

[Aaron Boodman, 2012/04/25 19:14:38]

Nit: Since this function call is so long, I would prefer to see it as if/else
rather than a ternary operator.

[Aaron Boodman, 2012/04/25 19:14:38]

Do not need the "ExtensionKeybindingRegistry::" qualifier here. Same in other
places in this file.

[Finnur, 2012/04/26 13:12:35]

Both done.

On 2012/04/25 19:14:38, Aaron Boodman wrote:

+      profile->GetPrefs(),
+      command->accelerator(),
+      extension_id,
+      command->command_name()) ? command : NULL;
+}
+
+// static
+const Extension::ExtensionKeybinding*
+    ExtensionKeybindingRegistry::GetActivePageActionCommand(
+        Profile* profile,
+        const std::string& extension_id) {
+  const Extension::ExtensionKeybinding* command =
+      profile->GetExtensionService()->GetExtensionById(
+          extension_id, false)->page_action_command();
+  if (!command)
+    return NULL;
+  return ExtensionKeybindingRegistry::KeybindingActive(
+      profile->GetPrefs(),
+      command->accelerator(),
+      extension_id,
+      command->command_name()) ? command : NULL;
+}
+
+// static
+Extension::CommandMap ExtensionKeybindingRegistry::GetActiveNamedCommands(
+    Profile* profile,
+    const std::string& extension_id) {
+  Extension::CommandMap result;
+  const Extension::CommandMap& commands =
+      profile->GetExtensionService()->GetExtensionById(
+          extension_id, false)->named_commands();
+  if (commands.empty())
+    return result;
+
+  Extension::CommandMap::const_iterator iter = commands.begin();
+  for (; iter != commands.end(); ++iter) {
+    if (!ExtensionKeybindingRegistry::KeybindingActive(
+        profile->GetPrefs(),
+        iter->second.accelerator(),
+        extension_id,
+        iter->second.command_name())) {
+      continue;
+    }
+
+    result[iter->second.command_name()] = iter->second;
+  }
+
+  return result;
+}
+
+// static
+void ExtensionKeybindingRegistry::ResolveKeyBindings(
+    PrefService* user_prefs,
+    const Extension* extension) {
+  const Extension::CommandMap& commands = extension->named_commands();
+  Extension::CommandMap::const_iterator iter = commands.begin();
+  for (; iter != commands.end(); ++iter) {
+    AddKeybindingPref(user_prefs,
+                      iter->second.accelerator(),
+                      extension->id(),
+                      iter->second.command_name(),
+                      false);  // Overwriting not allowed.
+  }
+
+  const Extension::ExtensionKeybinding* browser_action_command =
+      extension->browser_action_command();
+  if (browser_action_command) {
+    AddKeybindingPref(user_prefs,
+                      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(user_prefs,
+                      page_action_command->accelerator(),
+                      extension->id(),
+                      page_action_command->command_name(),
+                      false);  // Overwriting not allowed.
+  }
+}
+
+// static
+bool ExtensionKeybindingRegistry::KeybindingActive(
+    PrefService* user_prefs,
+    const ui::Accelerator& accelerator,
+    std::string extension_id,
+    std::string command_name) {
+  DCHECK(!extension_id.empty());

[Aaron Boodman, 2012/04/25 19:14:38]

I prefer CHECK to DCHECK

+  DCHECK(!command_name.empty());
+
+  std::string key = UTF16ToUTF8(accelerator.GetShortcutText());
+  const DictionaryValue* bindings =
+      user_prefs->GetDictionary(prefs::kExtensionKeybindings);
+  if (!bindings->HasKey(key))
+    return false;
+
+  DictionaryValue* value;

[Aaron Boodman, 2012/04/25 19:14:38]

= NULL

+  if (!bindings->GetDictionary(key, &value) || !value)
+    return false;
+
+  std::string id;
+  if (!value->GetString(kExtension, &id) || id != extension_id)
+    return false;  // Already taken by another 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.
+}
+
+// static
+bool ExtensionKeybindingRegistry::AddKeybindingPref(
+    PrefService* user_prefs,
+    const ui::Accelerator& accelerator,
+    std::string extension_id,
+    std::string command_name,
+    bool allow_overrides) {
+  DictionaryPrefUpdate updater(user_prefs, prefs::kExtensionKeybindings);
+  DictionaryValue* bindings = updater.Get();
+
+  if (bindings->HasKey(UTF16ToUTF8(accelerator.GetShortcutText())) &&
+      !allow_overrides)
+    return false;  // Already taken.
+
+  DictionaryValue* keybinding = new DictionaryValue();
+  keybinding->SetString(kExtension, extension_id);
+  keybinding->SetString(kCommandName, command_name);
+
+  bindings->Set(UTF16ToUTF8(accelerator.GetShortcutText()), keybinding);
+  return true;
+}
+
+// static
+void ExtensionKeybindingRegistry::RemoveKeybindingPref(

[Aaron Boodman, 2012/04/25 19:14:38]

RemoveKeybindingPrefs (plural) since this can remove multiple keybindings?

+    PrefService* user_prefs,
+    std::string extension_id) {
+  DictionaryPrefUpdate updater(user_prefs, 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);
+  }
+}
+
+
 void ExtensionKeybindingRegistry::Observe(
     int type,
     const content::NotificationSource& source,