Enable commands API for platform apps.

chrome/browser/extensions/extension_keybinding_registry.cc

Index: chrome/browser/extensions/extension_keybinding_registry.cc
diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc b/chrome/browser/extensions/extension_keybinding_registry.cc
index 1d6001e280b12cde9da61c881be6af3164d68353..1603851127ece69e03c682b63afd3ad7e05cebe4 100644
--- a/chrome/browser/extensions/extension_keybinding_registry.cc
+++ b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -13,8 +13,9 @@
 
 namespace extensions {
 
-ExtensionKeybindingRegistry::ExtensionKeybindingRegistry(Profile* profile)
-    : profile_(profile) {
+ExtensionKeybindingRegistry::ExtensionKeybindingRegistry(
+    Profile* profile, ExtensionFilter extension_filter)
+    : profile_(profile), extension_filter_(extension_filter) {
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
                  content::Source<Profile>(profile->GetOriginalProfile()));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
@@ -36,7 +37,8 @@ void ExtensionKeybindingRegistry::Init() {
   const ExtensionSet* extensions = service->extensions();
   ExtensionSet::const_iterator iter = extensions->begin();
   for (; iter != extensions->end(); ++iter)
-    AddExtensionKeybinding(*iter, std::string());
+    if (ExtensionMatchesFilter(*iter))
+      AddExtensionKeybinding(*iter, std::string());

[Finnur, 2012/09/11 10:29:42]

nit: This is now multiline and needs braces.

 }
 
 bool ExtensionKeybindingRegistry::ShouldIgnoreCommand(
@@ -51,16 +53,20 @@ void ExtensionKeybindingRegistry::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   switch (type) {
-    case chrome::NOTIFICATION_EXTENSION_LOADED:
-      AddExtensionKeybinding(
-          content::Details<const extensions::Extension>(details).ptr(),
-          std::string());
+    case chrome::NOTIFICATION_EXTENSION_LOADED: {
+      const extensions::Extension* extension =
+          content::Details<const extensions::Extension>(details).ptr();
+      if (ExtensionMatchesFilter(extension))
+        AddExtensionKeybinding(extension, std::string());
       break;
-    case chrome::NOTIFICATION_EXTENSION_UNLOADED:
-      RemoveExtensionKeybinding(
-          content::Details<UnloadedExtensionInfo>(details)->extension,
-          std::string());
+    }
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
+      const extensions::Extension* extension =
+          content::Details<UnloadedExtensionInfo>(details)->extension;
+      if (ExtensionMatchesFilter(extension))
+        RemoveExtensionKeybinding(extension, std::string());
       break;
+    }
     case chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED:
     case chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED: {
       std::pair<const std::string, const std::string>* payload =
@@ -75,10 +81,12 @@ void ExtensionKeybindingRegistry::Observe(
       if (!extension)
         return;
 
-      if (type == chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED)
-        AddExtensionKeybinding(extension, payload->second);
-      else
-        RemoveExtensionKeybinding(extension, payload->second);
+      if (ExtensionMatchesFilter(extension)) {
+        if (type == chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED)
+          AddExtensionKeybinding(extension, payload->second);
+        else
+          RemoveExtensionKeybinding(extension, payload->second);
+      }

[Finnur, 2012/09/11 10:29:42]

I'm a little worried about the fact that this class was originally intended to
be completely in sync with other such classes (on the same platform). You'd
always get the same answer no matter what window was checking the registry, but
now the registry contains different shortcut assignments based on which type of
window it belongs to.

Now it seems that platform-app windows only know about platform-app shortcuts,
but other windows know about all shortcuts. Can that lead to problems?

What happens (or should happen) when you hit a platform-app-specific keyboard
shortcut in a non-platform-app window?

And similar for the reverse, should a keyboard shortcut for a regular extension
(non-platform app) be ignored when struck while a platform-app window has focus?

Also (thinking out loud here), lets assume you have an extension (not a
platform-app) register Ctrl+Y and then you install a platform-app that also
wants Ctrl+Y. I think AssignInitialKeybinding will do the right thing and not
override the Pref keybinding for the non-platform-app extension. Correct? Then,
the platform-app shortcut becomes inactive at install time, because another one
has taken Ctrl+Y? That's expected, right?

And finally, how should this work in the config UI? I assume platform apps can
only have named commands specified (not browser/page actions) and that those
just get listed and you can assign a shortcut from a platform app to a
non-platform app and vice versa, without problems. Correct?

       break;
     }
     default:
@@ -87,4 +95,18 @@ void ExtensionKeybindingRegistry::Observe(
   }
 }
 
+bool ExtensionKeybindingRegistry::ExtensionMatchesFilter(
+    const extensions::Extension* extension)
+{

[Finnur, 2012/09/11 10:29:42]

nit: Should be on the line above.

+  switch (extension_filter_) {
+    case ALL_EXTENSIONS:
+      return true;
+    case PLATFORM_APPS_ONLY:
+      return extension->is_platform_app();
+    default:
+      NOTREACHED();

[Finnur, 2012/09/11 10:29:42]

For enum types it is better to leave out the default clause because then you'll
get a compile error on non-Windows platforms when a new enum value is added and
they forget to update this switch statement to account for it.

+  }
+  return false;
+}
+
 }  // namespace extensions