Uncouple ActiveScriptController from LocationBarController

Since the indication of an active script running won't always be tied to the
location bar, it doesn't make sense for ActiveScriptController to be owned by
LocationBarController.

Since the "generated" actions were also only used for display
in the location bar, move that logic to the LocationBarController.

And, since we're there, also add a bit more testing for LocationBarController.

BUG=408676
TBR=avi@chromium.org (for method name change in cocoa)

Review URL: https://codereview.chromium.org/527963003

Cr-Commit-Position: refs/heads/master@{#293153}

chrome/browser/extensions/active_script_controller.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_util.h"
-#include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/permissions_updater.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
@@ -78,13 +77,7 @@ ActiveScriptController* ActiveScriptController::GetForWebContents(
   if (!web_contents)
     return NULL;
   TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
-  if (!tab_helper)
-    return NULL;
-  LocationBarController* location_bar_controller =
-      tab_helper->location_bar_controller();
-  // This should never be NULL.
-  DCHECK(location_bar_controller);
-  return location_bar_controller->active_script_controller();
+  return tab_helper ? tab_helper->active_script_controller() : NULL;
 }
 
 void ActiveScriptController::OnActiveTabPermissionGranted(
@@ -149,29 +142,10 @@ void ActiveScriptController::OnClicked(const Extension* extension) {
   RunPendingForExtension(extension);
 }
 
-bool ActiveScriptController::HasActiveScriptAction(const Extension* extension) {
+bool ActiveScriptController::WantsToRun(const Extension* extension) {
   return enabled_ && pending_requests_.count(extension->id()) > 0;
 }
 
-ExtensionAction* ActiveScriptController::GetActionForExtension(
-    const Extension* extension) {
-  if (!enabled_ || pending_requests_.count(extension->id()) == 0)
-    return NULL;  // No action for this extension.
-
-  ActiveScriptMap::iterator existing =
-      active_script_actions_.find(extension->id());
-  if (existing != active_script_actions_.end())
-    return existing->second.get();
-
-  linked_ptr<ExtensionAction> action(ExtensionActionManager::Get(
-      Profile::FromBrowserContext(web_contents()->GetBrowserContext()))
-      ->GetBestFitAction(*extension, ActionInfo::TYPE_PAGE).release());
-  action->SetIsVisible(ExtensionAction::kDefaultTabId, true);
-
-  active_script_actions_[extension->id()] = action;
-  return action.get();
-}
-
 PermissionsData::AccessType
 ActiveScriptController::RequiresUserConsentForScriptInjection(
     const Extension* extension,

chrome/browser/extensions/active_script_controller.h

@@ -12,8 +12,7 @@
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
-#include "base/memory/linked_ptr.h"
-#include "chrome/browser/extensions/location_bar_controller.h"
+#include "base/scoped_observer.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/common/permissions/permissions_data.h"
@@ -64,12 +63,9 @@ class ActiveScriptController : public content::WebContentsObserver,
   // been clicked, running any pending tasks that were previously shelved.
   void OnClicked(const Extension* extension);
 
-  // Returns true if there is an active script injection action for |extension|.
-  bool HasActiveScriptAction(const Extension* extension);
-
-  // Returns the action to display for the given |extension|, or NULL if no
-  // action should be displayed.
-  ExtensionAction* GetActionForExtension(const Extension* extension);
+  // Returns true if the given |extension| has a pending script that wants to
+  // run.
+  bool WantsToRun(const Extension* extension);
 
 #if defined(UNIT_TEST)
   // Only used in tests.
@@ -141,12 +137,6 @@ class ActiveScriptController : public content::WebContentsObserver,
   // should incorporate more fully with ActiveTab.
   std::set<std::string> permitted_extensions_;
 
-  // Script badges that have been generated for extensions. This is both those
-  // with actions already declared that are copied and normalised, and actions
-  // that get generated for extensions that haven't declared anything.
-  typedef std::map<std::string, linked_ptr<ExtensionAction> > ActiveScriptMap;
-  ActiveScriptMap active_script_actions_;
-
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;
 

chrome/browser/extensions/active_script_controller_browsertest.cc

@@ -9,7 +9,6 @@
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_test_message_listener.h"
-#include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/ui/browser.h"
@@ -171,14 +170,12 @@ class ActiveScriptTester {
   testing::AssertionResult Verify();
 
  private:
-  // Returns the location bar controller, or NULL if one does not exist.
-  LocationBarController* GetLocationBarController();
-
   // Returns the active script controller, or NULL if one does not exist.
   ActiveScriptController* GetActiveScriptController();
 
-  // Get the ExtensionAction for this extension, or NULL if one does not exist.
-  ExtensionAction* GetAction();
+  // Returns true if the extension has a pending request with the active script
+  // controller.
+  bool WantsToRun();
 
   // The name of the extension, and also the message it sends.
   std::string name_;
@@ -237,26 +234,19 @@ testing::AssertionResult ActiveScriptTester::Verify() {
   // Make sure all running tasks are complete.
   content::RunAllPendingInMessageLoop();
 
-  LocationBarController* location_bar_controller = GetLocationBarController();
-  if (!location_bar_controller) {
-    return testing::AssertionFailure()
-        << "Could not find location bar controller";
-  }
-
-  ActiveScriptController* controller =
-      location_bar_controller->active_script_controller();
+  ActiveScriptController* controller = GetActiveScriptController();
   if (!controller)
     return testing::AssertionFailure() << "Could not find controller.";
 
-  ExtensionAction* action = GetAction();
-  bool has_action = action != NULL;
+  bool wants_to_run = WantsToRun();
 
   // An extension should have an action displayed iff it requires user consent.
-  if ((requires_consent_ == REQUIRES_CONSENT && !has_action) ||
-      (requires_consent_ == DOES_NOT_REQUIRE_CONSENT && has_action)) {
+  if ((requires_consent_ == REQUIRES_CONSENT && !wants_to_run) ||
+      (requires_consent_ == DOES_NOT_REQUIRE_CONSENT && wants_to_run)) {
     return testing::AssertionFailure()
-        << "Improper action status for " << name_ << ": expected "
-        << (requires_consent_ == REQUIRES_CONSENT) << ", found " << has_action;
+        << "Improper wants to run for " << name_ << ": expected "
+        << (requires_consent_ == REQUIRES_CONSENT) << ", found "
+        << wants_to_run;
   }
 
   // If the extension has permission, we should be able to simply wait for it
@@ -273,8 +263,8 @@ testing::AssertionResult ActiveScriptTester::Verify() {
         name_ << "'s script ran without permission.";
   }
 
-  // If we reach this point, we should always have an action.
-  DCHECK(action);
+  // If we reach this point, we should always want to run.
+  DCHECK(wants_to_run);
 
   // Grant permission by clicking on the extension action.
   controller->OnClicked(extension_);
@@ -282,17 +272,17 @@ testing::AssertionResult ActiveScriptTester::Verify() {
   // Now, the extension should be able to inject the script.
   inject_success_listener_->WaitUntilSatisfied();
 
-  // The Action should have disappeared.
-  has_action = GetAction() != NULL;
-  if (has_action) {
+  // The extension should no longer want to run.
+  wants_to_run = WantsToRun();
+  if (wants_to_run) {
     return testing::AssertionFailure()
-        << "Extension " << name_ << " has lingering action.";
+        << "Extension " << name_ << " still wants to run after injecting.";
   }
 
   return testing::AssertionSuccess();
 }
 
-LocationBarController* ActiveScriptTester::GetLocationBarController() {
+ActiveScriptController* ActiveScriptTester::GetActiveScriptController() {
   content::WebContents* web_contents =
       browser_ ? browser_->tab_strip_model()->GetActiveWebContents() : NULL;
 
@@ -300,18 +290,12 @@ LocationBarController* ActiveScriptTester::GetLocationBarController() {
     return NULL;
 
   TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
-  return tab_helper ? tab_helper->location_bar_controller() : NULL;
-}
-
-ActiveScriptController* ActiveScriptTester::GetActiveScriptController() {
-  LocationBarController* location_bar_controller = GetLocationBarController();
-  return location_bar_controller ?
-      location_bar_controller->active_script_controller() : NULL;
+  return tab_helper ? tab_helper->active_script_controller() : NULL;
 }
 
-ExtensionAction* ActiveScriptTester::GetAction() {
+bool ActiveScriptTester::WantsToRun() {
   ActiveScriptController* controller = GetActiveScriptController();
-  return controller ? controller->GetActionForExtension(extension_) : NULL;
+  return controller ? controller->WantsToRun(extension_) : false;
 }
 
 IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
@@ -396,8 +380,8 @@ IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
       browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
 
   // Both extensions should have pending requests.
-  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension1));
-  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension2));
+  EXPECT_TRUE(active_script_controller->WantsToRun(extension1));
+  EXPECT_TRUE(active_script_controller->WantsToRun(extension2));
 
   // Unload one of the extensions.
   UnloadExtension(extension2->id());
@@ -406,8 +390,8 @@ IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
 
   // We should have pending requests for extension1, but not the removed
   // extension2.
-  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension1));
-  EXPECT_FALSE(active_script_controller->GetActionForExtension(extension2));
+  EXPECT_TRUE(active_script_controller->WantsToRun(extension1));
+  EXPECT_FALSE(active_script_controller->WantsToRun(extension2));
 
   // We should still be able to run the request for extension1.
   ExtensionTestMessageListener inject_success_listener(

chrome/browser/extensions/active_script_controller_unittest.cc

@@ -164,11 +164,9 @@ void ActiveScriptControllerUnitTest::SetUp() {
 
   TabHelper::CreateForWebContents(web_contents());
   TabHelper* tab_helper = TabHelper::FromWebContents(web_contents());
-  // None of these should ever be NULL.
+  // These should never be NULL.
   DCHECK(tab_helper);
-  DCHECK(tab_helper->location_bar_controller());
-  active_script_controller_ =
-      tab_helper->location_bar_controller()->active_script_controller();
+  active_script_controller_ = tab_helper->active_script_controller();
   DCHECK(active_script_controller_);
 }
 
@@ -182,22 +180,23 @@ TEST_F(ActiveScriptControllerUnitTest, RequestPermissionAndExecute) {
 
   // Ensure that there aren't any executions pending.
   ASSERT_EQ(0u, GetExecutionCountForExtension(extension->id()));
-  ASSERT_FALSE(controller()->GetActionForExtension(extension));
+  ASSERT_FALSE(controller()->WantsToRun(extension));
 
   // Since the extension requests all_hosts, we should require user consent.
   EXPECT_TRUE(RequiresUserConsent(extension));
 
-  // Request an injection. There should be an action visible, but no executions.
+  // Request an injection. The extension should want to run, but should not have
+  // executed.
   RequestInjection(extension);
-  EXPECT_TRUE(controller()->GetActionForExtension(extension));
+  EXPECT_TRUE(controller()->WantsToRun(extension));
   EXPECT_EQ(0u, GetExecutionCountForExtension(extension->id()));
 
   // Click to accept the extension executing.
   controller()->OnClicked(extension);
 
-  // The extension should execute, and the action should go away.
+  // The extension should execute, and the extension shouldn't want to run.
   EXPECT_EQ(1u, GetExecutionCountForExtension(extension->id()));
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
 
   // Since we already executed on the given page, we shouldn't need permission
   // for a second time.
@@ -220,7 +219,7 @@ TEST_F(ActiveScriptControllerUnitTest, RequestPermissionAndExecute) {
   RequestInjection(extension);
   controller()->OnClicked(extension);
   EXPECT_EQ(2u, GetExecutionCountForExtension(extension->id()));
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
 
   // Navigating to another site should also clear the permissions.
   NavigateAndCommit(GURL("https://www.foo.com"));
@@ -237,15 +236,15 @@ TEST_F(ActiveScriptControllerUnitTest, PendingInjectionsRemovedAtNavigation) {
 
   ASSERT_EQ(0u, GetExecutionCountForExtension(extension->id()));
 
-  // Request an injection. There should be an action visible, but no executions.
+  // Request an injection. The extension should want to run, but not execute.
   RequestInjection(extension);
-  EXPECT_TRUE(controller()->GetActionForExtension(extension));
+  EXPECT_TRUE(controller()->WantsToRun(extension));
   EXPECT_EQ(0u, GetExecutionCountForExtension(extension->id()));
 
   // Reload. This should remove the pending injection, and we should not
   // execute anything.
   Reload();
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
   EXPECT_EQ(0u, GetExecutionCountForExtension(extension->id()));
 
   // Request and accept a new injection.
@@ -255,7 +254,7 @@ TEST_F(ActiveScriptControllerUnitTest, PendingInjectionsRemovedAtNavigation) {
   // The extension should only have executed once, even though a grand total
   // of two executions were requested.
   EXPECT_EQ(1u, GetExecutionCountForExtension(extension->id()));
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
 }
 
 // Test that queueing multiple pending injections, and then accepting, triggers
@@ -278,7 +277,7 @@ TEST_F(ActiveScriptControllerUnitTest, MultiplePendingInjection) {
 
   // All pending injections should have executed.
   EXPECT_EQ(kNumInjections, GetExecutionCountForExtension(extension->id()));
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
 }
 
 TEST_F(ActiveScriptControllerUnitTest, ActiveScriptsUseActiveTabPermissions) {
@@ -315,7 +314,7 @@ TEST_F(ActiveScriptControllerUnitTest, ActiveScriptsUseActiveTabPermissions) {
   EXPECT_TRUE(RequiresUserConsent(extension));
 
   RequestInjection(extension);
-  EXPECT_TRUE(controller()->GetActionForExtension(extension));
+  EXPECT_TRUE(controller()->WantsToRun(extension));
   EXPECT_EQ(0u, GetExecutionCountForExtension(extension->id()));
 
   // Grant active tab.
@@ -324,7 +323,7 @@ TEST_F(ActiveScriptControllerUnitTest, ActiveScriptsUseActiveTabPermissions) {
   // The pending injections should have run since active tab permission was
   // granted.
   EXPECT_EQ(1u, GetExecutionCountForExtension(extension->id()));
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
 }
 
 TEST_F(ActiveScriptControllerUnitTest, ActiveScriptsCanHaveAllUrlsPref) {
@@ -359,22 +358,22 @@ TEST_F(ActiveScriptControllerUnitTest, TestAlwaysRun) {
 
   // Ensure that there aren't any executions pending.
   ASSERT_EQ(0u, GetExecutionCountForExtension(extension->id()));
-  ASSERT_FALSE(controller()->GetActionForExtension(extension));
+  ASSERT_FALSE(controller()->WantsToRun(extension));
 
   // Since the extension requests all_hosts, we should require user consent.
   EXPECT_TRUE(RequiresUserConsent(extension));
 
-  // Request an injection. There should be an action visible, but no executions.
+  // Request an injection. The extension should want to run, but not execute.
   RequestInjection(extension);
-  EXPECT_TRUE(controller()->GetActionForExtension(extension));
+  EXPECT_TRUE(controller()->WantsToRun(extension));
   EXPECT_EQ(0u, GetExecutionCountForExtension(extension->id()));
 
   // Allow the extension to always run on this origin.
   controller()->AlwaysRunOnVisibleOrigin(extension);
 
-  // The extension should execute, and the action should go away.
+  // The extension should execute, and the extension shouldn't want to run.
   EXPECT_EQ(1u, GetExecutionCountForExtension(extension->id()));
-  EXPECT_FALSE(controller()->GetActionForExtension(extension));
+  EXPECT_FALSE(controller()->WantsToRun(extension));
 
   // Since we already executed on the given page, we shouldn't need permission
   // for a second time.

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

@@ -175,7 +175,7 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction(
       ActiveScriptController::GetForWebContents(web_contents);
   bool has_pending_scripts = false;
   if (active_script_controller &&
-      active_script_controller->GetActionForExtension(extension)) {
+      active_script_controller->WantsToRun(extension)) {
     has_pending_scripts = true;
   }
 

chrome/browser/extensions/extension_context_menu_model.cc

@@ -283,7 +283,7 @@ void ExtensionContextMenuModel::InitMenu(const Extension* extension) {
   WebContents* web_contents = GetActiveWebContents();
   if (web_contents &&
       extensions::ActiveScriptController::GetForWebContents(web_contents)
-          ->HasActiveScriptAction(extension)) {
+          ->WantsToRun(extension)) {
     AddItemWithStringId(ALWAYS_RUN, IDS_EXTENSIONS_ALWAYS_RUN);
   }
 

chrome/browser/extensions/location_bar_controller.cc

@@ -20,7 +20,6 @@ LocationBarController::LocationBarController(
       action_manager_(ExtensionActionManager::Get(browser_context_)),
       should_show_page_actions_(
           !FeatureSwitch::extension_action_redesign()->IsEnabled()),
-      active_script_controller_(new ActiveScriptController(web_contents_)),
       extension_registry_observer_(this) {
   if (should_show_page_actions_)
     extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
@@ -36,6 +35,8 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
   if (!should_show_page_actions_)
     return current_actions;
 
+  ActiveScriptController* active_script_controller =
+      ActiveScriptController::GetForWebContents(web_contents_);
   for (ExtensionSet::const_iterator iter = extensions.begin();
        iter != extensions.end();
        ++iter) {
@@ -43,8 +44,22 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
     // one action per extension. If clicking on an active script action ever
     // has a response, then we will need to split the actions.
     ExtensionAction* action = action_manager_->GetPageAction(**iter);
-    if (!action)
-      action = active_script_controller_->GetActionForExtension(iter->get());
+    if (!action && active_script_controller->WantsToRun(*iter)) {
+      ExtensionActionMap::iterator existing =
+          active_script_actions_.find((*iter)->id());
+      if (existing != active_script_actions_.end()) {
+        action = existing->second.get();
+      } else {
+        linked_ptr<ExtensionAction> active_script_action(
+            ExtensionActionManager::Get(browser_context_)->
+                GetBestFitAction(**iter, ActionInfo::TYPE_PAGE).release());
+        active_script_action->SetIsVisible(
+            ExtensionAction::kDefaultTabId, true);
+        active_script_actions_[(*iter)->id()] = active_script_action;
+        action = active_script_action.get();
+      }
+    }
+
     if (action)
       current_actions.push_back(action);
   }
@@ -55,8 +70,7 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
 void LocationBarController::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
-  if (action_manager_->GetPageAction(*extension) ||
-      active_script_controller_->GetActionForExtension(extension)) {
+  if (action_manager_->GetPageAction(*extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }

chrome/browser/extensions/location_bar_controller.h

@@ -5,10 +5,11 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_LOCATION_BAR_CONTROLLER_H_
 #define CHROME_BROWSER_EXTENSIONS_LOCATION_BAR_CONTROLLER_H_
 
+#include <map>
 #include <vector>
 
 #include "base/macros.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/memory/linked_ptr.h"
 #include "base/scoped_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
 
@@ -20,8 +21,6 @@ class BrowserContext;
 }
 
 namespace extensions {
-
-class ActiveScriptController;
 class Extension;
 class ExtensionActionManager;
 class ExtensionRegistry;
@@ -36,15 +35,11 @@ class LocationBarController : public ExtensionRegistryObserver {
   // Returns the actions which should be displayed in the location bar.
   std::vector<ExtensionAction*> GetCurrentActions();
 
-  ActiveScriptController* active_script_controller() {
-    return active_script_controller_.get();
-  }
-
  private:
   // ExtensionRegistryObserver implementation.
   virtual void OnExtensionLoaded(
       content::BrowserContext* browser_context,
-      const Extension* extnesion) OVERRIDE;
+      const Extension* extension) OVERRIDE;
   virtual void OnExtensionUnloaded(
       content::BrowserContext* browser_context,
       const Extension* extension,
@@ -63,9 +58,11 @@ class LocationBarController : public ExtensionRegistryObserver {
   // false with the toolbar redesign enabled.)
   bool should_show_page_actions_;
 
-  // The ActiveScriptController, which could also add actions for extensions if
-  // they have a pending script.
-  scoped_ptr<ActiveScriptController> active_script_controller_;
+  // Manufactured page actions that have been generated for extensions that want
+  // to run a script, but were blocked.
+  typedef std::map<std::string, linked_ptr<ExtensionAction> >
+      ExtensionActionMap;
+  ExtensionActionMap active_script_actions_;
 
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;

chrome/browser/extensions/location_bar_controller_unittest.cc

@@ -7,16 +7,20 @@
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "chrome/browser/extensions/active_script_controller.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/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/crx_file/id_util.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
+#include "extensions/common/feature_switch.h"
 #include "extensions/common/value_builder.h"
 
 #if defined(OS_CHROMEOS)
@@ -31,6 +35,9 @@ namespace {
 class LocationBarControllerUnitTest : public ChromeRenderViewHostTestHarness {
  protected:
   virtual void SetUp() OVERRIDE {
+    active_script_override_.reset(new FeatureSwitch::ScopedOverride(
+        FeatureSwitch::scripts_require_action(), true));
+
     ChromeRenderViewHostTestHarness::SetUp();
 #if defined OS_CHROMEOS
   test_user_manager_.reset(new chromeos::ScopedTestUserManager());
@@ -57,6 +64,25 @@ class LocationBarControllerUnitTest : public ChromeRenderViewHostTestHarness {
     return SessionTabHelper::IdForTab(web_contents());
   }
 
+  const Extension* AddExtension(bool has_page_actions,
+                                const std::string& name) {
+    DictionaryBuilder manifest;
+    manifest.Set("name", name)
+            .Set("version", "1.0.0")
+            .Set("manifest_version", 2)
+            .Set("permissions", ListBuilder().Append("tabs"));
+    if (has_page_actions) {
+      manifest.Set("page_action", DictionaryBuilder()
+              .Set("default_title", "Hello"));
+    }
+    scoped_refptr<const Extension> extension =
+        ExtensionBuilder().SetManifest(manifest.Pass())
+                          .SetID(crx_file::id_util::GenerateId(name))
+                          .Build();
+    extension_service_->AddExtension(extension.get());
+    return extension;
+  }
+
   ExtensionService* extension_service_;
 
  private:
@@ -65,30 +91,73 @@ class LocationBarControllerUnitTest : public ChromeRenderViewHostTestHarness {
   chromeos::ScopedTestCrosSettings test_cros_settings_;
   scoped_ptr<chromeos::ScopedTestUserManager> test_user_manager_;
 #endif
+
+  // Since we also test that we show page actions for pending script requests,
+  // we need to enable that feature.
+  scoped_ptr<FeatureSwitch::ScopedOverride> active_script_override_;
 };
 
+// Test that the location bar gets the proper current actions.
+TEST_F(LocationBarControllerUnitTest, LocationBarDisplaysPageActions) {
+  // Load up two extensions, one with a page action and one without.
+  const Extension* page_action = AddExtension(true, "page_actions");
+  const Extension* no_action = AddExtension(false, "no_actions");
+
+  TabHelper* tab_helper = TabHelper::FromWebContents(web_contents());
+  ASSERT_TRUE(tab_helper);
+  LocationBarController* controller = tab_helper->location_bar_controller();
+  ASSERT_TRUE(controller);
+
+  // There should only be one action - the action for the extension with a
+  // page action.
+  std::vector<ExtensionAction*> current_actions =
+      controller->GetCurrentActions();
+  ASSERT_EQ(1u, current_actions.size());
+  EXPECT_EQ(page_action->id(), current_actions[0]->extension_id());
+
+  // If we request a script injection, then the location bar controller should
+  // also show a page action for that extension.
+  ActiveScriptController* active_script_controller =
+      ActiveScriptController::GetForWebContents(web_contents());
+  ASSERT_TRUE(active_script_controller);
+  active_script_controller->RequestScriptInjectionForTesting(no_action,
+                                                             base::Closure());
+  current_actions = controller->GetCurrentActions();
+  ASSERT_EQ(2u, current_actions.size());
+  // Check that each extension is present in the vector.
+  EXPECT_TRUE(current_actions[0]->extension_id() == no_action->id() ||
+              current_actions[1]->extension_id() == no_action->id());
+  EXPECT_TRUE(current_actions[0]->extension_id() == page_action->id() ||
+              current_actions[1]->extension_id() == page_action->id());
+
+  // If we request a script injection for an extension that already has a
+  // page action, only one action should be visible.
+  active_script_controller->RequestScriptInjectionForTesting(page_action,
+                                                             base::Closure());
+  current_actions = controller->GetCurrentActions();
+  ASSERT_EQ(2u, current_actions.size());
+  EXPECT_TRUE(current_actions[0]->extension_id() == no_action->id() ||
+              current_actions[1]->extension_id() == no_action->id());
+  EXPECT_TRUE(current_actions[0]->extension_id() == page_action->id() ||
+              current_actions[1]->extension_id() == page_action->id());
+
+  // Navigating away means that only page actions are shown again.
+  NavigateAndCommit(GURL("http://google.com"));
+  current_actions = controller->GetCurrentActions();
+  ASSERT_EQ(1u, current_actions.size());
+  EXPECT_EQ(page_action->id(), current_actions[0]->extension_id());
+}
+
 // Test that navigating clears all state in a page action.
 TEST_F(LocationBarControllerUnitTest, NavigationClearsState) {
-  scoped_refptr<const Extension> extension =
-      ExtensionBuilder()
-      .SetManifest(DictionaryBuilder()
-                   .Set("name", "Extension with page action")
-                   .Set("version", "1.0.0")
-                   .Set("manifest_version", 2)
-                   .Set("permissions", ListBuilder()
-                        .Append("tabs"))
-                   .Set("page_action", DictionaryBuilder()
-                        .Set("default_title", "Hello")))
-      .Build();
-  extension_service_->AddExtension(extension.get());
+  const Extension* extension = AddExtension(true, "page_actions");
 
   NavigateAndCommit(GURL("http://www.google.com"));
 
   ExtensionAction& page_action =
-      *ExtensionActionManager::Get(profile())->GetPageAction(*extension.get());
+      *ExtensionActionManager::Get(profile())->GetPageAction(*extension);
   page_action.SetTitle(tab_id(), "Goodbye");
-  page_action.SetPopupUrl(
-      tab_id(), extension->GetResourceURL("popup.html"));
+  page_action.SetPopupUrl(tab_id(), extension->GetResourceURL("popup.html"));
 
   EXPECT_EQ("Goodbye", page_action.GetTitle(tab_id()));
   EXPECT_EQ(extension->GetResourceURL("popup.html"),
@@ -106,12 +175,7 @@ TEST_F(LocationBarControllerUnitTest, NavigationClearsState) {
 
   EXPECT_EQ("Hello", page_action.GetTitle(tab_id()));
   EXPECT_EQ(GURL(), page_action.GetPopupUrl(tab_id()));
-};
-
-// TODO(devlin): We should really have more tests for this.
-// NavigationClearsState doesn't test at all that the LocationBarController
-// actually *returns* the proper PageActions in GetCurrentActions. Do we do
-// this elsewhere?
+}
 
 }  // namespace
 }  // namespace extensions

chrome/browser/extensions/tab_helper.cc

@@ -88,6 +88,7 @@ TabHelper::TabHelper(content::WebContents* web_contents)
       script_executor_(
           new ScriptExecutor(web_contents, &script_execution_observers_)),
       location_bar_controller_(new LocationBarController(web_contents)),
+      active_script_controller_(new ActiveScriptController(web_contents)),
       image_loader_ptr_factory_(this),
       webstore_inline_installer_factory_(new WebstoreInlineInstallerFactory()) {
   // The ActiveTabPermissionManager requires a session ID; ensure this

chrome/browser/extensions/tab_helper.h

@@ -38,6 +38,7 @@ class Image;
 }
 
 namespace extensions {
+class ActiveScriptController;
 class BookmarkAppHelper;
 class Extension;
 class LocationBarController;
@@ -109,6 +110,10 @@ class TabHelper : public content::WebContentsObserver,
     return location_bar_controller_.get();
   }
 
+  ActiveScriptController* active_script_controller() {
+    return active_script_controller_.get();
+  }
+
   ActiveTabPermissionGranter* active_tab_permission_granter() {
     return active_tab_permission_granter_.get();
   }
@@ -245,6 +250,8 @@ class TabHelper : public content::WebContentsObserver,
 
   scoped_ptr<LocationBarController> location_bar_controller_;
 
+  scoped_ptr<ActiveScriptController> active_script_controller_;
+
   scoped_ptr<ActiveTabPermissionGranter> active_tab_permission_granter_;
 
   scoped_ptr<BookmarkAppHelper> bookmark_app_helper_;

chrome/browser/ui/cocoa/extensions/extension_action_context_menu_controller.mm

@@ -120,7 +120,7 @@ class AsyncUninstaller : public extensions::ExtensionUninstallDialog::Delegate {
       browser_->tab_strip_model()->GetActiveWebContents();
   if (activeTab &&
       extensions::ActiveScriptController::GetForWebContents(activeTab)
-          ->HasActiveScriptAction(extension_)) {
+          ->WantsToRun(extension_)) {
     item = [menu addItemWithTitle:
                 l10n_util::GetNSStringWithFixup(IDS_EXTENSIONS_ALWAYS_RUN)
                            action:@selector(onAlwaysRun:)