[Extensions] Let declarativeContent.showPageAction work with browser actions

There's not really much point in the distinction between page and
browser actions anymore, and we'd like to merge them. Make the
declarativeContent.showPageAction API work with browser actions.
A follow-up will introduce declarativeContent.showAction.

Bug: 893375
Change-Id: Ib4651c225099c62070239f0c37cb4b075d124b1c
Reviewed-on: https://chromium-review.googlesource.com/c/1270006Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611019}

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

@@ -42,8 +42,8 @@ const char kInvalidInstanceTypeError[] =
     "An action has an invalid instanceType: %s";
 const char kMissingInstanceTypeError[] = "Action is missing instanceType";
 const char kMissingParameter[] = "Missing parameter is required: %s";
-const char kNoPageAction[] =
-    "Can't use declarativeContent.ShowPageAction without a page action";
+const char kNoAction[] =
+    "Can't use declarativeContent.ShowPageAction without an action";
 const char kNoPageOrBrowserAction[] =
     "Can't use declarativeContent.SetIcon without a page or browser action";
 const char kIconNotSufficientlyVisible[] =
@@ -66,9 +66,12 @@ class ShowPageAction : public ContentAction {
       const Extension* extension,
       const base::DictionaryValue* dict,
       std::string* error) {
-    // We can't show a page action if the extension doesn't have one.
-    if (ActionInfo::GetPageActionInfo(extension) == NULL) {
-      *error = kNoPageAction;
+    // TODO(devlin): We should probably throw an error if the extension has no
+    // action specified in the manifest. Currently, this is allowed since
+    // extensions will have a synthesized page action.
+    if (!ActionInfo::GetPageActionInfo(extension) &&
+        !ActionInfo::GetBrowserActionInfo(extension)) {
+      *error = kNoAction;
       return std::unique_ptr<ContentAction>();
     }
     return base::WrapUnique(new ShowPageAction);
@@ -77,7 +80,7 @@ class ShowPageAction : public ContentAction {
   // Implementation of ContentAction:
   void Apply(const ApplyInfo& apply_info) const override {
     ExtensionAction* action =
-        GetPageAction(apply_info.browser_context, apply_info.extension);
+        GetAction(apply_info.browser_context, apply_info.extension);
     action->DeclarativeShow(ExtensionTabUtil::GetTabId(apply_info.tab));
     ExtensionActionAPI::Get(apply_info.browser_context)->NotifyChange(
         action, apply_info.tab, apply_info.browser_context);
@@ -86,7 +89,7 @@ class ShowPageAction : public ContentAction {
   void Reapply(const ApplyInfo& apply_info) const override {}
   void Revert(const ApplyInfo& apply_info) const override {
     if (ExtensionAction* action =
-            GetPageAction(apply_info.browser_context, apply_info.extension)) {
+            GetAction(apply_info.browser_context, apply_info.extension)) {
       action->UndoDeclarativeShow(ExtensionTabUtil::GetTabId(apply_info.tab));
       ExtensionActionAPI::Get(apply_info.browser_context)->NotifyChange(
           action, apply_info.tab, apply_info.browser_context);
@@ -94,11 +97,10 @@ class ShowPageAction : public ContentAction {
   }
 
  private:
-  static ExtensionAction* GetPageAction(
-      content::BrowserContext* browser_context,
-      const Extension* extension) {
+  static ExtensionAction* GetAction(content::BrowserContext* browser_context,
+                                    const Extension* extension) {
     return ExtensionActionManager::Get(browser_context)
-        ->GetPageAction(*extension);
+        ->GetExtensionAction(*extension);
   }
 
   DISALLOW_COPY_AND_ASSIGN(ShowPageAction);

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

@@ -122,44 +122,80 @@ TEST(DeclarativeContentActionTest, ShowPageActionWithoutPageAction) {
                  "  \"instanceType\": \"declarativeContent.ShowPageAction\",\n"
                  "}"),
       &error);
-  EXPECT_THAT(error, testing::HasSubstr("without a page action"));
+  EXPECT_THAT(error, testing::HasSubstr("without an action"));
   ASSERT_FALSE(result.get());
 }
 
-TEST(DeclarativeContentActionTest, ShowPageAction) {
+class ParameterizedDeclarativeContentActionTest
+    : public ::testing::TestWithParam<ExtensionBuilder::ActionType> {};
+
+TEST_P(ParameterizedDeclarativeContentActionTest, ShowPageAction) {
   TestExtensionEnvironment env;
 
-  const Extension* extension = env.MakeExtension(
-      *ParseJson("{\"page_action\": { \"default_title\": \"Extension\" } }"));
-  TestingProfile profile;
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("extension")
+          .SetAction(GetParam())
+          .SetLocation(Manifest::INTERNAL)
+          .Build();
+
+  env.GetExtensionService()->AddExtension(extension.get());
+
   std::string error;
+  TestingProfile profile;
   std::unique_ptr<const ContentAction> result = ContentAction::Create(
-      &profile, extension,
-      *ParseJson("{\n"
-                 "  \"instanceType\": \"declarativeContent.ShowPageAction\",\n"
-                 "}"),
+      nullptr, extension.get(),
+      *ParseJson(R"({"instanceType": "declarativeContent.ShowPageAction"})"),
       &error);
-  EXPECT_EQ("", error);
+  EXPECT_TRUE(error.empty()) << error;
   ASSERT_TRUE(result.get());
 
-  ExtensionAction* page_action =
-      ExtensionActionManager::Get(env.profile())->GetPageAction(*extension);
+  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);
+    // Switch the default so we properly see the action toggling.
+    action->SetIsVisible(ExtensionAction::kDefaultTabId, false);
+  } else {
+    action = action_manager->GetPageAction(*extension);
+    ASSERT_TRUE(action);
+  }
+
   std::unique_ptr<content::WebContents> contents = env.MakeTab();
   const int tab_id = ExtensionTabUtil::GetTabId(contents.get());
-  EXPECT_FALSE(page_action->GetIsVisible(tab_id));
-  ContentAction::ApplyInfo apply_info = {
-    extension, env.profile(), contents.get(), 100
-  };
+
+  // Currently, the action is not visible on the given tab.
+  EXPECT_FALSE(action->GetIsVisible(tab_id));
+  ContentAction::ApplyInfo apply_info = {extension.get(), env.profile(),
+                                         contents.get(), /*priority=*/100};
+
+  // Apply the content action once. The action should be visible on the tab.
   result->Apply(apply_info);
-  EXPECT_TRUE(page_action->GetIsVisible(tab_id));
+  EXPECT_TRUE(action->GetIsVisible(tab_id));
+  // Apply the content action a second time. The extension action should be
+  // visible on the tab, with a "count" of 2 (i.e., two different content
+  // actions are keeping it visible).
   result->Apply(apply_info);
-  EXPECT_TRUE(page_action->GetIsVisible(tab_id));
+  EXPECT_TRUE(action->GetIsVisible(tab_id));
+  // Revert one of the content actions. Since two content actions caused the
+  // extension action to be visible, it should still be visible after reverting
+  // only one.
   result->Revert(apply_info);
-  EXPECT_TRUE(page_action->GetIsVisible(tab_id));
+  EXPECT_TRUE(action->GetIsVisible(tab_id));
+  // Revert the final content action. The extension action should now be hidden
+  // again.
   result->Revert(apply_info);
-  EXPECT_FALSE(page_action->GetIsVisible(tab_id));
+  EXPECT_FALSE(action->GetIsVisible(tab_id));
 }
 
+INSTANTIATE_TEST_CASE_P(
+    ,
+    ParameterizedDeclarativeContentActionTest,
+    testing::Values(ExtensionBuilder::ActionType::BROWSER_ACTION,
+                    ExtensionBuilder::ActionType::PAGE_ACTION));
+
 TEST(DeclarativeContentActionTest, SetIcon) {
   TestExtensionEnvironment env;
 

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

@@ -16,6 +16,7 @@
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -713,13 +714,15 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
 
 namespace {
 
-class ShowPageActionWithoutPageActionTest
+// TODO(devlin): Would this be better as a parameterized test case that could
+// exercise all of (page action, browser action, no action)?
+class ShowPageActionWithoutSpecifiedPageActionTest
     : public DeclarativeContentApiTest,
       public testing::WithParamInterface<bool> {};
 
 }  // namespace
 
-IN_PROC_BROWSER_TEST_P(ShowPageActionWithoutPageActionTest, Test) {
+IN_PROC_BROWSER_TEST_P(ShowPageActionWithoutSpecifiedPageActionTest, Test) {
   bool has_browser_action = GetParam();
   // Load an extension without a page action.
   std::string manifest_without_page_action = kDeclarativeContentManifest;
@@ -739,38 +742,50 @@ IN_PROC_BROWSER_TEST_P(ShowPageActionWithoutPageActionTest, Test) {
   content::BrowserContext::GetDefaultStoragePartition(profile())
       ->FlushNetworkInterfaceForTesting();
 
+  ExtensionAction* action = ExtensionActionManager::Get(browser()->profile())
+                                ->GetExtensionAction(*extension);
+  ASSERT_TRUE(action);
+  if (has_browser_action) {
+    // Set the browser action default visibility to false, so that we check the
+    // same behavior as with page actions.
+    action->SetIsVisible(ExtensionAction::kDefaultTabId, false);
+  }
+
   const char kScript[] =
       "setRules([{\n"
       "  conditions: [new PageStateMatcher({\n"
       "                   pageUrl: {hostPrefix: \"test\"}})],\n"
       "  actions: [new ShowPageAction()]\n"
       "}], 'test_rule');\n";
-  const char kErrorSubstr[] = "without a page action";
+  const char kSuccessStr[] = "test_rule";
+
   std::string result = ExecuteScriptInBackgroundPage(extension->id(), kScript);
 
-  // Extensions with no action provided are given a page action by default
-  // (for visibility reasons). If an extension has a browser action, it
-  // should cause an error.
-  if (has_browser_action)
-    EXPECT_THAT(result, testing::HasSubstr(kErrorSubstr));
-  else
-    EXPECT_THAT(result, testing::Not(testing::HasSubstr(kErrorSubstr)));
+  // Since extensions with no action provided are given a page action by default
+  // (for visibility reasons) and ShowPageAction() should also work with
+  // browser actions, both of these should pass.
+  EXPECT_THAT(result, testing::HasSubstr(kSuccessStr));
 
   content::WebContents* const tab =
       browser()->tab_strip_model()->GetWebContentsAt(0);
   NavigateInRenderer(tab, GURL("http://test/"));
 
+  const int tab_id = SessionTabHelper::IdForTab(tab).id();
+  EXPECT_TRUE(action->GetIsVisible(tab_id));
+
   bool expect_page_action = !has_browser_action;
-  ExtensionAction* page_action =
-      ExtensionActionManager::Get(browser()->profile())
-          ->GetPageAction(*extension);
-  EXPECT_EQ(expect_page_action, page_action != nullptr);
-  EXPECT_EQ(expect_page_action ? 1u : 0u,
-            extension_action_test_util::GetVisiblePageActionCount(tab));
+  if (expect_page_action) {
+    ExtensionAction* page_action =
+        ExtensionActionManager::Get(browser()->profile())
+            ->GetPageAction(*extension);
+    EXPECT_EQ(expect_page_action, page_action != nullptr);
+    EXPECT_EQ(expect_page_action ? 1u : 0u,
+              extension_action_test_util::GetVisiblePageActionCount(tab));
+  }
 }
 
-INSTANTIATE_TEST_CASE_P(ShowPageActionWithoutPageActionTest,
-                        ShowPageActionWithoutPageActionTest,
+INSTANTIATE_TEST_CASE_P(ShowPageActionWithoutSpecifiedPageActionTest,
+                        ShowPageActionWithoutSpecifiedPageActionTest,
                         ::testing::Bool());
 
 IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,