Enable commands API for platform apps.

chrome/browser/extensions/extension_keybinding_registry.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_keybinding_registry.h"
 
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.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"
 
 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,
                  content::Source<Profile>(profile->GetOriginalProfile()));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_COMMAND_ADDED,
                  content::Source<Profile>(profile->GetOriginalProfile()));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_COMMAND_REMOVED,
                  content::Source<Profile>(profile->GetOriginalProfile()));
 }
 
 ExtensionKeybindingRegistry::~ExtensionKeybindingRegistry() {
 }
 
 void ExtensionKeybindingRegistry::Init() {
   ExtensionService* service = profile_->GetExtensionService();
   if (!service)
     return;  // ExtensionService can be null during testing.
 
   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(
     const std::string& command) const {
   return command == extension_manifest_values::kPageActionCommandEvent ||
          command == extension_manifest_values::kBrowserActionCommandEvent ||
          command == extension_manifest_values::kScriptBadgeCommandEvent;
 }
 
 void ExtensionKeybindingRegistry::Observe(
     int type,
     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 =
           content::Details<std::pair<const std::string, const std::string> >(
               details).ptr();
 
       const extensions::Extension* extension =
           ExtensionSystem::Get(profile_)->extension_service()->
               extensions()->GetByID(payload->first);
       // During install and uninstall the extension won't be found. We'll catch
       // those events above, with the LOADED/UNLOADED, so we ignore this event.
       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:
       NOTREACHED();
       break;
   }
 }
 
+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