Add an extension bubble explaining which extensions are in dev mode.

chrome/browser/ui/views/extensions/extension_message_bubble_view.cc

Index: chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
diff --git a/chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.cc b/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
similarity index 46%
rename from chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.cc
rename to chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
index 2eb92b6045358d75648bf86262e6a9e2963cb6d6..1c545aa3482c694d94a1b1efe40986765ad2703d 100644
--- a/chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.cc
+++ b/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
@@ -2,17 +2,22 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.h"
+#include "chrome/browser/ui/views/extensions/extension_message_bubble_view.h"
 
 #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_delegate.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
-#include "chrome/browser/extensions/suspicious_extension_bubble_controller.h"
+#include "chrome/browser/extensions/suspicious_extension_bubble_controller_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.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/toolbar_view.h"
 #include "grit/locale_settings.h"
 #include "ui/base/accessibility/accessible_view_state.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -20,6 +25,7 @@
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/layout/grid_layout.h"
+#include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
@@ -42,20 +48,21 @@ const int kBubbleAppearanceWaitTime = 5;
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
-// SuspiciousExtensionBubbleView
+// ExtensionMessageBubbleView
 
 namespace extensions {
 
-SuspiciousExtensionBubbleView::SuspiciousExtensionBubbleView(
+ExtensionMessageBubbleView::ExtensionMessageBubbleView(
     views::View* anchor_view,
-    SuspiciousExtensionBubbleController* controller)
+    ExtensionMessageBubbleController::Delegate* delegate)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
       weak_factory_(this),
-      controller_(controller),
+      delegate_(delegate),
       headline_(NULL),
       learn_more_(NULL),
       dismiss_button_(NULL),
-      link_clicked_(false) {
+      link_clicked_(false),
+      action_taken_(false) {
   DCHECK(anchor_view->GetWidget());
   set_close_on_deactivate(false);
   set_move_with_anchor(true);
@@ -66,31 +73,86 @@ SuspiciousExtensionBubbleView::SuspiciousExtensionBubbleView(
 }
 
 // static
-void SuspiciousExtensionBubbleView::MaybeShow(
+void ExtensionMessageBubbleView::MaybeShow(
     Browser* browser,
     views::View* anchor_view) {
-  SuspiciousExtensionBubbleController* controller =
-      extensions::SuspiciousExtensionBubbleController::Get(
+  // The list of suspicious extensions takes priority over the dev mode bubble,
+  // since that needs to be shown as soon as we disable something. The dev mode
+  // bubble is not as time sensitive so we'll catch the dev mode extensions on
+  // the next startup. That way, we're not too spammy with the bubbles.
+  SuspiciousExtensionBubbleControllerDelegate* suspicious_extensions =

[not at google - send to devlin, 2013/12/10 01:21:39]

These Delegates are supposed to be implementation details of the Controllers,
not actually exposed as part of the functional interface. I.e. I would expect to
see this accessing a controller for the suspicious extensions then calling
Show() on it. This would prevent those odd methods on the Delegates which just
call through to the Controller.

This raises the problem of where to put the ::Get(Profile*) method. IIRC you
need the per-profile singleton-ness to enforce that the bubble only gets shown
once per launch? For that perhaps you could implement that within the Delegate
by maintaining a LazyInstance<ProfileSet> where ProfileSet contains a
set<Profile*>.

Incidentally, this whole method is a bit suspect. This MaybeShow is now actually
responsible for showing *two* views and (see below) that's subtle and perhaps it
should go in that Controller so that it can be tested.

[Finnur, 2013/12/11 23:12:18]

The call-through methods are gone. As for the two bubbles showing, I refer to
our in-person chat and the comment above.

+      extensions::SuspiciousExtensionBubbleControllerDelegate::Get(
           browser->profile());
-  if (controller->HasSuspiciousExtensions()) {
-    SuspiciousExtensionBubbleView* bubble_delegate =
-        new SuspiciousExtensionBubbleView(anchor_view, controller);
+  if (suspicious_extensions->HasExtensionList()) {
+    ExtensionMessageBubbleView* bubble_delegate =
+        new ExtensionMessageBubbleView(anchor_view, suspicious_extensions);
     views::BubbleDelegateView::CreateBubble(bubble_delegate);
-    controller->Show(bubble_delegate);
+    suspicious_extensions->Show(bubble_delegate);
+    return;

[not at google - send to devlin, 2013/12/10 01:21:39]

so what is supposed to happen when there are both suspicious extensions and it's
in dev mode? It looks like the first time MaybeShow is called the suspicious
bubble is shown, the second time the dev mode bubble is shown.

just checking that's what you intended.

[Finnur, 2013/12/11 23:12:18]

Yes. That is exactly what I intended. See comment at the top of the function.

+  }
+
+  DevModeBubbleControllerDelegate* dev_mode_extensions =
+      extensions::DevModeBubbleControllerDelegate::Get(
+          browser->profile());
+  if (dev_mode_extensions->HasExtensionList()) {
+    // Find an extension browser action view to anchor against in the toolbar.
+    BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
+    views::View* reference_view = NULL;
+    BrowserActionsContainer* container =
+        browser_view->GetToolbarView()->browser_actions();
+    if (container->animating())
+      return;
+
+    ExtensionService* service = extensions::ExtensionSystem::Get(
+        browser->profile())->extension_service();
+    extensions::ExtensionActionManager* extension_action_manager =
+        extensions::ExtensionActionManager::Get(browser->profile());
+
+    const ExtensionIdList extension_list =
+        dev_mode_extensions->GetExtensionIdList();
+    for (size_t i = 0; i < extension_list.size(); ++i) {
+      const Extension* extension =
+          service->GetExtensionById(extension_list[i], false);
+      if (!extension)
+        continue;
+      reference_view = container->GetBrowserActionView(
+          extension_action_manager->GetBrowserAction(*extension));
+      if (reference_view && reference_view->visible())
+        break;  // Found a good candidate.

[not at google - send to devlin, 2013/12/10 01:21:39]

all this logic should be in the Delegate implementation for dev mode bubble so
that it can be tested.

[Finnur, 2013/12/11 23:12:18]

This is becoming kind of a moot point because the UX designer wants to force the
dev mode extensions to be visible and not in the overflow, so I'll need to rip
this out anyway (in my next changelist). For now, I'd like to land this as-is so
that I can get this changelist out of the way. :)

+    }
+    if (reference_view) {
+      // If we have a view, it means we found a browser action and we want to
+      // point to the chevron, not the hotdog menu.
+      if (!reference_view->visible())
+        reference_view = container->chevron();  // It's hidden, use the chevron.
+    }
+    if (reference_view && reference_view->visible())
+      anchor_view = reference_view;  // Catch-all is the hotdog menu.
+
+    // Show the bubble.
+    ExtensionMessageBubbleView* bubble_delegate =
+        new ExtensionMessageBubbleView(anchor_view, dev_mode_extensions);
+    views::BubbleDelegateView::CreateBubble(bubble_delegate);
+    dev_mode_extensions->Show(bubble_delegate);
   }
 }
 
-void SuspiciousExtensionBubbleView::OnButtonClicked(
+void ExtensionMessageBubbleView::OnActionButtonClicked(
     const base::Closure& callback) {
-  button_callback_ = callback;
+  action_callback_ = callback;
 }
 
-void SuspiciousExtensionBubbleView::OnLinkClicked(
+void ExtensionMessageBubbleView::OnDismissButtonClicked(
+    const base::Closure& callback) {
+  dismiss_callback_ = callback;
+}
+
+void ExtensionMessageBubbleView::OnLinkClicked(
     const base::Closure& callback) {
   link_callback_ = callback;
 }
 
-void SuspiciousExtensionBubbleView::Show() {
+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
   // other things that want focus at startup). This allows Esc to work to close
@@ -101,29 +163,29 @@ void SuspiciousExtensionBubbleView::Show() {
   // startup).
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
-      base::Bind(&SuspiciousExtensionBubbleView::ShowBubble,
+      base::Bind(&ExtensionMessageBubbleView::ShowBubble,
                  weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromSeconds(kBubbleAppearanceWaitTime));
 }
 
-void SuspiciousExtensionBubbleView::OnWidgetDestroying(views::Widget* widget) {
+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_)
-    button_callback_.Run();
+  if (!link_clicked_ && !action_taken_)
+    dismiss_callback_.Run();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// SuspiciousExtensionBubbleView - private.
+// ExtensionMessageBubbleView - private.
 
-SuspiciousExtensionBubbleView::~SuspiciousExtensionBubbleView() {
+ExtensionMessageBubbleView::~ExtensionMessageBubbleView() {
 }
 
-void SuspiciousExtensionBubbleView::ShowBubble() {
+void ExtensionMessageBubbleView::ShowBubble() {
   StartFade(true);
 }
 
-void SuspiciousExtensionBubbleView::Init() {
+void ExtensionMessageBubbleView::Init() {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
 
   views::GridLayout* layout = views::GridLayout::CreatePanel(this);
@@ -140,7 +202,7 @@ void SuspiciousExtensionBubbleView::Init() {
 
   headline_ = new views::Label();
   headline_->SetFont(rb.GetFont(ui::ResourceBundle::MediumFont));
-  headline_->SetText(controller_->GetTitle());
+  headline_->SetText(delegate_->GetTitle());
   layout->AddView(headline_);
 
   layout->AddPaddingRow(0, kHeadlineRowPadding);
@@ -155,45 +217,50 @@ void SuspiciousExtensionBubbleView::Init() {
   views::Label* message = new views::Label();
   message->SetMultiLine(true);
   message->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  message->SetText(controller_->GetMessageBody());
+  message->SetText(delegate_->GetMessageBody());
   message->SizeToFit(views::Widget::GetLocalizedContentsWidth(
       IDS_EXTENSION_WIPEOUT_BUBBLE_WIDTH_CHARS));
   layout->AddView(message);
 
-  const int extension_list_column_set_id = 2;
-  views::ColumnSet* middle_columns =
-      layout->AddColumnSet(extension_list_column_set_id);
-  middle_columns->AddPaddingColumn(0, kExtensionListPadding);
-  middle_columns->AddColumn(
-      views::GridLayout::LEADING, views::GridLayout::CENTER,
-      0, views::GridLayout::USE_PREF, 0, 0);
-
-  layout->StartRowWithPadding(0, extension_list_column_set_id,
-      0, kHeadlineMessagePadding);
-  views::Label* extensions = new views::Label();
-  extensions->SetMultiLine(true);
-  extensions->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-
-  std::vector<string16> extension_list;
-  char16 bullet_point = 0x2022;
-
-  std::vector<string16> suspicious = controller_->GetSuspiciousExtensionNames();
-  size_t i = 0;
-  for (; i < suspicious.size() && i < kMaxExtensionsToShow; ++i) {
-    // Add each extension with bullet point.
-    extension_list.push_back(bullet_point + ASCIIToUTF16(" ") + suspicious[i]);
+  if (delegate_->ShouldShowExtensionList()) {
+    const int extension_list_column_set_id = 2;
+    views::ColumnSet* middle_columns =
+        layout->AddColumnSet(extension_list_column_set_id);
+    middle_columns->AddPaddingColumn(0, kExtensionListPadding);
+    middle_columns->AddColumn(
+        views::GridLayout::LEADING, views::GridLayout::CENTER,
+        0, views::GridLayout::USE_PREF, 0, 0);
+
+    layout->StartRowWithPadding(0, extension_list_column_set_id,
+        0, kHeadlineMessagePadding);
+    views::Label* extensions = new views::Label();
+    extensions->SetMultiLine(true);
+    extensions->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+
+    std::vector<string16> extension_list;
+    char16 bullet_point = 0x2022;
+
+    std::vector<string16> suspicious = delegate_->GetExtensionList();
+    size_t i = 0;
+    for (; i < suspicious.size() && i < kMaxExtensionsToShow; ++i) {
+      // Add each extension with bullet point.
+      extension_list.push_back(
+          bullet_point + ASCIIToUTF16(" ") + suspicious[i]);
+    }
+
+    if (i > kMaxExtensionsToShow) {
+      string16 difference = base::IntToString16(i - kMaxExtensionsToShow);
+      extension_list.push_back(bullet_point + ASCIIToUTF16(" ") +
+          delegate_->GetOverflowText(difference));
+    }
+
+    extensions->SetText(JoinString(extension_list, ASCIIToUTF16("\n")));
+    extensions->SizeToFit(views::Widget::GetLocalizedContentsWidth(
+        IDS_EXTENSION_WIPEOUT_BUBBLE_WIDTH_CHARS));
+    layout->AddView(extensions);
   }
 
-  if (i > kMaxExtensionsToShow) {
-    string16 difference = base::IntToString16(i - kMaxExtensionsToShow);
-    extension_list.push_back(bullet_point + ASCIIToUTF16(" ") +
-        controller_->GetOverflowText(difference));
-  }
-
-  extensions->SetText(JoinString(extension_list, ASCIIToUTF16("\n")));
-  extensions->SizeToFit(views::Widget::GetLocalizedContentsWidth(
-      IDS_EXTENSION_WIPEOUT_BUBBLE_WIDTH_CHARS));
-  layout->AddView(extensions);
+  string16 action_button = delegate_->GetActionButtonLabel();
 
   const int action_row_column_set_id = 3;
   views::ColumnSet* bottom_columns =
@@ -203,39 +270,54 @@ void SuspiciousExtensionBubbleView::Init() {
   bottom_columns->AddPaddingColumn(1, 0);
   bottom_columns->AddColumn(views::GridLayout::TRAILING,
       views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
+  if (!action_button.empty()) {
+    bottom_columns->AddColumn(views::GridLayout::TRAILING,
+        views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
+  }
   layout->StartRowWithPadding(0, action_row_column_set_id,
                               0, kMessageBubblePadding);
 
-  learn_more_ = new views::Link(controller_->GetLearnMoreLabel());
+  learn_more_ = new views::Link(delegate_->GetLearnMoreLabel());
   learn_more_->set_listener(this);
   layout->AddView(learn_more_);
 
+  if (!action_button.empty()) {
+    action_button_ = new views::LabelButton(this, action_button.c_str());
+    action_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
+    layout->AddView(action_button_);
+  }
+
   dismiss_button_ = new views::LabelButton(this,
-      controller_->GetDismissButtonLabel());
+      delegate_->GetDismissButtonLabel());
   dismiss_button_->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);
   layout->AddView(dismiss_button_);
 }
 
-void SuspiciousExtensionBubbleView::ButtonPressed(views::Button* sender,
-                                                  const ui::Event& event) {
-  DCHECK_EQ(dismiss_button_, sender);
+void ExtensionMessageBubbleView::ButtonPressed(views::Button* sender,
+                                               const ui::Event& event) {
+  if (sender == action_button_) {
+    action_taken_ = true;
+    action_callback_.Run();
+  } else {
+    DCHECK_EQ(dismiss_button_, sender);
+  }
   GetWidget()->Close();
 }
 
-void SuspiciousExtensionBubbleView::LinkClicked(views::Link* source,
-                                                int event_flags) {
+void ExtensionMessageBubbleView::LinkClicked(views::Link* source,
+                                             int event_flags) {
   DCHECK_EQ(learn_more_, source);
   link_clicked_ = true;
   link_callback_.Run();
   GetWidget()->Close();
 }
 
-void SuspiciousExtensionBubbleView::GetAccessibleState(
+void ExtensionMessageBubbleView::GetAccessibleState(
     ui::AccessibleViewState* state) {
   state->role = ui::AccessibilityTypes::ROLE_ALERT;
 }
 
-void SuspiciousExtensionBubbleView::ViewHierarchyChanged(
+void ExtensionMessageBubbleView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   if (details.is_add && details.child == this)
     NotifyAccessibilityEvent(ui::AccessibilityTypes::EVENT_ALERT, true);