Commit 6e7e5edc authored by rdevlin.cronin's avatar rdevlin.cronin Committed by Commit bot

Stop showing page actions in the location bar with redesign enabled

No longer show page actions in the location bar iff the
extension action redesign switch is enabled. In addition,
make all extensions tests* pass with the switch on! Woot!

*This is based on running these tests locally.  Once
https://codereview.chromium.org/489183005/ goes through,
I can push a run through the trybots with the switch on
by default for kicks and grins.

BUG=397259
BUG=408261

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

Cr-Commit-Position: refs/heads/master@{#292648}
parent e1e99f77
......@@ -61,8 +61,11 @@ bool ShouldRecordExtension(const Extension* extension) {
ActiveScriptController::ActiveScriptController(
content::WebContents* web_contents)
: content::WebContentsObserver(web_contents),
enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()) {
enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()),
extension_registry_observer_(this) {
CHECK(web_contents);
extension_registry_observer_.Add(
ExtensionRegistry::Get(web_contents->GetBrowserContext()));
}
ActiveScriptController::~ActiveScriptController() {
......@@ -169,12 +172,6 @@ ExtensionAction* ActiveScriptController::GetActionForExtension(
return action.get();
}
void ActiveScriptController::OnExtensionUnloaded(const Extension* extension) {
PendingRequestMap::iterator iter = pending_requests_.find(extension->id());
if (iter != pending_requests_.end())
pending_requests_.erase(iter);
}
PermissionsData::AccessType
ActiveScriptController::RequiresUserConsentForScriptInjection(
const Extension* extension,
......@@ -323,16 +320,6 @@ void ActiveScriptController::PermitScriptInjection(int64 request_id) {
}
}
bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
bool handled = true;
IPC_BEGIN_MESSAGE_MAP(ActiveScriptController, message)
IPC_MESSAGE_HANDLER(ExtensionHostMsg_RequestScriptInjectionPermission,
OnRequestScriptInjectionPermission)
IPC_MESSAGE_UNHANDLED(handled = false)
IPC_END_MESSAGE_MAP()
return handled;
}
void ActiveScriptController::LogUMA() const {
UMA_HISTOGRAM_COUNTS_100(
"Extensions.ActiveScriptController.ShownActiveScriptsOnPage",
......@@ -350,6 +337,16 @@ void ActiveScriptController::LogUMA() const {
}
}
bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
bool handled = true;
IPC_BEGIN_MESSAGE_MAP(ActiveScriptController, message)
IPC_MESSAGE_HANDLER(ExtensionHostMsg_RequestScriptInjectionPermission,
OnRequestScriptInjectionPermission)
IPC_MESSAGE_UNHANDLED(handled = false)
IPC_END_MESSAGE_MAP()
return handled;
}
void ActiveScriptController::DidNavigateMainFrame(
const content::LoadCommittedDetails& details,
const content::FrameNavigateParams& params) {
......@@ -361,4 +358,16 @@ void ActiveScriptController::DidNavigateMainFrame(
pending_requests_.clear();
}
void ActiveScriptController::OnExtensionUnloaded(
content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) {
PendingRequestMap::iterator iter = pending_requests_.find(extension->id());
if (iter != pending_requests_.end()) {
pending_requests_.erase(iter);
ExtensionActionAPI::Get(web_contents()->GetBrowserContext())->
NotifyPageActionsChanged(web_contents());
}
}
} // namespace extensions
......@@ -15,6 +15,7 @@
#include "base/memory/linked_ptr.h"
#include "chrome/browser/extensions/location_bar_controller.h"
#include "content/public/browser/web_contents_observer.h"
#include "extensions/browser/extension_registry_observer.h"
#include "extensions/common/permissions/permissions_data.h"
#include "extensions/common/user_script.h"
......@@ -30,12 +31,14 @@ class ExtensionAction;
namespace extensions {
class Extension;
class ExtensionRegistry;
// 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 LocationBar"Controller".
class ActiveScriptController : public content::WebContentsObserver {
class ActiveScriptController : public content::WebContentsObserver,
public ExtensionRegistryObserver {
public:
explicit ActiveScriptController(content::WebContents* web_contents);
virtual ~ActiveScriptController();
......@@ -68,10 +71,6 @@ class ActiveScriptController : public content::WebContentsObserver {
// 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.
PermissionsData::AccessType RequiresUserConsentForScriptInjectionForTesting(
......@@ -113,14 +112,20 @@ class ActiveScriptController : public content::WebContentsObserver {
// Grants permission for the given request to run.
void PermitScriptInjection(int64 request_id);
// Log metrics.
void LogUMA() const;
// 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;
// ExtensionRegistryObserver:
virtual void OnExtensionUnloaded(
content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) OVERRIDE;
// Whether or not the ActiveScriptController is enabled (corresponding to the
// kActiveScriptEnforcement switch). If it is not, it acts as an empty shell,
......@@ -142,6 +147,9 @@ class ActiveScriptController : public content::WebContentsObserver {
typedef std::map<std::string, linked_ptr<ExtensionAction> > ActiveScriptMap;
ActiveScriptMap active_script_actions_;
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
extension_registry_observer_;
DISALLOW_COPY_AND_ASSIGN(ActiveScriptController);
};
......
......@@ -4,12 +4,13 @@
#include "chrome/browser/extensions/extension_action.h"
#include "chrome/browser/extensions/extension_action_manager.h"
#include "chrome/browser/extensions/extension_action_test_util.h"
#include "chrome/browser/extensions/extension_apitest.h"
#include "chrome/browser/extensions/extension_tab_util.h"
#include "chrome/browser/extensions/extension_test_message_listener.h"
#include "chrome/browser/extensions/test_extension_dir.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/location_bar/location_bar.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/extensions/features/feature_channel.h"
#include "content/public/test/browser_test_utils.h"
......@@ -176,10 +177,8 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
EXPECT_TRUE(page_action->GetIsVisible(tab_id));
EXPECT_TRUE(WaitForPageActionVisibilityChangeTo(1));
LocationBarTesting* location_bar =
browser()->window()->GetLocationBar()->GetLocationBarForTesting();
EXPECT_EQ(1, location_bar->PageActionCount());
EXPECT_EQ(1, location_bar->PageActionVisibleCount());
EXPECT_EQ(1u, extension_action_test_util::GetVisiblePageActionCount(tab));
EXPECT_EQ(1u, extension_action_test_util::GetTotalPageActionCount(tab));
ReloadExtension(extension_id); // Invalidates page_action and extension.
EXPECT_EQ("test_rule",
......@@ -188,14 +187,14 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
// navigation.
NavigateInRenderer(tab, GURL("http://test/"));
EXPECT_TRUE(WaitForPageActionVisibilityChangeTo(1));
EXPECT_EQ(1, location_bar->PageActionCount());
EXPECT_EQ(1, location_bar->PageActionVisibleCount());
EXPECT_EQ(1u, extension_action_test_util::GetVisiblePageActionCount(tab));
EXPECT_EQ(1u, extension_action_test_util::GetTotalPageActionCount(tab));
UnloadExtension(extension_id);
NavigateInRenderer(tab, GURL("http://test/"));
EXPECT_TRUE(WaitForPageActionVisibilityChangeTo(0));
EXPECT_EQ(0, location_bar->PageActionCount());
EXPECT_EQ(0, location_bar->PageActionVisibleCount());
EXPECT_EQ(0u, extension_action_test_util::GetVisiblePageActionCount(tab));
EXPECT_EQ(0u, extension_action_test_util::GetTotalPageActionCount(tab));
}
// This tests against a renderer crash that was present during development.
......@@ -275,9 +274,7 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
EXPECT_EQ(NULL,
ExtensionActionManager::Get(browser()->profile())->
GetPageAction(*extension));
EXPECT_EQ(0,
browser()->window()->GetLocationBar()->GetLocationBarForTesting()->
PageActionCount());
EXPECT_EQ(0u, extension_action_test_util::GetVisiblePageActionCount(tab));
}
IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
......
......@@ -171,14 +171,13 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction(
int tab_id = SessionTabHelper::IdForTab(web_contents);
ExtensionAction* extension_action =
ExtensionActionManager::Get(browser_context_)->GetExtensionAction(
*extension);
// Anything that calls this should have a page or browser action.
DCHECK(extension_action);
if (!extension_action->GetIsVisible(tab_id))
return ExtensionAction::ACTION_NONE;
ActiveScriptController* active_script_controller =
ActiveScriptController::GetForWebContents(web_contents);
bool has_pending_scripts = false;
if (active_script_controller &&
active_script_controller->GetActionForExtension(extension)) {
has_pending_scripts = true;
}
// Grant active tab if appropriate.
if (grant_active_tab_permissions) {
......@@ -186,13 +185,20 @@ ExtensionAction::ShowAction ExtensionActionAPI::ExecuteExtensionAction(
GrantIfRequested(extension);
}
// Notify ActiveScriptController that the action was clicked, if appropriate.
ActiveScriptController* active_script_controller =
ActiveScriptController::GetForWebContents(web_contents);
if (active_script_controller &&
active_script_controller->GetActionForExtension(extension)) {
active_script_controller->OnClicked(extension);
}
// If this was a request to run a script, it will have been run once active
// tab was granted. Return without executing the action, since we should only
// run pending scripts OR the extension action, not both.
if (has_pending_scripts)
return ExtensionAction::ACTION_NONE;
ExtensionAction* extension_action =
ExtensionActionManager::Get(browser_context_)->GetExtensionAction(
*extension);
// Anything that gets here should have a page or browser action.
DCHECK(extension_action);
if (!extension_action->GetIsVisible(tab_id))
return ExtensionAction::ACTION_NONE;
if (extension_action->HasPopup(tab_id))
return ExtensionAction::ACTION_SHOW_POPUP;
......
......@@ -6,6 +6,7 @@
#include "chrome/browser/extensions/extension_action.h"
#include "chrome/browser/extensions/extension_action_icon_factory.h"
#include "chrome/browser/extensions/extension_action_manager.h"
#include "chrome/browser/extensions/extension_action_test_util.h"
#include "chrome/browser/extensions/extension_apitest.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/extension_tab_util.h"
......@@ -14,7 +15,6 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/location_bar/location_bar.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/web_contents.h"
......@@ -168,9 +168,8 @@ IN_PROC_BROWSER_TEST_F(PageActionApiTest, DISABLED_ShowPageActionPopup) {
{
ResultCatcher catcher;
LocationBarTesting* location_bar =
browser()->window()->GetLocationBar()->GetLocationBarForTesting();
location_bar->TestPageActionPressed(0);
ExtensionActionAPI::Get(browser()->profile())->ShowExtensionActionPopup(
extension, browser(), true);
ASSERT_TRUE(catcher.GetNextResult());
}
}
......
// Copyright 2014 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/extension_action_test_util.h"
#include "chrome/browser/extensions/extension_action.h"
#include "chrome/browser/extensions/extension_action_manager.h"
#include "chrome/browser/extensions/extension_toolbar_model.h"
#include "chrome/browser/extensions/location_bar_controller.h"
#include "chrome/browser/extensions/tab_helper.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/sessions/session_tab_helper.h"
#include "content/public/browser/web_contents.h"
#include "extensions/common/extension.h"
#include "extensions/common/feature_switch.h"
namespace extensions {
namespace extension_action_test_util {
namespace {
size_t GetPageActionCount(content::WebContents* web_contents,
bool only_count_visible) {
DCHECK(web_contents);
size_t count = 0u;
int tab_id = SessionTabHelper::IdForTab(web_contents);
// Page actions are either stored in the location bar (and provided by the
// LocationBarController), or in the main toolbar (and provided by the
// ExtensionToolbarModel), depending on whether or not the extension action
// redesign is enabled.
if (!FeatureSwitch::extension_action_redesign()->IsEnabled()) {
std::vector<ExtensionAction*> page_actions =
TabHelper::FromWebContents(web_contents)->
location_bar_controller()->GetCurrentActions();
count = page_actions.size();
// Trim any invisible page actions, if necessary.
if (only_count_visible) {
for (std::vector<ExtensionAction*>::iterator iter = page_actions.begin();
iter != page_actions.end(); ++iter) {
if (!(*iter)->GetIsVisible(tab_id))
--count;
}
}
} else {
ExtensionToolbarModel* toolbar_model =
ExtensionToolbarModel::Get(
Profile::FromBrowserContext(web_contents->GetBrowserContext()));
const ExtensionList& toolbar_extensions = toolbar_model->toolbar_items();
ExtensionActionManager* action_manager =
ExtensionActionManager::Get(web_contents->GetBrowserContext());
for (ExtensionList::const_iterator iter = toolbar_extensions.begin();
iter != toolbar_extensions.end(); ++iter) {
ExtensionAction* extension_action = action_manager->GetPageAction(**iter);
if (extension_action &&
(!only_count_visible || extension_action->GetIsVisible(tab_id)))
++count;
}
}
return count;
}
} // namespace
size_t GetVisiblePageActionCount(content::WebContents* web_contents) {
return GetPageActionCount(web_contents, true);
}
size_t GetTotalPageActionCount(content::WebContents* web_contents) {
return GetPageActionCount(web_contents, false);
}
} // namespace extension_action_test_util
} // namespace extensions
// Copyright 2014 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_EXTENSION_ACTION_TEST_UTIL_H_
#define CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_TEST_UTIL_H_
#include "base/basictypes.h"
namespace content {
class WebContents;
}
namespace extensions {
namespace extension_action_test_util {
// TODO(devlin): Should we also pull out methods to test browser actions?
// Returns the number of page actions that are visible in the given
// |web_contents|.
size_t GetVisiblePageActionCount(content::WebContents* web_contents);
// Returns the total number of page actions (visible or not) for the given
// |web_contents|.
size_t GetTotalPageActionCount(content::WebContents* web_contents);
} // namespace extension_action_test_util
} // namespace extensions
#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_ACTION_TEST_UTIL_H_
......@@ -5,10 +5,12 @@
#include "chrome/browser/extensions/extension_test_notification_observer.h"
#include "base/callback_list.h"
#include "chrome/browser/extensions/extension_action_test_util.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/render_view_host.h"
......@@ -26,9 +28,10 @@ namespace {
typedef base::Callback<bool(void)> ConditionCallback;
bool HasPageActionVisibilityReachedTarget(
LocationBarTesting* location_bar, int target_visible_page_action_count) {
return location_bar->PageActionVisibleCount() ==
target_visible_page_action_count;
Browser* browser, size_t target_visible_page_action_count) {
return extensions::extension_action_test_util::GetVisiblePageActionCount(
browser->tab_strip_model()->GetActiveWebContents()) ==
target_visible_page_action_count;
}
bool HaveAllExtensionRenderViewHostsFinishedLoading(
......@@ -125,11 +128,9 @@ void ExtensionTestNotificationObserver::WaitForNotification(
bool ExtensionTestNotificationObserver::WaitForPageActionVisibilityChangeTo(
int count) {
LocationBarTesting* location_bar =
browser_->window()->GetLocationBar()->GetLocationBarForTesting();
extensions::ExtensionActionAPI::Get(GetProfile())->AddObserver(this);
WaitForCondition(
base::Bind(&HasPageActionVisibilityReachedTarget, location_bar, count),
base::Bind(&HasPageActionVisibilityReachedTarget, browser_, count),
NULL);
extensions::ExtensionActionAPI::Get(GetProfile())->
RemoveObserver(this);
......
......@@ -9,6 +9,7 @@
#include "chrome/browser/extensions/extension_action_manager.h"
#include "content/public/browser/web_contents.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/common/feature_switch.h"
namespace extensions {
......@@ -17,9 +18,12 @@ LocationBarController::LocationBarController(
: web_contents_(web_contents),
browser_context_(web_contents->GetBrowserContext()),
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) {
extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
if (should_show_page_actions_)
extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
}
LocationBarController::~LocationBarController() {
......@@ -29,6 +33,9 @@ std::vector<ExtensionAction*> LocationBarController::GetCurrentActions() {
const ExtensionSet& extensions =
ExtensionRegistry::Get(browser_context_)->enabled_extensions();
std::vector<ExtensionAction*> current_actions;
if (!should_show_page_actions_)
return current_actions;
for (ExtensionSet::const_iterator iter = extensions.begin();
iter != extensions.end();
++iter) {
......@@ -59,16 +66,7 @@ void LocationBarController::OnExtensionUnloaded(
content::BrowserContext* browser_context,
const Extension* extension,
UnloadedExtensionInfo::Reason reason) {
bool should_update = false;
if (action_manager_->GetPageAction(*extension))
should_update = true;
if (active_script_controller_->GetActionForExtension(extension)) {
active_script_controller_->OnExtensionUnloaded(extension);
should_update = true;
}
if (should_update) {
if (action_manager_->GetPageAction(*extension)) {
ExtensionActionAPI::Get(browser_context)->
NotifyPageActionsChanged(web_contents_);
}
......
......@@ -59,6 +59,10 @@ class LocationBarController : public ExtensionRegistryObserver {
// The ExtensionActionManager to provide page actions.
ExtensionActionManager* action_manager_;
// Whether or not to show page actions in the location bar at all. (This is
// 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_;
......
......@@ -5,12 +5,12 @@
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/extensions/extension_action.h"
#include "chrome/browser/extensions/extension_action_manager.h"
#include "chrome/browser/extensions/extension_action_test_util.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/extension_tab_util.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/location_bar/location_bar.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/test/base/ui_test_utils.h"
#include "extensions/browser/extension_system.h"
......@@ -108,14 +108,14 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, UnloadPageAction) {
// Navigation prompts the location bar to load page actions.
GURL feed_url = test_server()->GetURL(kFeedPage);
ui_test_utils::NavigateToURL(browser(), feed_url);
LocationBarTesting* location_bar =
browser()->window()->GetLocationBar()->GetLocationBarForTesting();
EXPECT_EQ(1, location_bar->PageActionCount());
content::WebContents* tab =
browser()->tab_strip_model()->GetActiveWebContents();
EXPECT_EQ(1u, extension_action_test_util::GetTotalPageActionCount(tab));
UnloadExtension(last_loaded_extension_id());
// Make sure the page action goes away when it's unloaded.
EXPECT_EQ(0, location_bar->PageActionCount());
EXPECT_EQ(0u, extension_action_test_util::GetTotalPageActionCount(tab));
}
// Tests that we can load page actions in the Omnibox.
......
......@@ -80,6 +80,8 @@
'browser/download/download_test_file_activity_observer.h',
'browser/download/test_download_shelf.cc',
'browser/download/test_download_shelf.h',
'browser/extensions/extension_action_test_util.cc',
'browser/extensions/extension_action_test_util.h',
'browser/invalidation/fake_invalidation_service.cc',
'browser/invalidation/fake_invalidation_service.h',
'browser/media/fake_desktop_media_list.cc',
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment