[Extensions] Combine ExtensionAction logic on ExtensionActionManager

Instead of having separate stores and getters for page and browser
actions on ExtensionActionManager, have a single store for all actions.
Similarly, remove the type-specific GetPageAction() and
GetBrowserAction() methods from ExtensionActionManager, and only use
GetExtensionAction().

Call sites that rely on a certain type of action (which is relatively
rare) now check the action type through ExtensionAction::action_type().

Also clean up extension_action_manager_unittest.cc to use parameterized
tests and more targeted cases.

This CL does not yet hook up generic actions (specified through the
"action" key in the manifest) to ExtensionActionManager; that will
happen next.

Bug: 893373

Change-Id: Ib82f2f71b27f1118f2c02b9376cc5849aafab50a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1585959
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659757}

chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc

@@ -38,6 +38,7 @@
 #include "chrome/browser/chromeos/login/lock/screen_locker.h"
 #include "chrome/browser/chromeos/printing/cups_printers_manager.h"
 #include "chrome/browser/chromeos/system/input_device_settings.h"
+#include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
@@ -53,6 +54,7 @@
 #include "chrome/browser/ui/views/crostini/crostini_uninstaller_view.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/extensions/api/autotest_private.h"
+#include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager/session_manager_client.h"
@@ -379,9 +381,11 @@ AutotestPrivateGetExtensionsInfoFunction::Run() {
     extension_value->SetBoolean("isEnabled", service->IsExtensionEnabled(id));
     extension_value->SetBoolean(
         "allowedInIncognito", util::IsIncognitoEnabled(id, browser_context()));
+    const ExtensionAction* action =
+        extension_action_manager->GetExtensionAction(*extension);
     extension_value->SetBoolean(
         "hasPageAction",
-        extension_action_manager->GetPageAction(*extension) != NULL);
+        action && action->action_type() == ActionInfo::TYPE_PAGE);
 
     extensions_values->Append(std::move(extension_value));
   }

chrome/browser/extensions/api/declarative_content/content_action_unittest.cc

@@ -155,18 +155,15 @@ TEST_P(ParameterizedDeclarativeContentActionTest, ShowAction) {
   EXPECT_TRUE(error.empty()) << error;
   ASSERT_TRUE(result.get());
 
-  ExtensionAction* action = nullptr;
   auto* action_manager = ExtensionActionManager::Get(env.profile());
-  const bool is_browser_action =
-      GetParam() == ExtensionBuilder::ActionType::BROWSER_ACTION;
-  if (is_browser_action) {
-    action = action_manager->GetBrowserAction(*extension);
-    ASSERT_TRUE(action);
+  ExtensionAction* action = action_manager->GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
+  if (GetParam() == ExtensionBuilder::ActionType::BROWSER_ACTION) {
+    EXPECT_EQ(ActionInfo::TYPE_BROWSER, action->action_type());
     // Switch the default so we properly see the action toggling.
     action->SetIsVisible(ExtensionAction::kDefaultTabId, false);
   } else {
-    action = action_manager->GetPageAction(*extension);
-    ASSERT_TRUE(action);
+    EXPECT_EQ(ActionInfo::TYPE_PAGE, action->action_type());
   }
 
   std::unique_ptr<content::WebContents> contents = env.MakeTab();
@@ -242,21 +239,21 @@ TEST(DeclarativeContentActionTest, SetIcon) {
                   "Extensions.DeclarativeSetIconWasVisibleRendered"),
               testing::ElementsAre(base::Bucket(1, 1)));
 
-  ExtensionAction* page_action =
-      ExtensionActionManager::Get(env.profile())->GetPageAction(*extension);
+  ExtensionAction* action = ExtensionActionManager::Get(env.profile())
+                                ->GetExtensionAction(*extension);
   std::unique_ptr<content::WebContents> contents = env.MakeTab();
   const int tab_id = ExtensionTabUtil::GetTabId(contents.get());
-  EXPECT_FALSE(page_action->GetIsVisible(tab_id));
+  EXPECT_FALSE(action->GetIsVisible(tab_id));
   ContentAction::ApplyInfo apply_info = {
     extension, env.profile(), contents.get(), 100
   };
 
   // The declarative icon shouldn't exist unless the content action is applied.
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
   result->Apply(apply_info);
-  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_FALSE(action->GetDeclarativeIcon(tab_id).IsEmpty());
   result->Revert(apply_info);
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
 }
 
 TEST(DeclarativeContentActionTest, SetInvisibleIcon) {

chrome/browser/extensions/api/declarative_content/set_icon_apitest.cc

@@ -78,10 +78,10 @@ IN_PROC_BROWSER_TEST_F(SetIconAPITest, Overview) {
   // Wait for declarative rules to be set up.
   content::BrowserContext::GetDefaultStoragePartition(profile())
       ->FlushNetworkInterfaceForTesting();
-  const ExtensionAction* page_action =
-      ExtensionActionManager::Get(browser()->profile())->
-      GetPageAction(*extension);
-  ASSERT_TRUE(page_action);
+  const ExtensionAction* action =
+      ExtensionActionManager::Get(browser()->profile())
+          ->GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
 
   ASSERT_TRUE(ready.WaitUntilSatisfied());
   content::WebContents* const tab =
@@ -89,13 +89,13 @@ IN_PROC_BROWSER_TEST_F(SetIconAPITest, Overview) {
   const int tab_id = ExtensionTabUtil::GetTabId(tab);
 
   // There should be no declarative icon until we navigate to a matched page.
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
   NavigateInRenderer(tab, GURL("http://test1/"));
-  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_FALSE(action->GetDeclarativeIcon(tab_id).IsEmpty());
 
   // Navigating to an unmatched page should reset the icon.
   NavigateInRenderer(tab, GURL("http://test2/"));
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
 }
 }  // namespace
 }  // namespace extensions

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

