Remove PageActionsController

Since we've consolidated most of the extension action execution code, this
object is rather unneeded. Remove it.

BUG=407665

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

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

chrome/browser/extensions/active_script_controller.cc

@@ -22,6 +22,7 @@
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
@@ -168,12 +169,6 @@ ExtensionAction* ActiveScriptController::GetActionForExtension(
   return action.get();
 }
 
-void ActiveScriptController::OnNavigated() {
-  LogUMA();
-  permitted_extensions_.clear();
-  pending_requests_.clear();
-}
-
 void ActiveScriptController::OnExtensionUnloaded(const Extension* extension) {
   PendingRequestMap::iterator iter = pending_requests_.find(extension->id());
   if (iter != pending_requests_.end())
@@ -355,4 +350,15 @@ void ActiveScriptController::LogUMA() const {
   }
 }
 
+void ActiveScriptController::DidNavigateMainFrame(
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  if (details.is_in_page)
+    return;
+
+  LogUMA();
+  permitted_extensions_.clear();
+  pending_requests_.clear();
+}
+
 }  // namespace extensions

chrome/browser/extensions/active_script_controller.h

@@ -34,9 +34,8 @@ class Extension;
 // The provider for ExtensionActions corresponding to scripts which are actively
 // running or need permission.
 // TODO(rdevlin.cronin): This isn't really a controller, but it has good parity
-// with PageAction"Controller".
-class ActiveScriptController : public LocationBarController::ActionProvider,
-                               public content::WebContentsObserver {
+// with LocationBar"Controller".
+class ActiveScriptController : public content::WebContentsObserver {
  public:
   explicit ActiveScriptController(content::WebContents* web_contents);
   virtual ~ActiveScriptController();
@@ -65,11 +64,13 @@ class ActiveScriptController : public LocationBarController::ActionProvider,
   // Returns true if there is an active script injection action for |extension|.
   bool HasActiveScriptAction(const Extension* extension);
 
-  // LocationBarControllerProvider implementation.
-  virtual ExtensionAction* GetActionForExtension(
-      const Extension* extension) OVERRIDE;
-  virtual void OnNavigated() OVERRIDE;
-  virtual void OnExtensionUnloaded(const Extension* extension) OVERRIDE;
+  // Returns the action to display for the given |extension|, or NULL if no
+  // action should be displayed.
+  ExtensionAction* GetActionForExtension(const Extension* extension);
+
+  // Notifies that the given |extension| has been unloaded; forwarded from the
+  // ExtensionRegistryObserver method.
+  void OnExtensionUnloaded(const Extension* extension);
 
 #if defined(UNIT_TEST)
   // Only used in tests.
@@ -114,6 +115,9 @@ class ActiveScriptController : public LocationBarController::ActionProvider,
 
   // content::WebContentsObserver implementation.
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void DidNavigateMainFrame(
+      const content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& params) OVERRIDE;
 
   // Log metrics.
   void LogUMA() const;

chrome/browser/extensions/extension_action.cc

@@ -174,7 +174,7 @@ void ExtensionAction::ClearAllValuesForTab(int tab_id) {
   is_visible_.erase(tab_id);
   // TODO(jyasskin): Erase the element from declarative_show_count_
   // when the tab's closed.  There's a race between the
-  // PageActionController and the ContentRulesRegistry on navigation,
+  // LocationBarController and the ContentRulesRegistry on navigation,
   // which prevents me from cleaning everything up now.
 }
 

chrome/browser/extensions/location_bar_controller.cc

@@ -4,12 +4,9 @@
 
 #include "chrome/browser/extensions/location_bar_controller.h"
 
-#include "base/logging.h"
 #include "chrome/browser/extensions/active_script_controller.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
-#include "chrome/browser/extensions/page_action_controller.h"
-#include "chrome/common/extensions/api/extension_action/action_info.h"
-#include "content/public/browser/navigation_details.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_registry.h"
 
@@ -17,13 +14,11 @@ namespace extensions {
 
 LocationBarController::LocationBarController(
     content::WebContents* web_contents)
-    : WebContentsObserver(web_contents),
-      web_contents_(web_contents),
+    : web_contents_(web_contents),
+      browser_context_(web_contents->GetBrowserContext()),
       active_script_controller_(new ActiveScriptController(web_contents_)),
-      page_action_controller_(new PageActionController(web_contents_)),
       extension_registry_observer_(this) {
-  extension_registry_observer_.Add(
-      ExtensionRegistry::Get(web_contents_->GetBrowserContext()));
+  extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
 }
 
 LocationBarController::~LocationBarController() {
@@ -31,8 +26,9 @@ LocationBarController::~LocationBarController() {
 
 std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
   const ExtensionSet& extensions =
-      ExtensionRegistry::Get(web_contents_->GetBrowserContext())
-          ->enabled_extensions();
+      ExtensionRegistry::Get(browser_context_)->enabled_extensions();
+  ExtensionActionManager* action_manager =
+      ExtensionActionManager::Get(browser_context_);
   std::vector<ExtensionAction*> current_actions;
   for (ExtensionSet::const_iterator iter = extensions.begin();
        iter != extensions.end();
@@ -40,8 +36,7 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
     // Right now, we can consolidate these actions because we only want to show
     // 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 =
-        page_action_controller_->GetActionForExtension(iter->get());
+    ExtensionAction* action = action_manager->GetPageAction(**iter);
     if (!action)
       action = active_script_controller_->GetActionForExtension(iter->get());
     if (action)
@@ -51,25 +46,14 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
   return current_actions;
 }
 
-void LocationBarController::DidNavigateMainFrame(
-    const content::LoadCommittedDetails& details,
-    const content::FrameNavigateParams& params) {
-  if (details.is_in_page)
-    return;
-
-  page_action_controller_->OnNavigated();
-  active_script_controller_->OnNavigated();
-}
-
 void LocationBarController::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
   bool should_update = false;
-  if (page_action_controller_->GetActionForExtension(extension)) {
-    page_action_controller_->OnExtensionUnloaded(extension);
+  if (ExtensionActionManager::Get(browser_context_)->GetPageAction(*extension))
     should_update = true;
-  }
+
   if (active_script_controller_->GetActionForExtension(extension)) {
     active_script_controller_->OnExtensionUnloaded(extension);
     should_update = true;
@@ -77,7 +61,7 @@ void LocationBarController::OnExtensionUnloaded(
 
   if (should_update) {
     ExtensionActionAPI::Get(browser_context)->
-        NotifyPageActionsChanged(web_contents());
+        NotifyPageActionsChanged(web_contents_);
   }
 }
 

chrome/browser/extensions/location_bar_controller.h

@@ -10,12 +10,13 @@
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/scoped_observer.h"
-#include "chrome/browser/extensions/extension_action.h"
-#include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
 
+class ExtensionAction;
+
 namespace content {
 class WebContents;
+class BrowserContext;
 }
 
 namespace extensions {
@@ -23,32 +24,11 @@ namespace extensions {
 class ActiveScriptController;
 class Extension;
 class ExtensionRegistry;
-class PageActionController;
 
-// Interface for a class that controls the the extension icons that show up in
-// the location bar. Depending on switches, these icons can have differing
-// behavior.
-class LocationBarController : public content::WebContentsObserver,
-                              public ExtensionRegistryObserver {
+// Provides the UI with the current page actions for extensions. The execution
+// of these actions is handled in the ExtensionActionAPI.
+class LocationBarController : public ExtensionRegistryObserver {
  public:
-  class ActionProvider {
-   public:
-    // Returns the action for the given extension, or NULL if there isn't one.
-    virtual ExtensionAction* GetActionForExtension(
-        const Extension* extension) = 0;
-
-    // A notification that the WebContents has navigated in the main frame (and
-    // not in page), so any state relating to the current page should likely be
-    // reset.
-    virtual void OnNavigated() = 0;
-
-    // A notification that the given |extension| has been unloaded, and any
-    // actions associated with it should be removed.
-    // The LocationBarController will handle notifying of page action changes,
-    // if any.
-    virtual void OnExtensionUnloaded(const Extension* extension) {}
-  };
-
   explicit LocationBarController(content::WebContents* web_contents);
   virtual ~LocationBarController();
 
@@ -60,11 +40,6 @@ class LocationBarController : public content::WebContentsObserver,
   }
 
  private:
-  // content::WebContentsObserver implementation.
-  virtual void DidNavigateMainFrame(
-      const content::LoadCommittedDetails& details,
-      const content::FrameNavigateParams& params) OVERRIDE;
-
   // ExtensionRegistryObserver implementation.
   virtual void OnExtensionUnloaded(
       content::BrowserContext* browser_context,
@@ -74,13 +49,12 @@ class LocationBarController : public content::WebContentsObserver,
   // The associated WebContents.
   content::WebContents* web_contents_;
 
-  // The controllers for different sources of actions in the location bar.
-  // Currently, this is only page actions and active script actions, so we
-  // explicitly own and create both. If there are ever more, it will be worth
-  // considering making this class own a list of LocationBarControllerProviders
-  // instead.
+  // The associated BrowserContext.
+  content::BrowserContext* browser_context_;
+
+  // The ActiveScriptController, which could also add actions for extensions if
+  // they have a pending script.
   scoped_ptr<ActiveScriptController> active_script_controller_;
-  scoped_ptr<PageActionController> page_action_controller_;
 
   ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
       extension_registry_observer_;

chrome/browser/extensions/page_action_controller_unittest.cc → chrome/browser/extensions/location_bar_controller_unittest.cc

@@ -10,7 +10,6 @@
 #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/page_action_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"
@@ -29,7 +28,7 @@
 namespace extensions {
 namespace {
 
-class PageActionControllerTest : public ChromeRenderViewHostTestHarness {
+class LocationBarControllerUnitTest : public ChromeRenderViewHostTestHarness {
  protected:
   virtual void SetUp() OVERRIDE {
     ChromeRenderViewHostTestHarness::SetUp();
@@ -37,7 +36,7 @@ class PageActionControllerTest : public ChromeRenderViewHostTestHarness {
   test_user_manager_.reset(new chromeos::ScopedTestUserManager());
 #endif
     TabHelper::CreateForWebContents(web_contents());
-    // Create an ExtensionService so the PageActionController can find its
+    // Create an ExtensionService so the LocationBarController can find its
     // extensions.
     CommandLine command_line(CommandLine::NO_PROGRAM);
     Profile* profile =
@@ -68,7 +67,8 @@ class PageActionControllerTest : public ChromeRenderViewHostTestHarness {
 #endif
 };
 
-TEST_F(PageActionControllerTest, NavigationClearsState) {
+// Test that navigating clears all state in a page action.
+TEST_F(LocationBarControllerUnitTest, NavigationClearsState) {
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
       .SetManifest(DictionaryBuilder()
@@ -108,5 +108,10 @@ TEST_F(PageActionControllerTest, NavigationClearsState) {
   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/page_action_controller.cc

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/extensions/page_action_controller.h"
-
-#include "chrome/browser/extensions/extension_action_manager.h"
-#include "content/public/browser/web_contents.h"
-
-namespace extensions {
-
-PageActionController::PageActionController(content::WebContents* web_contents)
-    : web_contents_(web_contents),
-      browser_context_(web_contents_->GetBrowserContext()) {
-}
-
-PageActionController::~PageActionController() {
-}
-
-ExtensionAction* PageActionController::GetActionForExtension(
-    const Extension* extension) {
-  return ExtensionActionManager::Get(browser_context_)->GetPageAction(
-      *extension);
-}
-
-void PageActionController::OnNavigated() {
-  // Clearing extension action values is handled in TabHelper, so nothing to
-  // do here.
-}
-
-}  // namespace extensions

chrome/browser/extensions/page_action_controller.h

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_EXTENSIONS_PAGE_ACTION_CONTROLLER_H_
-#define CHROME_BROWSER_EXTENSIONS_PAGE_ACTION_CONTROLLER_H_
-
-#include "base/macros.h"
-#include "chrome/browser/extensions/location_bar_controller.h"
-
-namespace content {
-class BrowserContext;
-class WebContents;
-}
-
-namespace extensions {
-class Extension;
-
-// A LocationBarControllerProvider which populates the location bar with icons
-// based on the page_action extension API.
-// TODO(rdevlin.cronin): This isn't really a controller.
-class PageActionController : public LocationBarController::ActionProvider {
- public:
-  explicit PageActionController(content::WebContents* web_contents);
-  virtual ~PageActionController();
-
-  // LocationBarController::Provider implementation.
-  virtual ExtensionAction* GetActionForExtension(const Extension* extension)
-      OVERRIDE;
-  virtual void OnNavigated() OVERRIDE;
-
- private:
-  // The associated WebContents.
-  content::WebContents* web_contents_;
-
-  // The associated browser context.
-  content::BrowserContext* browser_context_;
-
-  DISALLOW_COPY_AND_ASSIGN(PageActionController);
-};
-
-}  // namespace extensions
-
-#endif  // CHROME_BROWSER_EXTENSIONS_PAGE_ACTION_CONTROLLER_H_

chrome/chrome_browser_extensions.gypi

@@ -822,8 +822,6 @@
       'browser/extensions/ntp_overridden_bubble_controller.h',
       'browser/extensions/pack_extension_job.cc',
       'browser/extensions/pack_extension_job.h',
-      'browser/extensions/page_action_controller.cc',
-      'browser/extensions/page_action_controller.h',
       'browser/extensions/path_util.cc',
       'browser/extensions/path_util.h',
       'browser/extensions/pending_enables.cc',

chrome/chrome_tests_unit.gypi

@@ -959,9 +959,9 @@
         'browser/extensions/external_provider_impl_chromeos_unittest.cc',
         'browser/extensions/favicon_downloader_unittest.cc',
         'browser/extensions/install_tracker_unittest.cc',
+        'browser/extensions/location_bar_controller_unittest.cc',
         'browser/extensions/menu_manager_unittest.cc',
         'browser/extensions/pack_extension_unittest.cc',
-        'browser/extensions/page_action_controller_unittest.cc',
         'browser/extensions/permissions_updater_unittest.cc',
         'browser/extensions/policy_handlers_unittest.cc',
         'browser/extensions/sandboxed_unpacker_unittest.cc',
@@ -2098,9 +2098,9 @@
             'browser/extensions/extension_error_controller_unittest.cc',
             'browser/extensions/extension_ui_unittest.cc',
             'browser/extensions/extension_web_ui_unittest.cc',
+            'browser/extensions/location_bar_controller_unittest.cc',
             'browser/extensions/menu_manager_unittest.cc',
             'browser/extensions/pack_extension_unittest.cc',
-            'browser/extensions/page_action_controller_unittest.cc',
             'browser/extensions/permissions_updater_unittest.cc',
             'browser/extensions/sandboxed_unpacker_unittest.cc',
             'browser/extensions/updater/extension_updater_unittest.cc',