[Extensions] Replace type-specific action getters in CommandService

Replace GetBrowserActionCommand() and GetPageActionCommand() in
CommandService with GetExtensionActionCommand(), which takes an
ActionInfo::Type to specify the type to look for.

This reduces the complexity of many callsites (like
ExtensionActionViewController and ExtensionInstalledBubbleModel) and
reduces the dependency on specific types of actions, which will make it
easier to support a generic "action".

Bug: 893373
Change-Id: I9f566669f77256092676ccdea4e6aeb7b5054bab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2145157
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758644}

chrome/browser/extensions/api/commands/command_service.cc

@@ -131,22 +131,6 @@ CommandService* CommandService::Get(content::BrowserContext* context) {
   return BrowserContextKeyedAPIFactory<CommandService>::Get(context);
 }
 
-bool CommandService::GetBrowserActionCommand(const std::string& extension_id,
-                                             QueryType type,
-                                             Command* command,
-                                             bool* active) const {
-  return GetExtensionActionCommand(extension_id, type, command, active,
-                                   ActionInfo::TYPE_BROWSER);
-}
-
-bool CommandService::GetPageActionCommand(const std::string& extension_id,
-                                          QueryType type,
-                                          Command* command,
-                                          bool* active) const {
-  return GetExtensionActionCommand(extension_id, type, command, active,
-                                   ActionInfo::TYPE_PAGE);
-}
-
 bool CommandService::GetNamedCommands(const std::string& extension_id,
                                       QueryType type,
                                       CommandScope scope,
@@ -397,13 +381,15 @@ void CommandService::RemoveRelinquishedKeybindings(const Extension* extension) {
     }
   }
 
+  // TODO(https://crbug.com/1067130): Extensions shouldn't be able to specify
+  // commands for actions they don't have, so we should just be able to query
+  // for a single action type.
   Command existing_browser_action_command;
   const Command* new_browser_action_command =
       CommandsInfo::GetBrowserActionCommand(extension);
-  if (GetBrowserActionCommand(extension->id(),
-                              CommandService::ACTIVE,
-                              &existing_browser_action_command,
-                              NULL) &&
+  if (GetExtensionActionCommand(extension->id(), ActionInfo::TYPE_BROWSER,
+                                CommandService::ACTIVE,
+                                &existing_browser_action_command, nullptr) &&
       // The browser action command may be defaulted to an unassigned
       // accelerator if a browser action is specified by the extension but a
       // keybinding is not declared. See
@@ -412,21 +398,18 @@ void CommandService::RemoveRelinquishedKeybindings(const Extension* extension) {
        new_browser_action_command->accelerator().key_code() ==
            ui::VKEY_UNKNOWN) &&
       !IsCommandShortcutUserModified(
-          extension,
-          existing_browser_action_command.command_name())) {
+          extension, existing_browser_action_command.command_name())) {
     RemoveKeybindingPrefs(extension->id(),
                           existing_browser_action_command.command_name());
   }
 
   Command existing_page_action_command;
-  if (GetPageActionCommand(extension->id(),
-                           CommandService::ACTIVE,
-                           &existing_page_action_command,
-                           NULL) &&
+  if (GetExtensionActionCommand(extension->id(), ActionInfo::TYPE_PAGE,
+                                CommandService::ACTIVE,
+                                &existing_page_action_command, nullptr) &&
       !CommandsInfo::GetPageActionCommand(extension) &&
       !IsCommandShortcutUserModified(
-          extension,
-          existing_page_action_command.command_name())) {
+          extension, existing_page_action_command.command_name())) {
     RemoveKeybindingPrefs(extension->id(),
                           existing_page_action_command.command_name());
   }
@@ -572,6 +555,7 @@ void CommandService::RemoveDefunctExtensionSuggestedCommandPrefs(
         current_prefs->DeepCopy());
     const CommandMap* named_commands =
         CommandsInfo::GetNamedCommands(extension);
+
     const Command* browser_action_command =
         CommandsInfo::GetBrowserActionCommand(extension);
     for (base::DictionaryValue::Iterator it(*current_prefs);
@@ -685,12 +669,11 @@ void CommandService::RemoveKeybindingPrefs(const std::string& extension_id,
   }
 }
 
