Conflict detection for Extension Keybinding.

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
===================================================================
--- chrome/browser/extensions/extension_service.cc	(revision 133458)
+++ chrome/browser/extensions/extension_service.cc	(working copy)
@@ -50,6 +50,7 @@
 #include "chrome/browser/extensions/extension_global_error.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_input_ime_api.h"
+#include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/extensions/extension_management_api.h"
 #include "chrome/browser/extensions/extension_preference_api.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
@@ -809,6 +810,9 @@
   // any of these resources.
   UnloadExtension(extension_id, extension_misc::UNLOAD_REASON_UNINSTALL);
 
+  ExtensionKeybindingRegistry::RemoveKeybindingPref(profile_->GetPrefs(),

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

Please avoid adding more glue code here. Instead, make KeybindingRegistry listen
to EXTENSION_INSATLLED and EXTENSION_UNINSTALLED and do this work itself.

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

I thought about that. The problem is that the ExtensionKeybindingRegistry class
is a base class for common Keybinding functionality across platforms. On Views,
for example, the ExtensionKeybindingRegistryViews is tied to a window because it
needs to communicate with the FocusManager for that window.

Long story short: There can be multiple ExtensionKeybindingRegistry objects
around (if you have multiple windows), which would then all listen for that
uninstall event and all try to remove the prefs. Not a big problem, but seemed
less wasteful to do this once here with a static function than to do it multiple
times from each observer.

I can make that change though, if you feel strongly about it. I also toyed with
the idea of adding this to the extension_prefs_->OnExtensionUninstalled() call
on the line below (since this is after all dealing with Extension Prefs). Is
that more acceptable?

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

[Aaron Boodman, 2012/04/26 19:29:30]

I see. In that case, I suggest a small KeybindingGlue object (needs a better
name!) that has Profile lifetime and which is owned by ExtensionSystem. This
object would listen to EXTENSION_(UN)INSTALLED and do the appropriate
registrations.

It also has the advantage of being easily unit-testable.

+                                                    extension_id);
+
   extension_prefs_->OnExtensionUninstalled(extension_id, extension->location(),
                                            external_uninstall);
 
@@ -2258,6 +2262,9 @@
     extension_prefs_->SetAllowFileAccess(id, true);
   }
 
+  ExtensionKeybindingRegistry::ResolveKeyBindings(profile_->GetPrefs(),
+                                                  extension);

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

Same question here. Move it to OnExtensionInstalled (see line 2252)?

+
   // Transfer ownership of |extension| to AddExtension.
   if (AddExtension(scoped_extension)) {
     content::NotificationService::current()->Notify(