@@ -145,8 +145,12 @@ class BrowserActionApiTest : public ExtensionApiTest {
   }
 
   ExtensionAction* GetBrowserAction(const Extension& extension) {
-    return ExtensionActionManager::Get(browser()->profile())->
-        GetBrowserAction(extension);
+    ExtensionAction* extension_action =
+        ExtensionActionManager::Get(browser()->profile())
+            ->GetExtensionAction(extension);
+    return extension_action->action_type() == ActionInfo::TYPE_BROWSER
+               ? extension_action
+               : nullptr;
   }
 
  private:

chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc

@@ -64,7 +64,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
   ASSERT_TRUE(listener.WaitUntilSatisfied());
 
   ExtensionAction* extension_action =
-      ExtensionActionManager::Get(profile())->GetBrowserAction(*extension);
+      ExtensionActionManager::Get(profile())->GetExtensionAction(*extension);
   ASSERT_TRUE(extension_action);
   EXPECT_EQ(SK_ColorBLUE, extension_action->GetBadgeBackgroundColor(0));
 }
@@ -89,7 +89,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BrowserActionDefaultPersistence) {
   ASSERT_TRUE(extension) << "Could not find extension in registry.";
 
   ExtensionAction* extension_action =
-      ExtensionActionManager::Get(profile())->GetBrowserAction(*extension);
+      ExtensionActionManager::Get(profile())->GetExtensionAction(*extension);
   ASSERT_TRUE(extension_action);
 
   // If the extension hasn't already set the badge text, then we should wait for

chrome/browser/extensions/api/extension_action/page_action_apitest.cc

@@ -33,8 +33,12 @@ namespace {
 class PageActionApiTest : public ExtensionApiTest {
  protected:
   ExtensionAction* GetPageAction(const Extension& extension) {
-    return ExtensionActionManager::Get(browser()->profile())->
-        GetPageAction(extension);
+    ExtensionAction* extension_action =
+        ExtensionActionManager::Get(browser()->profile())
+            ->GetExtensionAction(extension);
+    return extension_action->action_type() == ActionInfo::TYPE_PAGE
+               ? extension_action
+               : nullptr;
   }
 };
 

chrome/browser/extensions/extension_action_icon_factory_unittest.cc

@@ -152,8 +152,9 @@ class ExtensionActionIconFactoryTest
         IDR_EXTENSIONS_FAVICON);
   }
 
