[Extensions] Remove action-type-specific [g|s]etters from ActionInfo

Remove [G|S]etBrowserActionInfo(), [G|S]etPageActionInfo(), and
[G|S]etExtensionActionInfo() from ActionInfo, leaving only the
type-agnostic GetAnyActionInfo() (to be renamed GetActionInfo() or
similar) and SetExtensionActionInfo(). The only real users of these
type-specific getters were tests, which have been updated.

This is another step in moving towards a generic "action" key.

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

chrome/common/extensions/api/extension_action/action_info.cc

@@ -39,13 +39,6 @@ ActionInfoData::ActionInfoData(std::unique_ptr<ActionInfo> info)
 ActionInfoData::~ActionInfoData() {
 }
 
-static const ActionInfo* GetActionInfo(const Extension* extension,
-                                       const std::string& key) {
-  ActionInfoData* data = static_cast<ActionInfoData*>(
-      extension->GetManifestData(key));
-  return data ? data->action_info.get() : NULL;
-}
-
 }  // namespace
 
 ActionInfo::ActionInfo(Type type) : type(type), synthesized(false) {
@@ -152,52 +145,19 @@ std::unique_ptr<ActionInfo> ActionInfo::Load(const Extension* extension,
 
 // static
 const ActionInfo* ActionInfo::GetAnyActionInfo(const Extension* extension) {
-  // TODO(devlin): Since all actions are mutually exclusive, we can store
-  // them all under the same key. For now, we don't do that because some callers
-  // need to differentiate between action types.
-  const ActionInfo* info = GetActionInfo(extension, keys::kBrowserAction);
-  if (info)
-    return info;
-  info = GetActionInfo(extension, keys::kPageAction);
-  if (info)
-    return info;
-  return GetActionInfo(extension, keys::kAction);
-}
-
-// static
-const ActionInfo* ActionInfo::GetExtensionActionInfo(
-    const Extension* extension) {
-  return GetActionInfo(extension, keys::kAction);
-}
-
-// static
-const ActionInfo* ActionInfo::GetBrowserActionInfo(const Extension* extension) {
-  return GetActionInfo(extension, keys::kBrowserAction);
-}
-
-const ActionInfo* ActionInfo::GetPageActionInfo(const Extension* extension) {
-  return GetActionInfo(extension, keys::kPageAction);
+  const ActionInfoData* data =
+      static_cast<ActionInfoData*>(extension->GetManifestData(keys::kAction));
+  return data ? data->action_info.get() : nullptr;
 }
 
 // static
 void ActionInfo::SetExtensionActionInfo(Extension* extension,
                                         std::unique_ptr<ActionInfo> info) {
+  // Note: we store all actions (actions, browser actions, and page actions)
+  // under the same key for simplicity because they are mutually exclusive,
+  // and most callers shouldn't care about the type.
   extension->SetManifestData(keys::kAction,
                              std::make_unique<ActionInfoData>(std::move(info)));
 }
 
-// static
-void ActionInfo::SetBrowserActionInfo(Extension* extension,
-                                      std::unique_ptr<ActionInfo> info) {
-  extension->SetManifestData(keys::kBrowserAction,
-                             std::make_unique<ActionInfoData>(std::move(info)));
-}
-
-// static
-void ActionInfo::SetPageActionInfo(Extension* extension,
-                                   std::unique_ptr<ActionInfo> info) {
-  extension->SetManifestData(keys::kPageAction,
-                             std::make_unique<ActionInfoData>(std::move(info)));
-}
-
 }  // namespace extensions

chrome/common/extensions/api/extension_action/action_info.h

@@ -46,31 +46,13 @@ struct ActionInfo {
 
   // Returns any action associated with the extension, whether it's specified
   // under the "page_action", "browser_action", or "action" key.
-  // TODO(devlin): This is a crutch while moving away from the distinct action
-  // types. Remove it when that's done.
+  // TODO(devlin): Rename this to GetExtensionActionInfo().
   static const ActionInfo* GetAnyActionInfo(const Extension* extension);
 
-  // Returns the action specified under the "action" key, if any.
-  static const ActionInfo* GetExtensionActionInfo(const Extension* extension);
-
-  // Returns the extension's browser action, if any.
-  static const ActionInfo* GetBrowserActionInfo(const Extension* extension);
-
-  // Returns the extension's page action, if any.
-  static const ActionInfo* GetPageActionInfo(const Extension* extension);
-
   // Sets the extension's action.
   static void SetExtensionActionInfo(Extension* extension,
                                      std::unique_ptr<ActionInfo> info);
 
-  // Sets the extension's browser action.
-  static void SetBrowserActionInfo(Extension* extension,
-                                   std::unique_ptr<ActionInfo> info);
-
-  // Sets the extension's page action.
-  static void SetPageActionInfo(Extension* extension,
-                                std::unique_ptr<ActionInfo> info);
-
   // The key this action corresponds to. NOTE: You should only use this if you
   // care about the actual manifest key. Use the other members (like
   // |default_state| for querying general info.

chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc

@@ -7,6 +7,7 @@
 
 #include "base/test/values_test_util.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
+#include "chrome/common/extensions/api/extension_action/action_info_test_util.h"
 #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
@@ -42,7 +43,7 @@ TEST_F(BrowserActionManifestTest,
 
   ASSERT_TRUE(extension.get());
   const ActionInfo* browser_action_info =
-      ActionInfo::GetBrowserActionInfo(extension.get());
+      GetActionInfoOfType(*extension, ActionInfo::TYPE_BROWSER);
   ASSERT_TRUE(browser_action_info);
   EXPECT_TRUE(browser_action_info->default_icon.empty());
 }
@@ -64,7 +65,7 @@ TEST_F(BrowserActionManifestTest,
 
   ASSERT_TRUE(extension.get());
   const ActionInfo* browser_action_info =
-      ActionInfo::GetBrowserActionInfo(extension.get());
+      GetActionInfoOfType(*extension, ActionInfo::TYPE_BROWSER);
   ASSERT_TRUE(browser_action_info);
   ASSERT_FALSE(browser_action_info->default_icon.empty());
 
@@ -98,7 +99,7 @@ TEST_F(BrowserActionManifestTest,
 
   ASSERT_TRUE(extension.get());
   const ActionInfo* browser_action_info =
-      ActionInfo::GetBrowserActionInfo(extension.get());
+      GetActionInfoOfType(*extension, ActionInfo::TYPE_BROWSER);
   ASSERT_TRUE(browser_action_info);
   ASSERT_FALSE(browser_action_info->default_icon.empty());
 

chrome/common/extensions/api/extension_action/page_action_manifest_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/path_service.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
+#include "chrome/common/extensions/api/extension_action/action_info_test_util.h"
 #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
@@ -35,7 +36,7 @@ std::unique_ptr<ActionInfo> PageActionManifestTest::LoadAction(
   scoped_refptr<Extension> extension = LoadAndExpectSuccess(
       manifest_filename.c_str());
   const ActionInfo* page_action_info =
-      ActionInfo::GetPageActionInfo(extension.get());
+      GetActionInfoOfType(*extension, ActionInfo::TYPE_PAGE);
   EXPECT_TRUE(page_action_info);
   if (page_action_info)
     return std::make_unique<ActionInfo>(*page_action_info);
@@ -49,7 +50,7 @@ TEST_F(PageActionManifestTest, ManifestVersion2DoesntAllowLegacyKeys) {
       LoadAndExpectSuccess("page_action_manifest_version_2.json"));
   ASSERT_TRUE(extension.get());
   const ActionInfo* page_action_info =
-      ActionInfo::GetPageActionInfo(extension.get());
+      GetActionInfoOfType(*extension, ActionInfo::TYPE_PAGE);
   ASSERT_TRUE(page_action_info);
 
   EXPECT_TRUE(page_action_info->default_icon.empty());
@@ -94,7 +95,7 @@ TEST_F(PageActionManifestTest, LoadPageActionHelper) {
   scoped_refptr<const Extension> extension =
       LoadAndExpectSuccess("page_action_default_popup.json");
   const ActionInfo* extension_action =
-      ActionInfo::GetPageActionInfo(extension.get());
+      GetActionInfoOfType(*extension, ActionInfo::TYPE_PAGE);
   ASSERT_TRUE(extension_action);
   EXPECT_EQ(extension->url().Resolve(kPopupHtmlFile),
             extension_action->default_popup_url);

chrome/common/extensions/manifest_handlers/extension_action_handler.cc

@@ -68,17 +68,7 @@ bool ExtensionActionHandler::Parse(Extension* extension,
     if (!action_info)
       return false;  // Failed to parse extension action definition.
 
-    switch (type) {
-      case ActionInfo::TYPE_ACTION:
-        ActionInfo::SetExtensionActionInfo(extension, std::move(action_info));
-        break;
-      case ActionInfo::TYPE_PAGE:
-        ActionInfo::SetPageActionInfo(extension, std::move(action_info));
-        break;
-      case ActionInfo::TYPE_BROWSER:
-        ActionInfo::SetBrowserActionInfo(extension, std::move(action_info));
-        break;
-    }
+    ActionInfo::SetExtensionActionInfo(extension, std::move(action_info));
   } else {  // No key, used for synthesizing an action for extensions with none.
     if (Manifest::IsComponentLocation(extension->location()))
       return true;  // Don't synthesize actions for component extensions.
@@ -89,7 +79,7 @@ bool ExtensionActionHandler::Parse(Extension* extension,
     // action) because the action should not be seen as enabled on every page.
     auto action_info = std::make_unique<ActionInfo>(ActionInfo::TYPE_PAGE);
     action_info->synthesized = true;
-    ActionInfo::SetPageActionInfo(extension, std::move(action_info));
+    ActionInfo::SetExtensionActionInfo(extension, std::move(action_info));
   }
 
   return true;