[Extensions] Remove Command::Type

Command::Type specified a type for a command of page action, browser
action, or "named" (i.e., any other command). This was posing a bit of a
problem for the generic "action" key, because we'd need to add a new
entry for it.

As it turns out, Command::Type was never really used. It was only ever
passed as a hint of which command to look up for a helper function, and
we can use ActionInfo::Type directly there.

Remove Command::Type completely.

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

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

@@ -136,7 +136,7 @@ bool CommandService::GetBrowserActionCommand(const std::string& extension_id,
                                              Command* command,
                                              bool* active) const {
   return GetExtensionActionCommand(extension_id, type, command, active,
-                                   Command::Type::kBrowserAction);
+                                   ActionInfo::TYPE_BROWSER);
 }
 
 bool CommandService::GetPageActionCommand(const std::string& extension_id,
@@ -144,7 +144,7 @@ bool CommandService::GetPageActionCommand(const std::string& extension_id,
                                           Command* command,
                                           bool* active) const {
   return GetExtensionActionCommand(extension_id, type, command, active,
-                                   Command::Type::kPageAction);
+                                   ActionInfo::TYPE_PAGE);
 }
 
 bool CommandService::GetNamedCommands(const std::string& extension_id,
@@ -778,7 +778,7 @@ bool CommandService::GetExtensionActionCommand(
     QueryType query_type,
     Command* command,
     bool* active,
-    Command::Type action_type) const {
+    ActionInfo::Type action_type) const {
   const ExtensionSet& extensions =
       ExtensionRegistry::Get(profile_)->enabled_extensions();
   const Extension* extension = extensions.GetByID(extension_id);
@@ -789,13 +789,14 @@ bool CommandService::GetExtensionActionCommand(
 
   const Command* requested_command = NULL;
   switch (action_type) {
-    case Command::Type::kBrowserAction:
+    case ActionInfo::TYPE_BROWSER:
       requested_command = CommandsInfo::GetBrowserActionCommand(extension);
       break;
-    case Command::Type::kPageAction:
+    case ActionInfo::TYPE_PAGE:
       requested_command = CommandsInfo::GetPageActionCommand(extension);
       break;
-    case Command::Type::kNamed:
+    case ActionInfo::TYPE_ACTION:
+      // TODO(devlin): Add support for the "action" key.
       NOTREACHED();
       return false;
   }

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

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/scoped_observer.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/extensions/command.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/extension_registry.h"
@@ -245,7 +246,7 @@ class CommandService : public BrowserContextKeyedAPI,
                                  QueryType query_type,
                                  Command* command,
                                  bool* active,
-                                 Command::Type type) const;
+                                 ActionInfo::Type type) const;
 
   // A weak pointer to the profile we are associated with. Not owned by us.
   Profile* profile_;

chrome/common/extensions/command.cc

@@ -39,16 +39,9 @@ static const int kMaxTokenSize = 4;
 static const int kMaxTokenSize = 3;
 #endif  // OS_CHROMEOS
 
-Command::Type GetCommandType(const std::string& command_name) {
-  if (command_name == values::kPageActionCommandEvent)
-    return Command::Type::kPageAction;
-  if (command_name == values::kBrowserActionCommandEvent)
-    return Command::Type::kBrowserAction;
-  return Command::Type::kNamed;
-}
-
 bool IsNamedCommand(const std::string& command_name) {
-  return GetCommandType(command_name) == Command::Type::kNamed;
+  return command_name != values::kPageActionCommandEvent &&
+         command_name != values::kBrowserActionCommandEvent;
 }
 
 bool DoesRequireModifier(const std::string& accelerator) {
@@ -273,19 +266,16 @@ std::string NormalizeShortcutSuggestion(const std::string& suggestion,
 
 }  // namespace
 
-Command::Command() : global_(false), type_(Type::kNamed) {}
+Command::Command() : global_(false) {}
 
 Command::Command(const std::string& command_name,
                  const base::string16& description,
                  const std::string& accelerator,
                  bool global)
-    : command_name_(command_name),
-      description_(description),
-      global_(global),
-      type_(GetCommandType(command_name)) {
+    : command_name_(command_name), description_(description), global_(global) {
   base::string16 error;
   accelerator_ = ParseImpl(accelerator, CommandPlatform(), 0,
-                           type_ == Type::kNamed, &error);
+                           IsNamedCommand(command_name), &error);
 }
 
 Command::Command(const Command& other) = default;
@@ -533,7 +523,6 @@ bool Command::Parse(const base::DictionaryValue* command,
       command_name_ = command_name;
       description_ = description;
       global_ = global;
-      type_ = GetCommandType(command_name);
     }
   }
   return true;

chrome/common/extensions/command.h

@@ -19,12 +19,6 @@ namespace extensions {
 
 class Command {
  public:
-  enum class Type {
-    kBrowserAction,
-    kPageAction,
-    kNamed,
-  };
-
   Command();
   Command(const std::string& command_name,
           const base::string16& description,
@@ -61,7 +55,6 @@ class Command {
   const ui::Accelerator& accelerator() const { return accelerator_; }
   const base::string16& description() const { return description_; }
   bool global() const { return global_; }
-  Type type() const { return type_; }
 
   // Setter:
   void set_accelerator(const ui::Accelerator& accelerator) {
@@ -76,7 +69,6 @@ class Command {
   ui::Accelerator accelerator_;
   base::string16 description_;
   bool global_;
-  Type type_;
 };
 
 // A mapping of command name (std::string) to a command object.

chrome/common/extensions/command_unittest.cc

@@ -9,7 +9,6 @@
 #include <memory>
 #include <utility>
 
-#include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -28,7 +27,6 @@ struct ConstCommandsTestData {
   const char* command_name;
   const char* key;
   const char* description;
-  base::Optional<Command::Type> type;
 };
 
 // Checks the |suggested_key| value parses into a command when specified as a
@@ -91,8 +89,6 @@ void CheckParse(const ConstCommandsTestData& data,
                    base::UTF16ToASCII(command.description()).c_str());
       EXPECT_STREQ(data.command_name, command.command_name().c_str());
       EXPECT_EQ(data.accelerator, command.accelerator());
-      ASSERT_TRUE(data.type) << "Parsed commands must specify an expected type";
-      EXPECT_EQ(*data.type, command.type());
     }
   }
 }
@@ -153,67 +149,43 @@ TEST(CommandTest, ExtensionCommandParsing) {
       {false, shift_f, "command", "Shift+F", "description"},
       {false, shift_f, "command", "F+Shift", "description"},
       // Basic tests.
-      {true, none, "command", "", "description", Command::Type::kNamed},
-      {true, ctrl_f, "command", "Ctrl+F", "description", Command::Type::kNamed},
-      {true, alt_f, "command", "Alt+F", "description", Command::Type::kNamed},
-      {true, ctrl_shift_f, "command", "Ctrl+Shift+F", "description",
-       Command::Type::kNamed},
-      {true, alt_shift_f, "command", "Alt+Shift+F", "description",
-       Command::Type::kNamed},
-      {true, ctrl_1, "command", "Ctrl+1", "description", Command::Type::kNamed},
+      {true, none, "command", "", "description"},
+      {true, ctrl_f, "command", "Ctrl+F", "description"},
+      {true, alt_f, "command", "Alt+F", "description"},
+      {true, ctrl_shift_f, "command", "Ctrl+Shift+F", "description"},
+      {true, alt_shift_f, "command", "Alt+Shift+F", "description"},
+      {true, ctrl_1, "command", "Ctrl+1", "description"},
       // Shortcut token order tests.
-      {true, ctrl_f, "command", "F+Ctrl", "description", Command::Type::kNamed},
-      {true, alt_f, "command", "F+Alt", "description", Command::Type::kNamed},
-      {true, ctrl_shift_f, "command", "F+Ctrl+Shift", "description",
-       Command::Type::kNamed},
-      {true, ctrl_shift_f, "command", "F+Shift+Ctrl", "description",
-       Command::Type::kNamed},
-      {true, alt_shift_f, "command", "F+Alt+Shift", "description",
-       Command::Type::kNamed},
-      {true, alt_shift_f, "command", "F+Shift+Alt", "description",
-       Command::Type::kNamed},
+      {true, ctrl_f, "command", "F+Ctrl", "description"},
+      {true, alt_f, "command", "F+Alt", "description"},
+      {true, ctrl_shift_f, "command", "F+Ctrl+Shift", "description"},
+      {true, ctrl_shift_f, "command", "F+Shift+Ctrl", "description"},
+      {true, alt_shift_f, "command", "F+Alt+Shift", "description"},
+      {true, alt_shift_f, "command", "F+Shift+Alt", "description"},
       // Case insensitivity is not OK.
       {false, ctrl_f, "command", "Ctrl+f", "description"},
       {false, ctrl_f, "command", "cTrL+F", "description"},
       // Skipping description is OK for browser- and pageActions.
-      {true, ctrl_f, "_execute_browser_action", "Ctrl+F", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_f, "_execute_page_action", "Ctrl+F", "",
-       Command::Type::kPageAction},
+      {true, ctrl_f, "_execute_browser_action", "Ctrl+F", ""},
+      {true, ctrl_f, "_execute_page_action", "Ctrl+F", ""},
       // Home, End, Arrow keys, etc.
-      {true, ctrl_comma, "_execute_browser_action", "Ctrl+Comma", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_dot, "_execute_browser_action", "Ctrl+Period", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_left, "_execute_browser_action", "Ctrl+Left", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_right, "_execute_browser_action", "Ctrl+Right", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_up, "_execute_browser_action", "Ctrl+Up", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_down, "_execute_browser_action", "Ctrl+Down", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_ins, "_execute_browser_action", "Ctrl+Insert", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_del, "_execute_browser_action", "Ctrl+Delete", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_home, "_execute_browser_action", "Ctrl+Home", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_end, "_execute_browser_action", "Ctrl+End", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_pgup, "_execute_browser_action", "Ctrl+PageUp", "",
-       Command::Type::kBrowserAction},
-      {true, ctrl_pgdwn, "_execute_browser_action", "Ctrl+PageDown", "",
-       Command::Type::kBrowserAction},
+      {true, ctrl_comma, "_execute_browser_action", "Ctrl+Comma", ""},
+      {true, ctrl_dot, "_execute_browser_action", "Ctrl+Period", ""},
+      {true, ctrl_left, "_execute_browser_action", "Ctrl+Left", ""},
+      {true, ctrl_right, "_execute_browser_action", "Ctrl+Right", ""},
+      {true, ctrl_up, "_execute_browser_action", "Ctrl+Up", ""},
+      {true, ctrl_down, "_execute_browser_action", "Ctrl+Down", ""},
+      {true, ctrl_ins, "_execute_browser_action", "Ctrl+Insert", ""},
+      {true, ctrl_del, "_execute_browser_action", "Ctrl+Delete", ""},
+      {true, ctrl_home, "_execute_browser_action", "Ctrl+Home", ""},
+      {true, ctrl_end, "_execute_browser_action", "Ctrl+End", ""},
+      {true, ctrl_pgup, "_execute_browser_action", "Ctrl+PageUp", ""},
+      {true, ctrl_pgdwn, "_execute_browser_action", "Ctrl+PageDown", ""},
       // Media keys.
-      {true, next_track, "command", "MediaNextTrack", "description",
-       Command::Type::kNamed},
-      {true, play_pause, "command", "MediaPlayPause", "description",
-       Command::Type::kNamed},
-      {true, prev_track, "command", "MediaPrevTrack", "description",
-       Command::Type::kNamed},
-      {true, stop, "command", "MediaStop", "description",
-       Command::Type::kNamed},
+      {true, next_track, "command", "MediaNextTrack", "description"},
+      {true, play_pause, "command", "MediaPlayPause", "description"},
+      {true, prev_track, "command", "MediaPrevTrack", "description"},
+      {true, stop, "command", "MediaStop", "description"},
       {false, none, "_execute_browser_action", "MediaNextTrack", ""},
       {false, none, "_execute_page_action", "MediaPrevTrack", ""},
       {false, none, "command", "Ctrl+Shift+MediaPrevTrack", "description"},
@@ -319,10 +291,8 @@ TEST(CommandTest, ExtensionCommandParsingPlatformSpecific) {
                                  ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN);
 
   ConstCommandsTestData kChromeOsTests[] = {
-      {true, search_shift_z, "command", "Search+Shift+Z", "description",
-       Command::Type::kNamed},
-      {true, search_a, "command", "Search+A", "description",
-       Command::Type::kNamed},
+      {true, search_shift_z, "command", "Search+Shift+Z", "description"},
+      {true, search_a, "command", "Search+A", "description"},
       // Command is not valid on Chrome OS.
       {false, search_shift_z, "command", "Command+Shift+Z", "description"},
   };