From a9a17da0de2260f6cb1a10b542a6d3a2b5fbf504 Mon Sep 17 00:00:00 2001 From: "rdevlin.cronin" Date: Fri, 10 Apr 2015 16:57:15 -0700 Subject: [PATCH] [Extensions] Make extension message bubble factory platform-abstract We want to show extension message bubbles on multiple platforms; the first step towards this is to make the bubble factory platform-abstract. Do so, and as an added bonus, shave off 200 lines of code. BUG=474092 Review URL: https://codereview.chromium.org/1078543002 Cr-Commit-Position: refs/heads/master@{#324715} --- .../extensions/dev_mode_bubble_controller.cc | 5 + .../extensions/extension_message_bubble.h | 10 - .../extension_message_bubble_controller.cc | 23 +- .../extension_message_bubble_controller.h | 7 + ...sion_message_bubble_controller_unittest.cc | 94 +++--- .../ntp_overridden_bubble_controller.cc | 5 + .../proxy_overridden_bubble_controller.cc | 5 + .../settings_api_bubble_controller.cc | 27 +- .../settings_api_bubble_controller.h | 7 +- .../suspicious_extension_bubble_controller.cc | 8 +- .../extensions/browser_actions_controller.mm | 8 + chrome/browser/ui/extensions/OWNERS | 5 +- .../extension_message_bubble_factory.cc | 92 ++++++ .../extension_message_bubble_factory.h | 34 +++ .../browser/ui/toolbar/toolbar_actions_bar.cc | 20 +- .../browser/ui/toolbar/toolbar_actions_bar.h | 7 + .../ui/toolbar/toolbar_actions_bar_delegate.h | 9 + .../extension_message_bubble_view.cc | 278 +----------------- .../extension_message_bubble_view.h | 125 -------- .../views/settings_api_bubble_helper_views.cc | 40 +-- .../toolbar/browser_actions_container.cc | 29 ++ .../views/toolbar/browser_actions_container.h | 7 + .../browser/ui/views/toolbar/toolbar_view.cc | 14 +- .../browser/ui/views/toolbar/toolbar_view.h | 7 - chrome/chrome_browser_ui.gypi | 2 + 25 files changed, 342 insertions(+), 526 deletions(-) create mode 100644 chrome/browser/ui/extensions/extension_message_bubble_factory.cc create mode 100644 chrome/browser/ui/extensions/extension_message_bubble_factory.h diff --git a/chrome/browser/extensions/dev_mode_bubble_controller.cc b/chrome/browser/extensions/dev_mode_bubble_controller.cc index 202207ca5783c6..d6914e503a8352 100644 --- a/chrome/browser/extensions/dev_mode_bubble_controller.cc +++ b/chrome/browser/extensions/dev_mode_bubble_controller.cc @@ -52,6 +52,7 @@ class DevModeBubbleDelegate base::string16 GetActionButtonLabel() const override; base::string16 GetDismissButtonLabel() const override; bool ShouldShowExtensionList() const override; + bool ShouldHighlightExtensions() const override; void LogExtensionCount(size_t count) override; void LogAction( ExtensionMessageBubbleController::BubbleAction action) override; @@ -127,6 +128,10 @@ bool DevModeBubbleDelegate::ShouldShowExtensionList() const { return false; } +bool DevModeBubbleDelegate::ShouldHighlightExtensions() const { + return true; +} + void DevModeBubbleDelegate::LogExtensionCount(size_t count) { UMA_HISTOGRAM_COUNTS_100( "ExtensionBubble.ExtensionsInDevModeCount", count); diff --git a/chrome/browser/extensions/extension_message_bubble.h b/chrome/browser/extensions/extension_message_bubble.h index 71d6e30eb8c587..3819a82f8c1692 100644 --- a/chrome/browser/extensions/extension_message_bubble.h +++ b/chrome/browser/extensions/extension_message_bubble.h @@ -15,16 +15,6 @@ namespace extensions { // controller. class ExtensionMessageBubble { public: - // Setup the callback for when the action button is clicked in the - // bubble. - virtual void OnActionButtonClicked(const base::Closure& callback) = 0; - - // Setup the callback for when the dismiss button is clicked. - virtual void OnDismissButtonClicked(const base::Closure& callback) = 0; - - // Setup the callback for when the link is clicked in the bubble. - virtual void OnLinkClicked(const base::Closure& callback) = 0; - // Instruct the bubble to appear. virtual void Show() = 0; }; diff --git a/chrome/browser/extensions/extension_message_bubble_controller.cc b/chrome/browser/extensions/extension_message_bubble_controller.cc index 778c7ad2237579..a30b1f0756dad6 100644 --- a/chrome/browser/extensions/extension_message_bubble_controller.cc +++ b/chrome/browser/extensions/extension_message_bubble_controller.cc @@ -8,6 +8,7 @@ #include "base/metrics/histogram.h" #include "base/strings/utf_string_conversions.h" #include "chrome/browser/extensions/extension_message_bubble.h" +#include "chrome/browser/extensions/extension_toolbar_model.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" @@ -122,21 +123,15 @@ const ExtensionIdList& ExtensionMessageBubbleController::GetExtensionIdList() { bool ExtensionMessageBubbleController::CloseOnDeactivate() { return false; } -void ExtensionMessageBubbleController::Show(ExtensionMessageBubble* bubble) { - // Wire up all the callbacks, to get notified what actions the user took. - base::Closure dismiss_button_callback = - base::Bind(&ExtensionMessageBubbleController::OnBubbleDismiss, - base::Unretained(this)); - base::Closure action_button_callback = - base::Bind(&ExtensionMessageBubbleController::OnBubbleAction, - base::Unretained(this)); - base::Closure link_callback = - base::Bind(&ExtensionMessageBubbleController::OnLinkClicked, - base::Unretained(this)); - bubble->OnActionButtonClicked(action_button_callback); - bubble->OnDismissButtonClicked(dismiss_button_callback); - bubble->OnLinkClicked(link_callback); +void ExtensionMessageBubbleController::HighlightExtensionsIfNecessary() { + if (delegate_->ShouldHighlightExtensions()) { + const ExtensionIdList& extension_ids = GetExtensionIdList(); + DCHECK(!extension_ids.empty()); + ExtensionToolbarModel::Get(profile_)->HighlightExtensions(extension_ids); + } +} +void ExtensionMessageBubbleController::Show(ExtensionMessageBubble* bubble) { bubble->Show(); } diff --git a/chrome/browser/extensions/extension_message_bubble_controller.h b/chrome/browser/extensions/extension_message_bubble_controller.h index d932e1dc219d26..e56ff0c552acdc 100644 --- a/chrome/browser/extensions/extension_message_bubble_controller.h +++ b/chrome/browser/extensions/extension_message_bubble_controller.h @@ -59,6 +59,10 @@ class ExtensionMessageBubbleController { // Whether to show a list of extensions in the bubble. virtual bool ShouldShowExtensionList() const = 0; + // Returns true if the set of affected extensions should be highlighted in + // the toolbar. + virtual bool ShouldHighlightExtensions() const = 0; + // In some cases, we want the delegate only to handle a single extension // and this sets which extension. virtual void RestrictToSingleExtension(const std::string& extension_id); @@ -101,6 +105,9 @@ class ExtensionMessageBubbleController { // Whether to close the bubble when it loses focus. virtual bool CloseOnDeactivate(); + // Highlights the affected extensions if appropriate. + void HighlightExtensionsIfNecessary(); + // Sets up the callbacks and shows the bubble. virtual void Show(ExtensionMessageBubble* bubble); diff --git a/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc b/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc index 40f7c48061ac18..1a500921b86ce3 100644 --- a/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc +++ b/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc @@ -33,6 +33,12 @@ #include "extensions/common/value_builder.h" #include "testing/gtest/include/gtest/gtest.h" +#if defined(OS_CHROMEOS) +#include "chrome/browser/chromeos/login/users/scoped_test_user_manager.h" +#include "chrome/browser/chromeos/settings/cros_settings.h" +#include "chrome/browser/chromeos/settings/device_settings_service.h" +#endif + namespace { const char kId1[] = "iccfkkhkfiphcjdakkmcjmkfboccmndk"; @@ -207,39 +213,29 @@ class FakeExtensionMessageBubble : public ExtensionMessageBubble { BUBBLE_ACTION_CLICK_LINK, }; - FakeExtensionMessageBubble() {} + FakeExtensionMessageBubble() : controller_(nullptr) {} void set_action_on_show(ExtensionBubbleAction action) { action_ = action; } + void set_controller(ExtensionMessageBubbleController* controller) { + controller_ = controller; + } void Show() override { if (action_ == BUBBLE_ACTION_CLICK_ACTION_BUTTON) - action_callback_.Run(); + controller_->OnBubbleAction(); else if (action_ == BUBBLE_ACTION_CLICK_DISMISS_BUTTON) - dismiss_callback_.Run(); + controller_->OnBubbleDismiss(); else if (action_ == BUBBLE_ACTION_CLICK_LINK) - link_callback_.Run(); - } - - void OnActionButtonClicked(const base::Closure& callback) override { - action_callback_ = callback; - } - - void OnDismissButtonClicked(const base::Closure& callback) override { - dismiss_callback_ = callback; - } - - void OnLinkClicked(const base::Closure& callback) override { - link_callback_ = callback; + controller_->OnLinkClicked(); } private: ExtensionBubbleAction action_; + ExtensionMessageBubbleController* controller_; - base::Closure action_callback_; - base::Closure dismiss_callback_; - base::Closure link_callback_; + DISALLOW_COPY_AND_ASSIGN(FakeExtensionMessageBubble); }; class ExtensionMessageBubbleTest : public testing::Test { @@ -390,7 +386,6 @@ class ExtensionMessageBubbleTest : public testing::Test { void Init() { // The two lines of magical incantation required to get the extension // service to work inside a unit test and access the extension prefs. - thread_bundle_.reset(new content::TestBrowserThreadBundle); profile_.reset(new TestingProfile); static_cast(ExtensionSystem::Get(profile())) ->CreateExtensionService(base::CommandLine::ForCurrentProcess(), @@ -423,21 +418,22 @@ class ExtensionMessageBubbleTest : public testing::Test { ExtensionService* service_; private: + content::TestBrowserThreadBundle thread_bundle_; scoped_ptr command_line_; - scoped_ptr thread_bundle_; scoped_ptr profile_; +#if defined OS_CHROMEOS + chromeos::ScopedTestDeviceSettingsService test_device_settings_service_; + chromeos::ScopedTestCrosSettings test_cros_settings_; + chromeos::ScopedTestUserManager test_user_manager_; +#endif + DISALLOW_COPY_AND_ASSIGN(ExtensionMessageBubbleTest); }; -// The feature this is meant to test is only implemented on Windows. -#if defined(OS_WIN) -#define MAYBE_WipeoutControllerTest WipeoutControllerTest -#else -#define MAYBE_WipeoutControllerTest DISABLED_WipeoutControllerTest -#endif - -TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { +// The feature this is meant to test is only enacted on Windows, but it should +// pass on all platforms. +TEST_F(ExtensionMessageBubbleTest, WipeoutControllerTest) { Init(); // Add three extensions, and control two of them in this test (extension 1 // and 2). @@ -474,6 +470,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { suspicious_extensions = controller->GetExtensionList(); ASSERT_EQ(1U, suspicious_extensions.size()); EXPECT_TRUE(base::ASCIIToUTF16("Extension 1") == suspicious_extensions[0]); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->dismiss_click_count()); @@ -497,20 +494,16 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { ASSERT_EQ(2U, suspicious_extensions.size()); EXPECT_TRUE(base::ASCIIToUTF16("Extension 1") == suspicious_extensions[1]); EXPECT_TRUE(base::ASCIIToUTF16("Extension 2") == suspicious_extensions[0]); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->dismiss_click_count()); EXPECT_TRUE(controller->delegate()->HasBubbleInfoBeenAcknowledged(kId1)); } -// The feature this is meant to test is only implemented on Windows. -#if defined(OS_WIN) -#define MAYBE_DevModeControllerTest DevModeControllerTest -#else -#define MAYBE_DevModeControllerTest DISABLED_DevModeControllerTest -#endif - -TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { +// The feature this is meant to test is only enacted on Windows, but it should +// pass on all platforms. +TEST_F(ExtensionMessageBubbleTest, DevModeControllerTest) { FeatureSwitch::ScopedOverride force_dev_mode_highlighting( FeatureSwitch::force_dev_mode_highlighting(), true); Init(); @@ -539,6 +532,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { FakeExtensionMessageBubble bubble; bubble.set_action_on_show( FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_DISMISS_BUTTON); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -556,6 +550,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { EXPECT_TRUE(controller->ShouldShow()); dev_mode_extensions = controller->GetExtensionList(); EXPECT_EQ(2U, dev_mode_extensions.size()); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->action_click_count()); @@ -576,6 +571,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { EXPECT_TRUE(controller->ShouldShow()); dev_mode_extensions = controller->GetExtensionList(); EXPECT_EQ(2U, dev_mode_extensions.size()); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -643,7 +639,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_SettingsApiControllerTest) { profile(), static_cast(i))); // The list will contain one enabled unpacked extension (ext 2). - EXPECT_TRUE(controller->ShouldShow(kId2)); + EXPECT_TRUE(controller->ShouldShow()); std::vector override_extensions = controller->GetExtensionList(); ASSERT_EQ(1U, override_extensions.size()); @@ -657,6 +653,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_SettingsApiControllerTest) { FakeExtensionMessageBubble bubble; bubble.set_action_on_show( FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_DISMISS_BUTTON); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -678,6 +675,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_SettingsApiControllerTest) { FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_LINK); controller.reset(new TestSettingsApiBubbleController( profile(), static_cast(i))); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -698,9 +696,10 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_SettingsApiControllerTest) { FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_ACTION_BUTTON); controller.reset(new TestSettingsApiBubbleController( profile(), static_cast(i))); - EXPECT_TRUE(controller->ShouldShow(kId2)); + EXPECT_TRUE(controller->ShouldShow()); override_extensions = controller->GetExtensionList(); EXPECT_EQ(1U, override_extensions.size()); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->action_click_count()); @@ -730,14 +729,9 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_SettingsApiControllerTest) { } } -// The feature this is meant to test is only implemented on Windows. -#if defined(OS_WIN) -#define MAYBE_NtpOverriddenControllerTest NtpOverriddenControllerTest -#else -#define MAYBE_NtpOverriddenControllerTest DISABLED_NtpOverriddenControllerTest -#endif - -TEST_F(ExtensionMessageBubbleTest, MAYBE_NtpOverriddenControllerTest) { +// The feature this is meant to test is only enacted on Windows, but it should +// pass on all platforms. +TEST_F(ExtensionMessageBubbleTest, NtpOverriddenControllerTest) { Init(); // Load two extensions overriding new tab page and one overriding something // unrelated (to check for interference). Extension 2 should still win @@ -765,6 +759,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_NtpOverriddenControllerTest) { bubble.set_action_on_show( FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_DISMISS_BUTTON); EXPECT_TRUE(controller->ShouldShow(kId2)); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -786,6 +781,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_NtpOverriddenControllerTest) { FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_LINK); controller.reset(new TestNtpOverriddenBubbleController(profile())); EXPECT_TRUE(controller->ShouldShow(kId2)); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -808,6 +804,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_NtpOverriddenControllerTest) { EXPECT_TRUE(controller->ShouldShow(kId2)); override_extensions = controller->GetExtensionList(); EXPECT_EQ(1U, override_extensions.size()); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->action_click_count()); @@ -895,6 +892,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_ProxyOverriddenControllerTest) { FakeExtensionMessageBubble bubble; bubble.set_action_on_show( FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_DISMISS_BUTTON); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -916,6 +914,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_ProxyOverriddenControllerTest) { FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_LINK); controller.reset(new TestProxyOverriddenBubbleController(profile())); EXPECT_TRUE(controller->ShouldShow(kId2)); + bubble.set_controller(controller.get()); controller->Show(&bubble); EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); @@ -938,6 +937,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_ProxyOverriddenControllerTest) { EXPECT_TRUE(controller->ShouldShow(kId2)); override_extensions = controller->GetExtensionList(); EXPECT_EQ(1U, override_extensions.size()); + bubble.set_controller(controller.get()); controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->action_click_count()); diff --git a/chrome/browser/extensions/ntp_overridden_bubble_controller.cc b/chrome/browser/extensions/ntp_overridden_bubble_controller.cc index bec4ed3c628957..317b7bae84dc03 100644 --- a/chrome/browser/extensions/ntp_overridden_bubble_controller.cc +++ b/chrome/browser/extensions/ntp_overridden_bubble_controller.cc @@ -47,6 +47,7 @@ class NtpOverriddenBubbleDelegate base::string16 GetActionButtonLabel() const override; base::string16 GetDismissButtonLabel() const override; bool ShouldShowExtensionList() const override; + bool ShouldHighlightExtensions() const override; void RestrictToSingleExtension(const std::string& extension_id) override; void LogExtensionCount(size_t count) override; void LogAction(extensions::ExtensionMessageBubbleController::BubbleAction @@ -142,6 +143,10 @@ bool NtpOverriddenBubbleDelegate::ShouldShowExtensionList() const { return false; } +bool NtpOverriddenBubbleDelegate::ShouldHighlightExtensions() const { + return false; +} + void NtpOverriddenBubbleDelegate::RestrictToSingleExtension( const std::string& extension_id) { extension_id_ = extension_id; diff --git a/chrome/browser/extensions/proxy_overridden_bubble_controller.cc b/chrome/browser/extensions/proxy_overridden_bubble_controller.cc index a3cde4cacb2f36..d25204d58bdf09 100644 --- a/chrome/browser/extensions/proxy_overridden_bubble_controller.cc +++ b/chrome/browser/extensions/proxy_overridden_bubble_controller.cc @@ -51,6 +51,7 @@ class ProxyOverriddenBubbleDelegate base::string16 GetActionButtonLabel() const override; base::string16 GetDismissButtonLabel() const override; bool ShouldShowExtensionList() const override; + bool ShouldHighlightExtensions() const override; void RestrictToSingleExtension(const std::string& extension_id) override; void LogExtensionCount(size_t count) override; void LogAction( @@ -161,6 +162,10 @@ bool ProxyOverriddenBubbleDelegate::ShouldShowExtensionList() const { return false; } +bool ProxyOverriddenBubbleDelegate::ShouldHighlightExtensions() const { + return true; +} + void ProxyOverriddenBubbleDelegate::RestrictToSingleExtension( const std::string& extension_id) { extension_id_ = extension_id; diff --git a/chrome/browser/extensions/settings_api_bubble_controller.cc b/chrome/browser/extensions/settings_api_bubble_controller.cc index e89f42f1290331..e90cf936acd09c 100644 --- a/chrome/browser/extensions/settings_api_bubble_controller.cc +++ b/chrome/browser/extensions/settings_api_bubble_controller.cc @@ -54,6 +54,7 @@ class SettingsApiBubbleDelegate base::string16 GetActionButtonLabel() const override; base::string16 GetDismissButtonLabel() const override; bool ShouldShowExtensionList() const override; + bool ShouldHighlightExtensions() const override; void LogExtensionCount(size_t count) override; void LogAction( ExtensionMessageBubbleController::BubbleAction action) override; @@ -244,6 +245,10 @@ bool SettingsApiBubbleDelegate::ShouldShowExtensionList() const { return false; } +bool SettingsApiBubbleDelegate::ShouldHighlightExtensions() const { + return type_ == BUBBLE_TYPE_STARTUP_PAGES; +} + void SettingsApiBubbleDelegate::LogExtensionCount(size_t count) { } @@ -290,11 +295,27 @@ SettingsApiBubbleController::SettingsApiBubbleController( SettingsApiBubbleController::~SettingsApiBubbleController() {} -bool SettingsApiBubbleController::ShouldShow(const std::string& extension_id) { - if (delegate()->HasBubbleInfoBeenAcknowledged(extension_id)) +bool SettingsApiBubbleController::ShouldShow() { + const Extension* extension = nullptr; + switch (type_) { + case BUBBLE_TYPE_HOME_PAGE: + extension = GetExtensionOverridingHomepage(profile_); + break; + case BUBBLE_TYPE_SEARCH_ENGINE: + extension = GetExtensionOverridingSearchEngine(profile_); + break; + case BUBBLE_TYPE_STARTUP_PAGES: + extension = GetExtensionOverridingStartupPages(profile_); + break; + } + + if (!extension) + return false; + + if (delegate()->HasBubbleInfoBeenAcknowledged(extension->id())) return false; - if (!delegate()->ShouldIncludeExtension(extension_id)) + if (!delegate()->ShouldIncludeExtension(extension->id())) return false; // If the browser is showing the 'Chrome crashed' infobar, it won't be showing diff --git a/chrome/browser/extensions/settings_api_bubble_controller.h b/chrome/browser/extensions/settings_api_bubble_controller.h index df64077981f85a..42095330dc4e33 100644 --- a/chrome/browser/extensions/settings_api_bubble_controller.h +++ b/chrome/browser/extensions/settings_api_bubble_controller.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_EXTENSIONS_SETTINGS_API_BUBBLE_CONTROLLER_H_ #include + #include "chrome/browser/extensions/extension_message_bubble_controller.h" #include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h" @@ -18,9 +19,9 @@ class SettingsApiBubbleController : public ExtensionMessageBubbleController { SettingsApiBubbleController(Profile* profile, SettingsApiOverrideType type); ~SettingsApiBubbleController() override; - // Whether the controller knows that we should show the bubble for extension - // with |extension_id|. Returns true if so. - bool ShouldShow(const std::string& extension_id); + // Returns true if we should show the bubble for the extension actively + // overriding the setting of |type_|. + bool ShouldShow(); // ExtensionMessageBubbleController: bool CloseOnDeactivate() override; diff --git a/chrome/browser/extensions/suspicious_extension_bubble_controller.cc b/chrome/browser/extensions/suspicious_extension_bubble_controller.cc index da364b2781e949..94d189fe350ca6 100644 --- a/chrome/browser/extensions/suspicious_extension_bubble_controller.cc +++ b/chrome/browser/extensions/suspicious_extension_bubble_controller.cc @@ -53,6 +53,7 @@ class SuspiciousExtensionBubbleDelegate base::string16 GetActionButtonLabel() const override; base::string16 GetDismissButtonLabel() const override; bool ShouldShowExtensionList() const override; + bool ShouldHighlightExtensions() const override; void LogExtensionCount(size_t count) override; void LogAction( ExtensionMessageBubbleController::BubbleAction action) override; @@ -126,11 +127,14 @@ SuspiciousExtensionBubbleDelegate::GetDismissButtonLabel() const { return l10n_util::GetStringUTF16(IDS_EXTENSIONS_UNSUPPORTED_DISABLED_BUTTON); } -bool -SuspiciousExtensionBubbleDelegate::ShouldShowExtensionList() const { +bool SuspiciousExtensionBubbleDelegate::ShouldShowExtensionList() const { return true; } +bool SuspiciousExtensionBubbleDelegate::ShouldHighlightExtensions() const { + return false; +} + void SuspiciousExtensionBubbleDelegate::LogExtensionCount( size_t count) { UMA_HISTOGRAM_COUNTS_100( diff --git a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm index dc640a18c3ed71..19f9ac8aea7d24 100644 --- a/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm +++ b/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm @@ -171,6 +171,9 @@ void ResizeAndAnimate(gfx::Tween::Type tween_type, bool IsPopupRunning() const override; void OnOverflowedActionWantsToRunChanged(bool overflowed_action_wants_to_run) override; + void ShowExtensionMessageBubble( + scoped_ptr controller) + override; // The owning BrowserActionsController; weak. BrowserActionsController* controller_; @@ -248,6 +251,11 @@ void OnOverflowedActionWantsToRunChanged(bool overflowed_action_wants_to_run) setOverflowedToolbarActionWantsToRun:overflowed_action_wants_to_run]; } +void ToolbarActionsBarBridge::ShowExtensionMessageBubble( + scoped_ptr controller) { + NOTREACHED(); // Not yet implemented on Mac. +} + } // namespace @implementation BrowserActionsController diff --git a/chrome/browser/ui/extensions/OWNERS b/chrome/browser/ui/extensions/OWNERS index 98867c413c705a..e7cdf1afa6357a 100644 --- a/chrome/browser/ui/extensions/OWNERS +++ b/chrome/browser/ui/extensions/OWNERS @@ -1,6 +1,9 @@ benwells@chromium.org miket@chromium.org -# Extension/Toolbar Actions Files per-file extension_action*=finnur@chromium.org per-file extension_action*=rdevlin.cronin@chromium.org +per-file extension_installed_bubble*=finnur@chromium.org +per-file extension_installed_bubble*=rdevlin.cronin@chromium.org +per-file extension_message_bubble*=finnur@chromium.org +per-file extension_message_bubble*=rdevlin.cronin@chromium.org diff --git a/chrome/browser/ui/extensions/extension_message_bubble_factory.cc b/chrome/browser/ui/extensions/extension_message_bubble_factory.cc new file mode 100644 index 00000000000000..8737602256e502 --- /dev/null +++ b/chrome/browser/ui/extensions/extension_message_bubble_factory.cc @@ -0,0 +1,92 @@ +// Copyright 2015 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/ui/extensions/extension_message_bubble_factory.h" + +#include "base/lazy_instance.h" +#include "chrome/browser/extensions/dev_mode_bubble_controller.h" +#include "chrome/browser/extensions/extension_message_bubble_controller.h" +#include "chrome/browser/extensions/proxy_overridden_bubble_controller.h" +#include "chrome/browser/extensions/settings_api_bubble_controller.h" +#include "chrome/browser/extensions/settings_api_helpers.h" +#include "chrome/browser/extensions/suspicious_extension_bubble_controller.h" +#include "chrome/browser/profiles/profile.h" + +namespace { + +// A map of all profiles evaluated, so we can tell if it's the initial check. +// TODO(devlin): It would be nice to coalesce all the "profiles evaluated" maps +// that are in the different bubble controllers. +base::LazyInstance > g_profiles_evaluated = + LAZY_INSTANCE_INITIALIZER; + +} + +ExtensionMessageBubbleFactory::ExtensionMessageBubbleFactory(Profile* profile) + : profile_(profile) { +} + +ExtensionMessageBubbleFactory::~ExtensionMessageBubbleFactory() { +} + +scoped_ptr +ExtensionMessageBubbleFactory::GetController() { +// Currently only on windows. +#if !defined(OS_WIN) + return scoped_ptr(); +#endif + + Profile* original_profile = profile_->GetOriginalProfile(); + std::set& profiles_evaluated = g_profiles_evaluated.Get(); + bool is_initial_check = profiles_evaluated.count(original_profile) == 0; + profiles_evaluated.insert(original_profile); + + // The list of suspicious extensions takes priority over the dev mode bubble + // and the settings API bubble, since that needs to be shown as soon as we + // disable something. The settings API bubble is shown on first startup after + // an extension has changed the startup pages and it is acceptable if that + // waits until the next startup because of the suspicious extension bubble. + // The dev mode bubble is not time sensitive like the other two so we'll catch + // the dev mode extensions on the next startup/next window that opens. That + // way, we're not too spammy with the bubbles. + { + scoped_ptr controller( + new extensions::SuspiciousExtensionBubbleController(profile_)); + if (controller->ShouldShow()) + return controller.Pass(); + } + + { + // No use showing this if it's not the startup of the profile. + if (is_initial_check) { + scoped_ptr controller( + new extensions::SettingsApiBubbleController( + profile_, extensions::BUBBLE_TYPE_STARTUP_PAGES)); + if (controller->ShouldShow()) + return controller.Pass(); + } + } + + { + // TODO(devlin): Move the "GetExtensionOverridingProxy" part into the + // proxy bubble controller. + const extensions::Extension* extension = + extensions::GetExtensionOverridingProxy(profile_); + if (extension) { + scoped_ptr controller( + new extensions::ProxyOverriddenBubbleController(profile_)); + if (controller->ShouldShow(extension->id())) + return controller.Pass(); + } + } + + { + scoped_ptr controller( + new extensions::DevModeBubbleController(profile_)); + if (controller->ShouldShow()) + return controller.Pass(); + } + + return scoped_ptr(); +} diff --git a/chrome/browser/ui/extensions/extension_message_bubble_factory.h b/chrome/browser/ui/extensions/extension_message_bubble_factory.h new file mode 100644 index 00000000000000..703cd304e58d3e --- /dev/null +++ b/chrome/browser/ui/extensions/extension_message_bubble_factory.h @@ -0,0 +1,34 @@ +// Copyright 2015 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_UI_EXTENSIONS_EXTENSION_MESSAGE_BUBBLE_FACTORY_H_ +#define CHROME_BROWSER_UI_EXTENSIONS_EXTENSION_MESSAGE_BUBBLE_FACTORY_H_ + +#include "base/macros.h" +#include "base/memory/scoped_ptr.h" + +class Profile; + +namespace extensions { +class ExtensionMessageBubbleController; +} // namespace extensions + +// Create and show ExtensionMessageBubbles for either extensions that look +// suspicious and have therefore been disabled, or for extensions that are +// running in developer mode that we want to warn the user about. +class ExtensionMessageBubbleFactory { + public: + explicit ExtensionMessageBubbleFactory(Profile* profile); + ~ExtensionMessageBubbleFactory(); + + // Returns the controller for the bubble that should be shown, if any. + scoped_ptr GetController(); + + private: + Profile* profile_; + + DISALLOW_COPY_AND_ASSIGN(ExtensionMessageBubbleFactory); +}; + +#endif // CHROME_BROWSER_UI_EXTENSIONS_EXTENSION_MESSAGE_BUBBLE_FACTORY_H_ diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc index be3687c9fbfca7..a3323ede360e26 100644 --- a/chrome/browser/ui/toolbar/toolbar_actions_bar.cc +++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.cc @@ -7,12 +7,14 @@ #include "base/auto_reset.h" #include "base/profiler/scoped_tracker.h" #include "chrome/browser/extensions/extension_action_manager.h" +#include "chrome/browser/extensions/extension_message_bubble_controller.h" #include "chrome/browser/extensions/extension_util.h" #include "chrome/browser/profiles/profile.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/extensions/extension_action_view_controller.h" +#include "chrome/browser/ui/extensions/extension_message_bubble_factory.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model_observer.h" #include "chrome/browser/ui/toolbar/component_toolbar_actions_factory.h" @@ -418,7 +420,8 @@ ToolbarActionsBar::ToolbarActionsBar(ToolbarActionsBarDelegate* delegate, model_observer_(this), suppress_layout_(false), suppress_animation_(true), - overflowed_action_wants_to_run_(false) { + overflowed_action_wants_to_run_(false), + checked_extension_bubble_(false) { if (model_) // |model_| can be null in unittests. model_observer_.Add(model_); @@ -631,6 +634,11 @@ void ToolbarActionsBar::CreateActions() { // Once the actions are created, we should animate the changes. suppress_animation_ = false; + + // CreateActions() can be called multiple times, so we need to make sure we + // haven't already shown the bubble. + if (!checked_extension_bubble_) + MaybeShowExtensionBubble(); } void ToolbarActionsBar::DeleteActions() { @@ -729,6 +737,16 @@ bool ToolbarActionsBar::ShouldShowInfoBubble() { return true; } +void ToolbarActionsBar::MaybeShowExtensionBubble() { + checked_extension_bubble_ = true; + scoped_ptr controller = + ExtensionMessageBubbleFactory(browser_->profile()).GetController(); + if (controller) { + controller->HighlightExtensionsIfNecessary(); + delegate_->ShowExtensionMessageBubble(controller.Pass()); + } +} + void ToolbarActionsBar::OnToolbarExtensionAdded( const extensions::Extension* extension, int index) { diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar.h b/chrome/browser/ui/toolbar/toolbar_actions_bar.h index 3a6798fc29a10a..11a89021c61770 100644 --- a/chrome/browser/ui/toolbar/toolbar_actions_bar.h +++ b/chrome/browser/ui/toolbar/toolbar_actions_bar.h @@ -198,6 +198,9 @@ class ToolbarActionsBar : public extensions::ExtensionToolbarModel::Observer, // Sets |overflowed_action_wants_to_run_| to the proper value. void SetOverflowedActionWantsToRun(); + // Shows an extension message bubble, if any should be shown. + void MaybeShowExtensionBubble(); + bool in_overflow_mode() const { return main_bar_ != nullptr; } // The delegate for this object (in a real build, this is the view). @@ -247,6 +250,10 @@ class ToolbarActionsBar : public extensions::ExtensionToolbarModel::Observer, // True if an action in the overflow menu wants to run. bool overflowed_action_wants_to_run_; + // True if we have checked to see if there is an extension bubble that should + // be displayed, and, if there is, shown that bubble. + bool checked_extension_bubble_; + DISALLOW_COPY_AND_ASSIGN(ToolbarActionsBar); }; diff --git a/chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h b/chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h index 9a24e6dc3faf6c..af71316d6313b8 100644 --- a/chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h +++ b/chrome/browser/ui/toolbar/toolbar_actions_bar_delegate.h @@ -5,11 +5,16 @@ #ifndef CHROME_BROWSER_UI_TOOLBAR_TOOLBAR_ACTIONS_BAR_DELEGATE_H_ #define CHROME_BROWSER_UI_TOOLBAR_TOOLBAR_ACTIONS_BAR_DELEGATE_H_ +#include "base/memory/scoped_ptr.h" #include "ui/gfx/animation/tween.h" #include "ui/gfx/geometry/size.h" class ToolbarActionViewController; +namespace extensions { +class ExtensionMessageBubbleController; +} + // The delegate class (which, in production, represents the view) of the // ToolbarActionsBar. class ToolbarActionsBarDelegate { @@ -59,6 +64,10 @@ class ToolbarActionsBarDelegate { // action wants to run has changed. virtual void OnOverflowedActionWantsToRunChanged( bool overflowed_action_wants_to_run) = 0; + + // Displays the bubble for the passed ExtensionMessageBubbleController. + virtual void ShowExtensionMessageBubble( + scoped_ptr controller) = 0; }; #endif // CHROME_BROWSER_UI_TOOLBAR_TOOLBAR_ACTIONS_BAR_DELEGATE_H_ diff --git a/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc b/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc index 578d693c599f3c..a2b70d479088a1 100644 --- a/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc +++ b/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc @@ -7,24 +7,9 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" -#include "chrome/browser/extensions/dev_mode_bubble_controller.h" -#include "chrome/browser/extensions/extension_action_manager.h" #include "chrome/browser/extensions/extension_message_bubble_controller.h" -#include "chrome/browser/extensions/extension_service.h" -#include "chrome/browser/extensions/extension_toolbar_model.h" -#include "chrome/browser/extensions/proxy_overridden_bubble_controller.h" -#include "chrome/browser/extensions/settings_api_bubble_controller.h" -#include "chrome/browser/extensions/settings_api_helpers.h" -#include "chrome/browser/extensions/suspicious_extension_bubble_controller.h" -#include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/view_ids.h" -#include "chrome/browser/ui/views/frame/browser_view.h" -#include "chrome/browser/ui/views/toolbar/browser_actions_container.h" -#include "chrome/browser/ui/views/toolbar/browser_actions_container_observer.h" -#include "chrome/browser/ui/views/toolbar/toolbar_view.h" #include "chrome/grit/locale_settings.h" -#include "extensions/browser/extension_prefs.h" -#include "extensions/browser/extension_system.h" #include "ui/accessibility/ax_view_state.h" #include "ui/base/resource/resource_bundle.h" #include "ui/views/controls/button/label_button.h" @@ -36,9 +21,6 @@ namespace { -base::LazyInstance > g_profiles_evaluated = - LAZY_INSTANCE_INITIALIZER; - // Layout constants. const int kExtensionListPadding = 10; const int kInsetBottomRight = 13; @@ -79,21 +61,6 @@ ExtensionMessageBubbleView::ExtensionMessageBubbleView( set_anchor_view_insets(gfx::Insets(5, 0, 5, 0)); } -void ExtensionMessageBubbleView::OnActionButtonClicked( - const base::Closure& callback) { - action_callback_ = callback; -} - -void ExtensionMessageBubbleView::OnDismissButtonClicked( - const base::Closure& callback) { - dismiss_callback_ = callback; -} - -void ExtensionMessageBubbleView::OnLinkClicked( - const base::Closure& callback) { - link_callback_ = callback; -} - void ExtensionMessageBubbleView::Show() { // Not showing the bubble right away (during startup) has a few benefits: // We don't have to worry about focus being lost due to the Omnibox (or to @@ -114,7 +81,7 @@ void ExtensionMessageBubbleView::OnWidgetDestroying(views::Widget* widget) { // To catch Esc, we monitor destroy message. Unless the link has been clicked, // we assume Dismiss was the action taken. if (!link_clicked_ && !action_taken_) - dismiss_callback_.Run(); + controller_->OnBubbleDismiss(); } //////////////////////////////////////////////////////////////////////////////// @@ -241,7 +208,7 @@ void ExtensionMessageBubbleView::ButtonPressed(views::Button* sender, const ui::Event& event) { if (sender == action_button_) { action_taken_ = true; - action_callback_.Run(); + controller_->OnBubbleAction(); } else { DCHECK_EQ(dismiss_button_, sender); } @@ -252,7 +219,7 @@ void ExtensionMessageBubbleView::LinkClicked(views::Link* source, int event_flags) { DCHECK_EQ(learn_more_, source); link_clicked_ = true; - link_callback_.Run(); + controller_->OnLinkClicked(); GetWidget()->Close(); } @@ -267,243 +234,4 @@ void ExtensionMessageBubbleView::ViewHierarchyChanged( NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true); } -//////////////////////////////////////////////////////////////////////////////// -// ExtensionMessageBubbleFactory - -ExtensionMessageBubbleFactory::ExtensionMessageBubbleFactory( - Profile* profile, - ToolbarView* toolbar_view) - : profile_(profile), - toolbar_view_(toolbar_view), - shown_suspicious_extensions_bubble_(false), - shown_startup_override_extensions_bubble_(false), - shown_proxy_override_extensions_bubble_(false), - shown_dev_mode_extensions_bubble_(false), - is_observing_(false), - stage_(STAGE_START), - container_(NULL), - anchor_view_(NULL) {} - -ExtensionMessageBubbleFactory::~ExtensionMessageBubbleFactory() { - MaybeStopObserving(); -} - -void ExtensionMessageBubbleFactory::MaybeShow(views::View* anchor_view) { -#if defined(OS_WIN) - bool is_initial_check = IsInitialProfileCheck(profile_->GetOriginalProfile()); - RecordProfileCheck(profile_->GetOriginalProfile()); - - // The list of suspicious extensions takes priority over the dev mode bubble - // and the settings API bubble, since that needs to be shown as soon as we - // disable something. The settings API bubble is shown on first startup after - // an extension has changed the startup pages and it is acceptable if that - // waits until the next startup because of the suspicious extension bubble. - // The dev mode bubble is not time sensitive like the other two so we'll catch - // the dev mode extensions on the next startup/next window that opens. That - // way, we're not too spammy with the bubbles. - if (!shown_suspicious_extensions_bubble_ && - MaybeShowSuspiciousExtensionsBubble(anchor_view)) - return; - - if (!shown_startup_override_extensions_bubble_ && - is_initial_check && - MaybeShowStartupOverrideExtensionsBubble(anchor_view)) - return; - - if (!shown_proxy_override_extensions_bubble_ && - MaybeShowProxyOverrideExtensionsBubble(anchor_view)) - return; - - if (!shown_dev_mode_extensions_bubble_) - MaybeShowDevModeExtensionsBubble(anchor_view); -#endif // OS_WIN -} - -bool ExtensionMessageBubbleFactory::MaybeShowSuspiciousExtensionsBubble( - views::View* anchor_view) { - DCHECK(!shown_suspicious_extensions_bubble_); - - scoped_ptr suspicious_extensions( - new SuspiciousExtensionBubbleController(profile_)); - if (!suspicious_extensions->ShouldShow()) - return false; - - shown_suspicious_extensions_bubble_ = true; - SuspiciousExtensionBubbleController* weak_controller = - suspicious_extensions.get(); - ExtensionMessageBubbleView* bubble_delegate = - new ExtensionMessageBubbleView(anchor_view, - views::BubbleBorder::TOP_RIGHT, - suspicious_extensions.Pass()); - - views::BubbleDelegateView::CreateBubble(bubble_delegate); - weak_controller->Show(bubble_delegate); - - return true; -} - -bool ExtensionMessageBubbleFactory::MaybeShowStartupOverrideExtensionsBubble( - views::View* anchor_view) { -#if !defined(OS_WIN) - return false; -#else - DCHECK(!shown_startup_override_extensions_bubble_); - - const Extension* extension = GetExtensionOverridingStartupPages(profile_); - if (!extension) - return false; - - scoped_ptr settings_api_bubble( - new SettingsApiBubbleController(profile_, - BUBBLE_TYPE_STARTUP_PAGES)); - if (!settings_api_bubble->ShouldShow(extension->id())) - return false; - - shown_startup_override_extensions_bubble_ = true; - PrepareToHighlightExtensions(settings_api_bubble.Pass(), anchor_view); - return true; -#endif -} - -bool ExtensionMessageBubbleFactory::MaybeShowProxyOverrideExtensionsBubble( - views::View* anchor_view) { -#if !defined(OS_WIN) - return false; -#else - DCHECK(!shown_proxy_override_extensions_bubble_); - - const Extension* extension = GetExtensionOverridingProxy(profile_); - if (!extension) - return false; - - scoped_ptr proxy_bubble( - new ProxyOverriddenBubbleController(profile_)); - if (!proxy_bubble->ShouldShow(extension->id())) - return false; - - shown_proxy_override_extensions_bubble_ = true; - PrepareToHighlightExtensions(proxy_bubble.Pass(), anchor_view); - return true; -#endif -} - -bool ExtensionMessageBubbleFactory::MaybeShowDevModeExtensionsBubble( - views::View* anchor_view) { - DCHECK(!shown_dev_mode_extensions_bubble_); - - // Check the Developer Mode extensions. - scoped_ptr dev_mode_extensions( - new DevModeBubbleController(profile_)); - - // Return early if we have none to show. - if (!dev_mode_extensions->ShouldShow()) - return false; - - shown_dev_mode_extensions_bubble_ = true; - PrepareToHighlightExtensions(dev_mode_extensions.Pass(), anchor_view); - return true; -} - -void ExtensionMessageBubbleFactory::MaybeObserve() { - if (!is_observing_) { - is_observing_ = true; - container_->AddObserver(this); - } -} - -void ExtensionMessageBubbleFactory::MaybeStopObserving() { - if (is_observing_) { - is_observing_ = false; - container_->RemoveObserver(this); - } -} - -void ExtensionMessageBubbleFactory::RecordProfileCheck(Profile* profile) { - g_profiles_evaluated.Get().insert(profile); -} - -bool ExtensionMessageBubbleFactory::IsInitialProfileCheck(Profile* profile) { - return g_profiles_evaluated.Get().count(profile) == 0; -} - -void ExtensionMessageBubbleFactory::OnBrowserActionsContainerAnimationEnded() { - MaybeStopObserving(); - if (stage_ == STAGE_START) { - HighlightExtensions(); - } else if (stage_ == STAGE_HIGHLIGHTED) { - ShowHighlightingBubble(); - } else { // We shouldn't be observing if we've completed the process. - NOTREACHED(); - Finish(); - } -} - -void ExtensionMessageBubbleFactory::OnBrowserActionsContainerDestroyed() { - // If the container associated with the bubble is destroyed, abandon the - // process. - Finish(); -} - -void ExtensionMessageBubbleFactory::PrepareToHighlightExtensions( - scoped_ptr controller, - views::View* anchor_view) { - // We should be in the start stage (i.e., should not have a pending attempt to - // show a bubble). - DCHECK_EQ(stage_, STAGE_START); - - // Prepare to display and highlight the extensions before showing the bubble. - // Since this is an asynchronous process, set member variables for later use. - controller_ = controller.Pass(); - anchor_view_ = anchor_view; - container_ = toolbar_view_->browser_actions(); - - if (container_->animating()) - MaybeObserve(); - else - HighlightExtensions(); -} - -void ExtensionMessageBubbleFactory::HighlightExtensions() { - DCHECK_EQ(STAGE_START, stage_); - stage_ = STAGE_HIGHLIGHTED; - - const ExtensionIdList extension_list = controller_->GetExtensionIdList(); - DCHECK(!extension_list.empty()); - ExtensionToolbarModel::Get(profile_)->HighlightExtensions(extension_list); - if (container_->animating()) - MaybeObserve(); - else - ShowHighlightingBubble(); -} - -void ExtensionMessageBubbleFactory::ShowHighlightingBubble() { - DCHECK_EQ(stage_, STAGE_HIGHLIGHTED); - stage_ = STAGE_COMPLETE; - - views::View* reference_view = NULL; - if (container_->num_toolbar_actions() > 0u) - reference_view = container_->GetToolbarActionViewAt(0); - if (reference_view && reference_view->visible()) - anchor_view_ = reference_view; - - ExtensionMessageBubbleController* weak_controller = controller_.get(); - ExtensionMessageBubbleView* bubble_delegate = - new ExtensionMessageBubbleView( - anchor_view_, - views::BubbleBorder::TOP_RIGHT, - scoped_ptr( - controller_.release())); - views::BubbleDelegateView::CreateBubble(bubble_delegate); - weak_controller->Show(bubble_delegate); - - Finish(); -} - -void ExtensionMessageBubbleFactory::Finish() { - MaybeStopObserving(); - controller_.reset(); - anchor_view_ = NULL; - container_ = NULL; -} - } // namespace extensions diff --git a/chrome/browser/ui/views/extensions/extension_message_bubble_view.h b/chrome/browser/ui/views/extensions/extension_message_bubble_view.h index eadaf716feffa9..177673fad20e24 100644 --- a/chrome/browser/ui/views/extensions/extension_message_bubble_view.h +++ b/chrome/browser/ui/views/extensions/extension_message_bubble_view.h @@ -5,18 +5,14 @@ #ifndef CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_MESSAGE_BUBBLE_VIEW_H_ #define CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_MESSAGE_BUBBLE_VIEW_H_ -#include "base/compiler_specific.h" #include "base/macros.h" #include "base/memory/scoped_ptr.h" #include "chrome/browser/extensions/extension_message_bubble.h" -#include "chrome/browser/ui/views/toolbar/browser_actions_container_observer.h" #include "ui/views/bubble/bubble_delegate.h" #include "ui/views/controls/button/button.h" #include "ui/views/controls/link_listener.h" class Profile; -class BrowserActionsContainer; -class ToolbarView; namespace views { class Label; @@ -26,121 +22,8 @@ class View; namespace extensions { -class DevModeBubbleController; class ExtensionMessageBubbleController; -// Create and show ExtensionMessageBubbles for either extensions that look -// suspicious and have therefore been disabled, or for extensions that are -// running in developer mode that we want to warn the user about. -// Calling MaybeShow() will show one of the bubbles, if there is cause to (we -// don't show both in order to avoid spamminess). The suspicious extensions -// bubble takes priority over the developer mode extensions bubble. -class ExtensionMessageBubbleFactory : public BrowserActionsContainerObserver { - public: - ExtensionMessageBubbleFactory(Profile* profile, ToolbarView* toolbar_view); - ~ExtensionMessageBubbleFactory() override; - - void MaybeShow(views::View* anchor_view); - - private: - // The stage of showing the developer mode extensions bubble. STAGE_START - // corresponds to the beginning of the process, when nothing has been done. - // STAGE_HIGHLIGHTED indicates that the toolbar should be highlighting - // dangerous extensions. STAGE_COMPLETE means that the process should be - // ended. - enum Stage { STAGE_START, STAGE_HIGHLIGHTED, STAGE_COMPLETE }; - - // Shows the suspicious extensions bubble, if there are suspicious extensions - // and we have not done so already. - // Returns true if we have show the view. - bool MaybeShowSuspiciousExtensionsBubble(views::View* anchor_view); - - // Shows the settings API extensions bubble, if there are extensions - // overriding the startup pages and we have not done so already. - // Returns true if we show the view (or start the process). - bool MaybeShowStartupOverrideExtensionsBubble(views::View* anchor_view); - - // Shows the bubble for when there are extensions overriding the proxy (if we - // have not done so already). Returns true if we show the view (or start the - // process of doing so). - bool MaybeShowProxyOverrideExtensionsBubble(views::View* anchor_view); - - // Shows the developer mode extensions bubble, if there are extensions running - // in developer mode and we have not done so already. - // Returns true if we show the view (or start the process). - bool MaybeShowDevModeExtensionsBubble(views::View* anchor_view); - - // Starts or stops observing the BrowserActionsContainer, if necessary. - void MaybeObserve(); - void MaybeStopObserving(); - - // Adds |profile| to the list of profiles that have been evaluated for showing - // a bubble. Handy for things that only want to check once per profile. - void RecordProfileCheck(Profile* profile); - // Returns false if this profile has been evaluated before. - bool IsInitialProfileCheck(Profile* profile); - - // BrowserActionsContainer::Observer implementation. - void OnBrowserActionsContainerAnimationEnded() override; - void OnBrowserActionsContainerDestroyed() override; - - // Sets the stage for highlighting extensions and then showing the bubble - // controlled by |controller|, anchored to |anchor_view|. - void PrepareToHighlightExtensions( - scoped_ptr controller, - views::View* anchor_view); - - // Inform the ExtensionToolbarModel to highlight the appropriate extensions. - void HighlightExtensions(); - - // Shows the waiting bubbble, after highlighting the extensions. - void ShowHighlightingBubble(); - - // Finishes the process of showing the developer mode bubble. - void Finish(); - - // The associated profile. - Profile* profile_; - - // The toolbar view that the ExtensionMessageBubbleViews will attach to. - ToolbarView* toolbar_view_; - - // Whether or not we have shown the suspicious extensions bubble. - bool shown_suspicious_extensions_bubble_; - - // Whether or not we have shown the Settings API extensions bubble notifying - // the user about the startup pages being overridden. - bool shown_startup_override_extensions_bubble_; - - // Whether or not we have shown the bubble notifying the user about the proxy - // being overridden. - bool shown_proxy_override_extensions_bubble_; - - // Whether or not we have shown the developer mode extensions bubble. - bool shown_dev_mode_extensions_bubble_; - - // Whether or not we are already observing the BrowserActionsContainer (so - // we don't add ourselves twice). - bool is_observing_; - - // The current stage of showing the bubble. - Stage stage_; - - // The BrowserActionsContainer for the profile. This will be NULL if the - // factory is not currently in the process of showing a bubble. - BrowserActionsContainer* container_; - - // The default view to anchor the bubble to. This will be NULL if the factory - // is not currently in the process of showing a bubble. - views::View* anchor_view_; - - // The controller to show a bubble for. This will be NULL if the factory is - // not currently in the process of showing a bubble. - scoped_ptr controller_; - - DISALLOW_COPY_AND_ASSIGN(ExtensionMessageBubbleFactory); -}; - // This is a class that implements the UI for the bubble showing which // extensions look suspicious and have therefore been automatically disabled. class ExtensionMessageBubbleView : public ExtensionMessageBubble, @@ -154,9 +37,6 @@ class ExtensionMessageBubbleView : public ExtensionMessageBubble, scoped_ptr controller); // ExtensionMessageBubble methods. - void OnActionButtonClicked(const base::Closure& callback) override; - void OnDismissButtonClicked(const base::Closure& callback) override; - void OnLinkClicked(const base::Closure& callback) override; void Show() override; // WidgetObserver methods. @@ -198,11 +78,6 @@ class ExtensionMessageBubbleView : public ExtensionMessageBubble, bool link_clicked_; bool action_taken_; - // Callbacks into the controller. - base::Closure action_callback_; - base::Closure dismiss_callback_; - base::Closure link_callback_; - base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(ExtensionMessageBubbleView); diff --git a/chrome/browser/ui/views/settings_api_bubble_helper_views.cc b/chrome/browser/ui/views/settings_api_bubble_helper_views.cc index 341dc6124bff78..06bb439ba61786 100644 --- a/chrome/browser/ui/views/settings_api_bubble_helper_views.cc +++ b/chrome/browser/ui/views/settings_api_bubble_helper_views.cc @@ -25,13 +25,12 @@ namespace extensions { namespace { void ShowSettingsApiBubble(SettingsApiOverrideType type, - const std::string& extension_id, Profile* profile, views::View* anchor_view, views::BubbleBorder::Arrow arrow) { scoped_ptr settings_api_bubble( new SettingsApiBubbleController(profile, type)); - if (!settings_api_bubble->ShouldShow(extension_id)) + if (!settings_api_bubble->ShouldShow()) return; SettingsApiBubbleController* controller = settings_api_bubble.get(); @@ -48,18 +47,13 @@ void MaybeShowExtensionControlledHomeNotification(Browser* browser) { return; #endif - const Extension* extension = - GetExtensionOverridingHomepage(browser->profile()); - if (extension) { - // The bubble will try to anchor itself against the home button - views::View* anchor_view = BrowserView::GetBrowserViewForBrowser(browser)-> - toolbar()->home_button(); - ShowSettingsApiBubble(BUBBLE_TYPE_HOME_PAGE, - extension->id(), - browser->profile(), - anchor_view, - views::BubbleBorder::TOP_LEFT); - } + // The bubble will try to anchor itself against the home button + views::View* anchor_view = BrowserView::GetBrowserViewForBrowser(browser)-> + toolbar()->home_button(); + ShowSettingsApiBubble(BUBBLE_TYPE_HOME_PAGE, + browser->profile(), + anchor_view, + views::BubbleBorder::TOP_LEFT); } void MaybeShowExtensionControlledSearchNotification( @@ -72,17 +66,13 @@ void MaybeShowExtensionControlledSearchNotification( if (AutocompleteMatch::IsSearchType(match.type) && match.type != AutocompleteMatchType::SEARCH_OTHER_ENGINE) { - const Extension* extension = GetExtensionOverridingSearchEngine(profile); - if (extension) { - ToolbarView* toolbar = - BrowserView::GetBrowserViewForBrowser( - chrome::FindBrowserWithWebContents(web_contents))->toolbar(); - ShowSettingsApiBubble(BUBBLE_TYPE_SEARCH_ENGINE, - extension->id(), - profile, - toolbar->app_menu(), - views::BubbleBorder::TOP_RIGHT); - } + ToolbarView* toolbar = + BrowserView::GetBrowserViewForBrowser( + chrome::FindBrowserWithWebContents(web_contents))->toolbar(); + ShowSettingsApiBubble(BUBBLE_TYPE_SEARCH_ENGINE, + profile, + toolbar->app_menu(), + views::BubbleBorder::TOP_RIGHT); } } diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.cc b/chrome/browser/ui/views/toolbar/browser_actions_container.cc index b86b08413c2a0f..84deb1a94e32c5 100644 --- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc +++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc @@ -6,6 +6,7 @@ #include "base/compiler_specific.h" #include "base/stl_util.h" +#include "chrome/browser/extensions/extension_message_bubble_controller.h" #include "chrome/browser/extensions/tab_helper.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/browser.h" @@ -14,6 +15,7 @@ #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h" #include "chrome/browser/ui/view_ids.h" #include "chrome/browser/ui/views/extensions/browser_action_drag_data.h" +#include "chrome/browser/ui/views/extensions/extension_message_bubble_view.h" #include "chrome/browser/ui/views/extensions/extension_toolbar_icon_surfacing_bubble_views.h" #include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/browser/ui/views/toolbar/browser_actions_container_observer.h" @@ -339,6 +341,30 @@ void BrowserActionsContainer::OnOverflowedActionWantsToRunChanged( overflowed_action_wants_to_run); } +void BrowserActionsContainer::ShowExtensionMessageBubble( + scoped_ptr controller) { + if (animating()) { + // If the container is animating, we can't effectively anchor the bubble, + // so wait until animation stops. + pending_extension_bubble_controller_ = controller.Pass(); + return; + } + + views::View* reference_view = VisibleBrowserActions() > 0 ? + static_cast(toolbar_action_views_[0]) : + BrowserView::GetBrowserViewForBrowser(browser_)->toolbar()->app_menu(); + + extensions::ExtensionMessageBubbleController* weak_controller = + controller.get(); + extensions::ExtensionMessageBubbleView* bubble = + new extensions::ExtensionMessageBubbleView( + reference_view, + views::BubbleBorder::TOP_RIGHT, + controller.Pass()); + views::BubbleDelegateView::CreateBubble(bubble); + weak_controller->Show(bubble); +} + void BrowserActionsContainer::AddObserver( BrowserActionsContainerObserver* observer) { observers_.AddObserver(observer); @@ -668,6 +694,9 @@ void BrowserActionsContainer::AnimationEnded(const gfx::Animation* animation) { FOR_EACH_OBSERVER(BrowserActionsContainerObserver, observers_, OnBrowserActionsContainerAnimationEnded()); + + if (pending_extension_bubble_controller_) + ShowExtensionMessageBubble(pending_extension_bubble_controller_.Pass()); } content::WebContents* BrowserActionsContainer::GetCurrentWebContents() { diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.h b/chrome/browser/ui/views/toolbar/browser_actions_container.h index ab40e10dccbd61..3b26802d3dff44 100644 --- a/chrome/browser/ui/views/toolbar/browser_actions_container.h +++ b/chrome/browser/ui/views/toolbar/browser_actions_container.h @@ -252,6 +252,9 @@ class BrowserActionsContainer bool IsPopupRunning() const override; void OnOverflowedActionWantsToRunChanged( bool overflowed_action_wants_to_run) override; + void ShowExtensionMessageBubble( + scoped_ptr controller) + override; // Overridden from extension::ExtensionKeybindingRegistry::Delegate: extensions::ActiveTabPermissionGranter* GetActiveTabPermissionGranter() @@ -342,6 +345,10 @@ class BrowserActionsContainer // The class that registers for keyboard shortcuts for extension commands. scoped_ptr extension_keybinding_registry_; + // The controller of the bubble to show once animation finishes, if any. + scoped_ptr + pending_extension_bubble_controller_; + ObserverList observers_; DISALLOW_COPY_AND_ASSIGN(BrowserActionsContainer); diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.cc b/chrome/browser/ui/views/toolbar/toolbar_view.cc index 7c4073b7579642..df70ca44a09a23 100644 --- a/chrome/browser/ui/views/toolbar/toolbar_view.cc +++ b/chrome/browser/ui/views/toolbar/toolbar_view.cc @@ -31,7 +31,6 @@ #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/toolbar/wrench_menu_model.h" #include "chrome/browser/ui/view_ids.h" -#include "chrome/browser/ui/views/extensions/extension_message_bubble_view.h" #include "chrome/browser/ui/views/extensions/extension_popup.h" #include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/browser/ui/views/location_bar/page_action_image_view.h" @@ -130,10 +129,7 @@ ToolbarView::ToolbarView(Browser* browser) browser_actions_(NULL), app_menu_(NULL), browser_(browser), - badge_controller_(browser->profile(), this), - extension_message_bubble_factory_( - new extensions::ExtensionMessageBubbleFactory(browser->profile(), - this)) { + badge_controller_(browser->profile(), this) { set_id(VIEW_ID_TOOLBAR); SetEventTargeter( @@ -271,14 +267,6 @@ void ToolbarView::Init() { } } -void ToolbarView::OnWidgetVisibilityChanged(views::Widget* widget, - bool visible) { - if (visible) { - // Safe to call multiple times; the bubble will only appear once. - extension_message_bubble_factory_->MaybeShow(app_menu_); - } -} - void ToolbarView::OnWidgetActivationChanged(views::Widget* widget, bool active) { extensions::ExtensionCommandsGlobalRegistry* registry = diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.h b/chrome/browser/ui/views/toolbar/toolbar_view.h index 32fe3f6c55a698..0e60a162b36277 100644 --- a/chrome/browser/ui/views/toolbar/toolbar_view.h +++ b/chrome/browser/ui/views/toolbar/toolbar_view.h @@ -31,7 +31,6 @@ class WrenchToolbarButton; namespace extensions { class Command; class Extension; -class ExtensionMessageBubbleFactory; } namespace views { @@ -137,7 +136,6 @@ class ToolbarView : public views::AccessiblePaneView, void ButtonPressed(views::Button* sender, const ui::Event& event) override; // views::WidgetObserver: - void OnWidgetVisibilityChanged(views::Widget* widget, bool visible) override; void OnWidgetActivationChanged(views::Widget* widget, bool active) override; // content::NotificationObserver: @@ -245,11 +243,6 @@ class ToolbarView : public views::AccessiblePaneView, scoped_ptr wrench_menu_model_; scoped_ptr wrench_menu_; - // The factory to create bubbles to warn about dangerous/suspicious - // extensions. - scoped_ptr - extension_message_bubble_factory_; - // A list of listeners to call when the menu opens. ObserverList menu_listeners_; diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi index 5e6f594c0df0f9..fe5259fde79c3d 100644 --- a/chrome/chrome_browser_ui.gypi +++ b/chrome/chrome_browser_ui.gypi @@ -2592,6 +2592,8 @@ 'browser/ui/extensions/extension_action_platform_delegate.h', 'browser/ui/extensions/extension_action_view_controller.cc', 'browser/ui/extensions/extension_action_view_controller.h', + 'browser/ui/extensions/extension_message_bubble_factory.cc', + 'browser/ui/extensions/extension_message_bubble_factory.h', 'browser/ui/extensions/extension_enable_flow.cc', 'browser/ui/extensions/extension_enable_flow.h', 'browser/ui/extensions/extension_enable_flow_delegate.h',