-bool CommandService::GetExtensionActionCommand(
-    const std::string& extension_id,
-    QueryType query_type,
-    Command* command,
-    bool* active,
-    ActionInfo::Type action_type) const {
+bool CommandService::GetExtensionActionCommand(const std::string& extension_id,
+                                               ActionInfo::Type action_type,
+                                               QueryType query_type,
+                                               Command* command,
+                                               bool* active) const {
   const ExtensionSet& extensions =
       ExtensionRegistry::Get(profile_)->enabled_extensions();
   const Extension* extension = extensions.GetByID(extension_id);
@@ -709,7 +692,6 @@ bool CommandService::GetExtensionActionCommand(
       break;
     case ActionInfo::TYPE_ACTION:
       // TODO(devlin): Add support for the "action" key.
-      NOTREACHED();
       return false;
   }
   if (!requested_command)

chrome/browser/extensions/api/commands/command_service.h

@@ -95,27 +95,16 @@ class CommandService : public BrowserContextKeyedAPI,
   // Convenience method to get the CommandService for a profile.
   static CommandService* Get(content::BrowserContext* context);
 
-  // Gets the command (if any) for the browser action of an extension given
-  // its |extension_id|. The function consults the master list to see if
-  // the command is active. Returns false if the extension has no browser
-  // action. Returns false if the command is not active and |type| requested
-  // is ACTIVE. |command| contains the command found and |active| (if not
-  // NULL) contains whether |command| is active.
-  bool GetBrowserActionCommand(const std::string& extension_id,
-                               QueryType type,
-                               Command* command,
-                               bool* active) const;
-
-  // Gets the command (if any) for the page action of an extension given
-  // its |extension_id|. The function consults the master list to see if
-  // the command is active. Returns false if the extension has no page
-  // action. Returns false if the command is not active and |type| requested
-  // is ACTIVE. |command| contains the command found and |active| (if not
-  // NULL) contains whether |command| is active.
-  bool GetPageActionCommand(const std::string& extension_id,
-                            QueryType type,
-                            Command* command,
-                            bool* active) const;
+  // Gets the command (if any) for the specified |action_type| of an extension
+  // given its |extension_id|. The function consults the master list to see if
+  // the command is active. Returns false if the command is not active and
+  // |type| requested is ACTIVE. |command| contains the command found and
+  // |active| (if not null) contains whether |command| is active.
+  bool GetExtensionActionCommand(const std::string& extension_id,
+                                 ActionInfo::Type action_type,
+                                 QueryType type,
+                                 Command* command,
+                                 bool* active) const;
 
   // Gets the active named commands (if any) for the extension with
   // |extension_id|. The function consults the master list to see if the
@@ -225,12 +214,6 @@ class CommandService : public BrowserContextKeyedAPI,
   bool IsCommandShortcutUserModified(const Extension* extension,
                                      const std::string& command_name);
 
-  bool GetExtensionActionCommand(const std::string& extension_id,
-                                 QueryType query_type,
-                                 Command* command,
-                                 bool* active,
-                                 ActionInfo::Type type) const;
-
   // A weak pointer to the profile we are associated with. Not owned by us.
   Profile* profile_;
 

chrome/browser/extensions/api/commands/command_service_browsertest.cc

@@ -10,6 +10,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/pref_names.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/test/browser_test.h"
@@ -141,8 +142,9 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest,
   {
     Command command;
     bool active = false;
-    EXPECT_TRUE(command_service->GetBrowserActionCommand(
-        extension->id(), CommandService::ALL, &command, &active));
+    EXPECT_TRUE(command_service->GetExtensionActionCommand(
+        extension->id(), ActionInfo::TYPE_BROWSER, CommandService::ALL,
+        &command, &active));
 
     EXPECT_EQ(kBasicBrowserActionKeybinding,
               Command::AcceleratorToString(command.accelerator()));
@@ -156,8 +158,9 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest,
   {
     Command command;
     bool active = false;
-    EXPECT_TRUE(command_service->GetBrowserActionCommand(
-        extension->id(), CommandService::ALL, &command, &active));
+    EXPECT_TRUE(command_service->GetExtensionActionCommand(
+        extension->id(), ActionInfo::TYPE_BROWSER, CommandService::ALL,
+        &command, &active));
 
     EXPECT_EQ(kBasicAlternateKeybinding,
               Command::AcceleratorToString(command.accelerator()));
@@ -170,8 +173,9 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest,
   {
     Command command;
     bool active = true;
-    EXPECT_TRUE(command_service->GetBrowserActionCommand(
-        extension->id(), CommandService::ALL, &command, &active));
+    EXPECT_TRUE(command_service->GetExtensionActionCommand(
+        extension->id(), ActionInfo::TYPE_BROWSER, CommandService::ALL,
+        &command, &active));
 
     EXPECT_EQ(kBasicBrowserActionKeybinding,
               Command::AcceleratorToString(command.accelerator()));
@@ -191,8 +195,9 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest,
   {
     Command command;
     bool active = false;
-    EXPECT_TRUE(command_service->GetBrowserActionCommand(
-        extension->id(), CommandService::ACTIVE, &command, &active));
+    EXPECT_TRUE(command_service->GetExtensionActionCommand(
+        extension->id(), ActionInfo::TYPE_BROWSER, CommandService::ACTIVE,
+        &command, &active));
 
     EXPECT_EQ(kBasicBrowserActionKeybinding,
               Command::AcceleratorToString(command.accelerator()));
@@ -206,8 +211,9 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest,
   {
     Command command;
     bool active = false;
-    EXPECT_TRUE(command_service->GetBrowserActionCommand(
-        extension->id(), CommandService::ACTIVE, &command, &active));
+    EXPECT_TRUE(command_service->GetExtensionActionCommand(
+        extension->id(), ActionInfo::TYPE_BROWSER, CommandService::ACTIVE,
+        &command, &active));
 
     EXPECT_EQ(kBasicAlternateKeybinding,
               Command::AcceleratorToString(command.accelerator()));
@@ -220,8 +226,9 @@ IN_PROC_BROWSER_TEST_F(CommandServiceTest,
   {
     Command command;
     bool active = false;
-    EXPECT_FALSE(command_service->GetBrowserActionCommand(
-        extension->id(), CommandService::ACTIVE, &command, &active));
+    EXPECT_FALSE(command_service->GetExtensionActionCommand(
+        extension->id(), ActionInfo::TYPE_BROWSER, CommandService::ACTIVE,
+        &command, &active));
   }
 }
 

chrome/browser/extensions/api/commands/commands.cc

@@ -9,6 +9,7 @@
 
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
 
 namespace {
 
@@ -32,20 +33,21 @@ ExtensionFunction::ResponseAction GetAllCommandsFunction::Run() {
   extensions::CommandService* command_service =
       extensions::CommandService::Get(browser_context());
 
+  // TODO(https://crbug.com/1067130): We should be able to check what
+  // type of action (if any) the extension has, and just check for
+  // that one.
   extensions::Command browser_action;
   bool active = false;
-  if (command_service->GetBrowserActionCommand(extension_->id(),
-          extensions::CommandService::ALL,
-          &browser_action,
-          &active)) {
+  if (command_service->GetExtensionActionCommand(
+          extension_->id(), extensions::ActionInfo::TYPE_BROWSER,
+          extensions::CommandService::ALL, &browser_action, &active)) {
     command_list->Append(CreateCommandValue(browser_action, active));
   }
 
   extensions::Command page_action;
-  if (command_service->GetPageActionCommand(extension_->id(),
-          extensions::CommandService::ALL,
-          &page_action,
-          &active)) {
+  if (command_service->GetExtensionActionCommand(
+          extension_->id(), extensions::ActionInfo::TYPE_PAGE,
+          extensions::CommandService::ALL, &page_action, &active)) {
     command_list->Append(CreateCommandValue(page_action, active));
   }
 

chrome/browser/extensions/api/developer_private/extension_info_generator.cc

@@ -27,6 +27,7 @@
 #include "chrome/browser/extensions/shared_module_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/extensions/command.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/pref_names.h"
@@ -184,20 +185,22 @@ void ConstructCommands(CommandService* command_service,
     command_value.is_extension_action = is_extension_action;
     return command_value;
   };
+  // TODO(https://crbug.com/1067130): Extensions shouldn't be able to specify
+  // commands for actions they don't have, so we should just be able to query
+  // for a single action type.
   bool active = false;
   Command browser_action;
-  if (command_service->GetBrowserActionCommand(extension_id,
-                                               CommandService::ALL,
-                                               &browser_action,
-                                               &active)) {
+  if (command_service->GetExtensionActionCommand(
+          extension_id, ActionInfo::TYPE_BROWSER, CommandService::ALL,
+          &browser_action, &active)) {
     commands->push_back(construct_command(browser_action, active, true));
   }
 
   Command page_action;
-  if (command_service->GetPageActionCommand(extension_id,
-                                            CommandService::ALL,
-                                            &page_action,
-                                            &active)) {
+  active = false;
+  if (command_service->GetExtensionActionCommand(
+          extension_id, ActionInfo::TYPE_PAGE, CommandService::ALL,
+          &page_action, &active)) {
     commands->push_back(construct_command(page_action, active, true));
   }
 

chrome/browser/ui/extensions/extension_action_view_controller.cc

@@ -339,12 +339,9 @@ bool ExtensionActionViewController::GetExtensionCommand(
     return false;
 
   CommandService* command_service = CommandService::Get(browser_->profile());
-  if (extension_action_->action_type() == ActionInfo::TYPE_PAGE) {
-    return command_service->GetPageActionCommand(
-        extension_->id(), CommandService::ACTIVE, command, NULL);
-  }
-  return command_service->GetBrowserActionCommand(
-      extension_->id(), CommandService::ACTIVE, command, NULL);
+  return command_service->GetExtensionActionCommand(
+      extension_->id(), extension_action_->action_type(),
+      CommandService::ACTIVE, command, nullptr);
 }
 
 bool ExtensionActionViewController::CanHandleAccelerators() const {

chrome/browser/ui/extensions/extension_installed_bubble_model.cc

@@ -32,18 +32,12 @@ base::Optional<extensions::Command> CommandForExtensionAction(
   auto* service = extensions::CommandService::Get(profile);
   extensions::Command command;
 
-  if (info->type == extensions::ActionInfo::TYPE_BROWSER &&
-      service->GetBrowserActionCommand(extension->id(),
-                                       extensions::CommandService::ACTIVE,
-                                       &command, nullptr)) {
-    return command;
-  }
-  if (info->type == extensions::ActionInfo::TYPE_PAGE &&
-      service->GetPageActionCommand(extension->id(),
-                                    extensions::CommandService::ACTIVE,
-                                    &command, nullptr)) {
+  if (service->GetExtensionActionCommand(extension->id(), info->type,
+                                         extensions::CommandService::ACTIVE,
+                                         &command, nullptr)) {
     return command;
   }
+
   return base::nullopt;
 }