Conflict detection for Extension Keybinding.

chrome/browser/extensions/extension_service.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 "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/basictypes.h"
@@ skipping 32 matching lines @@
 #include "chrome/browser/extensions/default_apps_trial.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extension_cookies_api.h"
 #include "chrome/browser/extensions/extension_data_deleter.h"
 #include "chrome/browser/extensions/extension_disabled_ui.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extension_font_settings_api.h"
 #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"
 #include "chrome/browser/extensions/extension_processes_api.h"
 #include "chrome/browser/extensions/extension_sorting.h"
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
 #include "chrome/browser/extensions/external_extension_provider_impl.h"
@@ skipping 739 matching lines @@
 
   TemplateURLService* url_service =
       TemplateURLServiceFactory::GetForProfile(profile_);
   if (url_service)
     url_service->UnregisterExtensionKeyword(extension);
 
   // Unload before doing more cleanup to ensure that nothing is hanging on to
   // 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);
 
   // Tell the backend to start deleting installed extensions on the file thread.
   if (Extension::LOAD != extension->location()) {
     if (!BrowserThread::PostTask(
             BrowserThread::FILE, FROM_HERE,
             base::Bind(
                 &extension_file_util::UninstallExtension,
                 install_directory_,
@@ skipping 1429 matching lines @@
       from_webstore,
       page_ordinal);
 
   // Unpacked extensions default to allowing file access, but if that has been
   // overridden, don't reset the value.
   if (Extension::ShouldAlwaysAllowFileAccess(extension->location()) &&
       !extension_prefs_->HasAllowFileAccessSetting(id)) {
     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(
         chrome::NOTIFICATION_EXTENSION_INSTALLED,
         content::Source<Profile>(profile_),
         content::Details<const Extension>(extension));
   }
 }
 
 const Extension* ExtensionService::GetExtensionByIdInternal(
@@ skipping 374 matching lines @@
   // To coexist with certain unit tests that don't have an IO thread message
   // loop available at ExtensionService shutdown, we lazy-initialize this
   // object so that those cases neither create nor destroy an
   // APIResourceController.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!api_resource_controller_) {
     api_resource_controller_ = new extensions::APIResourceController();
   }
   return api_resource_controller_;
 }