-  ExtensionAction* GetBrowserAction(const Extension& extension) {
-    return ExtensionActionManager::Get(profile())->GetBrowserAction(extension);
+  ExtensionAction* GetExtensionAction(const Extension& extension) {
+    return ExtensionActionManager::Get(profile())->GetExtensionAction(
+        extension);
   }
 
   TestingProfile* profile() { return profile_.get(); }
@@ -180,19 +181,18 @@ TEST_F(ExtensionActionIconFactoryTest, NoIcons) {
   scoped_refptr<Extension> extension(
       CreateExtension("browser_action/no_icon", Manifest::UNPACKED));
   ASSERT_TRUE(extension.get() != nullptr);
-  ExtensionAction* browser_action = GetBrowserAction(*extension);
-  ASSERT_TRUE(browser_action);
-  ASSERT_FALSE(browser_action->default_icon());
-  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
+  ExtensionAction* action = GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
+  ASSERT_FALSE(action->default_icon());
+  ASSERT_TRUE(action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
-  ExtensionActionIconFactory icon_factory(
-      profile(), extension.get(), browser_action, this);
+  ExtensionActionIconFactory icon_factory(profile(), extension.get(), action,
+                                          this);
 
   gfx::Image icon = icon_factory.GetIcon(0);
 
   EXPECT_TRUE(ImageRepsAreEqual(
-      browser_action->GetDefaultIconImage().ToImageSkia()->GetRepresentation(
-          1.0f),
+      action->GetDefaultIconImage().ToImageSkia()->GetRepresentation(1.0f),
       icon.ToImageSkia()->GetRepresentation(1.0f)));
 }
 
@@ -206,10 +206,10 @@ TEST_F(ExtensionActionIconFactoryTest, InvisibleIcon) {
 
   // Check that the default icon is not sufficiently visible.
   ASSERT_TRUE(extension);
-  ExtensionAction* browser_action = GetBrowserAction(*extension);
-  ASSERT_TRUE(browser_action);
-  EXPECT_TRUE(browser_action->default_icon());
-  gfx::Image default_icon = browser_action->GetDefaultIconImage();
+  ExtensionAction* action = GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
+  EXPECT_TRUE(action->default_icon());
+  gfx::Image default_icon = action->GetDefaultIconImage();
   EXPECT_FALSE(default_icon.IsEmpty());
   const SkBitmap* const bitmap = default_icon.ToSkBitmap();
   ASSERT_TRUE(bitmap);
@@ -218,17 +218,16 @@ TEST_F(ExtensionActionIconFactoryTest, InvisibleIcon) {
   // Set the flag for testing.
   ExtensionActionIconFactory::SetAllowInvisibleIconsForTest(false);
 
-  ExtensionActionIconFactory icon_factory(profile(), extension.get(),
-                                          browser_action, this);
+  ExtensionActionIconFactory icon_factory(profile(), extension.get(), action,
+                                          this);
 
   base::HistogramTester histogram_tester;
   gfx::Image icon = icon_factory.GetIcon(0);
   // The default icon should not be returned, since it's invisible.
   // The placeholder icon should be returned instead.
-  EXPECT_TRUE(ImageRepsAreEqual(browser_action->GetPlaceholderIconImage()
-                                    .ToImageSkia()
-                                    ->GetRepresentation(1.0f),
-                                icon.ToImageSkia()->GetRepresentation(1.0f)));
+  EXPECT_TRUE(ImageRepsAreEqual(
+      action->GetPlaceholderIconImage().ToImageSkia()->GetRepresentation(1.0f),
+      icon.ToImageSkia()->GetRepresentation(1.0f)));
   EXPECT_THAT(histogram_tester.GetAllSamples(
                   "Extensions.ManifestIconSetIconWasVisibleForPacked"),
               testing::ElementsAre(base::Bucket(0, 1)));
@@ -249,20 +248,20 @@ TEST_F(ExtensionActionIconFactoryTest, AfterSetIcon) {
   scoped_refptr<Extension> extension(
       CreateExtension("browser_action/no_icon", Manifest::UNPACKED));
   ASSERT_TRUE(extension.get() != nullptr);
-  ExtensionAction* browser_action = GetBrowserAction(*extension);
-  ASSERT_TRUE(browser_action);
-  ASSERT_FALSE(browser_action->default_icon());
-  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
+  ExtensionAction* action = GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
+  ASSERT_FALSE(action->default_icon());
+  ASSERT_TRUE(action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
   gfx::Image set_icon = LoadIcon("browser_action/no_icon/icon.png");
   ASSERT_FALSE(set_icon.IsEmpty());
 
-  browser_action->SetIcon(0, set_icon);
+  action->SetIcon(0, set_icon);
 
-  ASSERT_FALSE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
+  ASSERT_FALSE(action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
-  ExtensionActionIconFactory icon_factory(
-      profile(), extension.get(), browser_action, this);
+  ExtensionActionIconFactory icon_factory(profile(), extension.get(), action,
+                                          this);
 
   gfx::Image icon = icon_factory.GetIcon(0);
 
@@ -274,8 +273,7 @@ TEST_F(ExtensionActionIconFactoryTest, AfterSetIcon) {
   icon = icon_factory.GetIcon(1);
 
   EXPECT_TRUE(ImageRepsAreEqual(
-      browser_action->GetDefaultIconImage().ToImageSkia()->GetRepresentation(
-          1.0f),
+      action->GetDefaultIconImage().ToImageSkia()->GetRepresentation(1.0f),
       icon.ToImageSkia()->GetRepresentation(1.0f)));
 }
 
@@ -288,10 +286,10 @@ TEST_F(ExtensionActionIconFactoryTest, DefaultIcon) {
   scoped_refptr<Extension> extension(
       CreateExtension("browser_action/no_icon", Manifest::UNPACKED));
   ASSERT_TRUE(extension.get() != nullptr);
-  ExtensionAction* browser_action = GetBrowserAction(*extension);
-  ASSERT_TRUE(browser_action);
-  ASSERT_FALSE(browser_action->default_icon());
-  ASSERT_TRUE(browser_action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
+  ExtensionAction* action = GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
+  ASSERT_FALSE(action->default_icon());
+  ASSERT_TRUE(action->GetExplicitlySetIcon(0 /*tab id*/).IsEmpty());
 
   scoped_refptr<const Extension> extension_with_icon =
       CreateExtension("browser_action_with_icon", Manifest::UNPACKED);
@@ -302,11 +300,11 @@ TEST_F(ExtensionActionIconFactoryTest, DefaultIcon) {
       EnsureImageSize(LoadIcon("browser_action_with_icon/icon.png"), icon_size);
   ASSERT_FALSE(default_icon.IsEmpty());
 
-  browser_action = GetBrowserAction(*extension_with_icon);
-  ASSERT_TRUE(browser_action->default_icon());
+  action = GetExtensionAction(*extension_with_icon);
+  ASSERT_TRUE(action->default_icon());
 
-  ExtensionActionIconFactory icon_factory(
-      profile(), extension_with_icon.get(), browser_action, this);
+  ExtensionActionIconFactory icon_factory(profile(), extension_with_icon.get(),
+                                          action, this);
 
   gfx::Image icon = icon_factory.GetIcon(0);
 

chrome/browser/extensions/extension_action_manager.cc

@@ -80,31 +80,29 @@ void ExtensionActionManager::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionReason reason) {
-  page_actions_.erase(extension->id());
-  browser_actions_.erase(extension->id());
+  actions_.erase(extension->id());
 }
 
-namespace {
-
-// Returns map[extension_id] if that entry exists. Otherwise, if
-// action_info!=nullptr, creates an ExtensionAction from it, fills in the map,
-// and returns that.  Otherwise (action_info==nullptr), returns nullptr.
-ExtensionAction* GetOrCreateOrNull(
-    std::map<std::string, std::unique_ptr<ExtensionAction>>* map,
-    const Extension& extension,
-    const ActionInfo* action_info,
-    Profile* profile) {
-  auto it = map->find(extension.id());
-  if (it != map->end())
-    return it->second.get();
+ExtensionAction* ExtensionActionManager::GetExtensionAction(
+    const Extension& extension) const {
+  auto iter = actions_.find(extension.id());
+  if (iter != actions_.end())
+    return iter->second.get();
+
+  // TODO(https://crbug.com/893373): Update this to use
+  // ActionInfo::GetAnyActionInfo() once all callers can handle a generic action
+  // type.
+  const ActionInfo* action_info = ActionInfo::GetBrowserActionInfo(&extension);
+  if (!action_info)
+    action_info = ActionInfo::GetPageActionInfo(&extension);
   if (!action_info)
     return nullptr;
 
   // Only create action info for enabled extensions.
   // This avoids bugs where actions are recreated just after being removed
   // in response to OnExtensionUnloaded().
-  if (!ExtensionRegistry::Get(profile)
-      ->enabled_extensions().Contains(extension.id())) {
+  if (!ExtensionRegistry::Get(profile_)->enabled_extensions().Contains(
+          extension.id())) {
     return nullptr;
   }
 
@@ -112,36 +110,14 @@ ExtensionAction* GetOrCreateOrNull(
 
   if (action->default_icon()) {
     action->SetDefaultIconImage(std::make_unique<IconImage>(
-        profile, &extension, *action->default_icon(),
+        profile_, &extension, *action->default_icon(),
         ExtensionAction::ActionIconSize(),
         ExtensionAction::FallbackIcon().AsImageSkia(), nullptr));
   }
 
   ExtensionAction* raw_action = action.get();
-  (*map)[extension.id()] = std::move(action);
+  actions_[extension.id()] = std::move(action);
   return raw_action;
 }
 
-}  // namespace
-
-ExtensionAction* ExtensionActionManager::GetPageAction(
-    const Extension& extension) const {
-  return GetOrCreateOrNull(&page_actions_, extension,
-                           ActionInfo::GetPageActionInfo(&extension),
-                           profile_);
-}
-
-ExtensionAction* ExtensionActionManager::GetBrowserAction(
-    const Extension& extension) const {
-  return GetOrCreateOrNull(&browser_actions_, extension,
-                           ActionInfo::GetBrowserActionInfo(&extension),
-                           profile_);
-}
-
-ExtensionAction* ExtensionActionManager::GetExtensionAction(
-    const Extension& extension) const {
-  ExtensionAction* action = GetBrowserAction(extension);
-  return action ? action : GetPageAction(extension);
-}
-
 }  // namespace extensions

chrome/browser/extensions/extension_action_manager.h

@@ -34,14 +34,7 @@ class ExtensionActionManager : public KeyedService,
   // shared between a profile and its incognito version.
   static ExtensionActionManager* Get(content::BrowserContext* browser_context);
 
-  // Retrieves the page action, browser action, or system indicator for
-  // |extension|.
-  // If the result is not NULL, it remains valid until the extension is
-  // unloaded.
-  ExtensionAction* GetPageAction(const Extension& extension) const;
-  ExtensionAction* GetBrowserAction(const Extension& extension) const;
-
-  // Returns either the PageAction or BrowserAction for |extension|, or NULL if
+  // Returns either the PageAction or BrowserAction for |extension|, or null if
   // none exists. Since an extension can only declare one of Browser|PageAction,
   // this is okay to use anywhere you need a generic "ExtensionAction".
   ExtensionAction* GetExtensionAction(const Extension& extension) const;
@@ -60,12 +53,10 @@ class ExtensionActionManager : public KeyedService,
 
   // Keyed by Extension ID.  These maps are populated lazily when their
   // ExtensionAction is first requested, and the entries are removed when the
-  // extension is unloaded.  Not every extension has a page action or browser
-  // action.
+  // extension is unloaded.  Not every extension has an action.
   using ExtIdToActionMap =
       std::map<std::string, std::unique_ptr<ExtensionAction>>;
-  mutable ExtIdToActionMap page_actions_;
-  mutable ExtIdToActionMap browser_actions_;
+  mutable ExtIdToActionMap actions_;
 };
 
 }  // namespace extensions

chrome/browser/extensions/extension_action_storage_manager.cc

@@ -214,8 +214,9 @@ ExtensionActionStorageManager::~ExtensionActionStorageManager() {
 void ExtensionActionStorageManager::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
-  if (!ExtensionActionManager::Get(browser_context_)->GetBrowserAction(
-          *extension))
+  ExtensionAction* action = ExtensionActionManager::Get(browser_context_)
+                                ->GetExtensionAction(*extension);
+  if (!action || action->action_type() != ActionInfo::TYPE_BROWSER)
     return;
 
   StateStore* store = GetStateStore();
@@ -237,6 +238,7 @@ void ExtensionActionStorageManager::OnExtensionActionUpdated(
   // is null. We only persist the default settings to disk, since per-tab
   // settings can't be persisted across browser sessions.
   bool for_default_tab = !web_contents;
+  // TODO(devlin): We should probably persist for TYPE_ACTION as well.
   if (browser_context_ == browser_context &&
       extension_action->action_type() == ActionInfo::TYPE_BROWSER &&
       for_default_tab) {
@@ -267,10 +269,9 @@ void ExtensionActionStorageManager::ReadFromStorage(
   if (!extension)
     return;
 
-  ExtensionAction* browser_action =
-      ExtensionActionManager::Get(browser_context_)->GetBrowserAction(
-          *extension);
-  if (!browser_action) {
+  ExtensionAction* action = ExtensionActionManager::Get(browser_context_)
+                                ->GetExtensionAction(*extension);
+  if (!action || action->action_type() != ActionInfo::TYPE_BROWSER) {
     // This can happen if the extension is updated between startup and when the
     // storage read comes back, and the update removes the browser action.
     // http://crbug.com/349371
@@ -281,7 +282,7 @@ void ExtensionActionStorageManager::ReadFromStorage(
   if (!value.get() || !value->GetAsDictionary(&dict))
     return;
 
-  SetDefaultsFromValue(dict, browser_action);
+  SetDefaultsFromValue(dict, action);
 }
 
 StateStore* ExtensionActionStorageManager::GetStateStore() {

chrome/browser/extensions/extension_action_test_util.cc

@@ -42,8 +42,9 @@ size_t GetPageActionCount(content::WebContents* web_contents,
   for (const ToolbarActionsModel::ActionId& action_id : toolbar_action_ids) {
     const Extension* extension = enabled_extensions.GetByID(action_id);
     ExtensionAction* extension_action =
-        action_manager->GetPageAction(*extension);
+        action_manager->GetExtensionAction(*extension);
     if (extension_action &&
+        extension_action->action_type() == ActionInfo::TYPE_PAGE &&
         (!only_count_visible || extension_action->GetIsVisible(tab_id.id()))) {
       ++count;
     }

chrome/browser/extensions/extension_keybinding_apitest.cc

@@ -258,10 +258,10 @@ IN_PROC_BROWSER_TEST_F(CommandsApiTest, PageAction) {
   ASSERT_TRUE(WaitForPageActionVisibilityChangeTo(1));
   int tab_id = SessionTabHelper::FromWebContents(
       browser()->tab_strip_model()->GetActiveWebContents())->session_id().id();
-  ExtensionAction* action =
-      ExtensionActionManager::Get(browser()->profile())->
-      GetPageAction(*extension);
+  ExtensionAction* action = ExtensionActionManager::Get(browser()->profile())
+                                ->GetExtensionAction(*extension);
   ASSERT_TRUE(action);
+  EXPECT_EQ(ActionInfo::TYPE_PAGE, action->action_type());
   EXPECT_EQ("Send message", action->GetTitle(tab_id));
 
   ExtensionTestMessageListener test_listener(false);  // Won't reply.

chrome/browser/extensions/native_bindings_apitest.cc

@@ -90,26 +90,26 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, DeclarativeEvents) {
   ASSERT_TRUE(listener.WaitUntilSatisfied());
 
   // The extension's page action should currently be hidden.
-  ExtensionAction* page_action =
-      ExtensionActionManager::Get(profile())->GetPageAction(*extension);
+  ExtensionAction* action =
+      ExtensionActionManager::Get(profile())->GetExtensionAction(*extension);
   content::WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   int tab_id = SessionTabHelper::IdForTab(web_contents).id();
-  EXPECT_FALSE(page_action->GetIsVisible(tab_id));
-  EXPECT_TRUE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_FALSE(action->GetIsVisible(tab_id));
+  EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
 
   // Navigating to example.com should show the page action.
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "example.com", "/native_bindings/simple.html"));
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(page_action->GetIsVisible(tab_id));
-  EXPECT_FALSE(page_action->GetDeclarativeIcon(tab_id).IsEmpty());
+  EXPECT_TRUE(action->GetIsVisible(tab_id));
+  EXPECT_FALSE(action->GetDeclarativeIcon(tab_id).IsEmpty());
 
   // And the extension should be notified of the click.
   ExtensionTestMessageListener clicked_listener("clicked and removed", false);
   ExtensionActionAPI::Get(profile())->DispatchExtensionActionClicked(
-      *page_action, web_contents, extension);
+      *action, web_contents, extension);
   ASSERT_TRUE(clicked_listener.WaitUntilSatisfied());
 }