[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}

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);

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;
 };

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();
 }
 

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);
 

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<TestExtensionSystem*>(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<base::CommandLine> command_line_;
-  scoped_ptr<content::TestBrowserThreadBundle> thread_bundle_;
   scoped_ptr<TestingProfile> 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<SettingsApiOverrideType>(i)));
 
     // The list will contain one enabled unpacked extension (ext 2).
-    EXPECT_TRUE(controller->ShouldShow(kId2));
+    EXPECT_TRUE(controller->ShouldShow());
     std::vector<base::string16> 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<SettingsApiOverrideType>(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<SettingsApiOverrideType>(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());

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;

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;