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 47%
copy from chrome/browser/ui/views/extensions/suspicious_extension_bubble_view.cc
copy to chrome/browser/ui/views/extensions/extension_message_bubble_view.cc
index e088b10742b258c931d544cdc96dfbdf0c8f9dd4..0239ff2fb6a409bc9d4aa93214513e8fe91867c4 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.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/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(

[not at google - send to devlin, 2013/12/12 18:21:38]

This MaybeShow code doesn't really belong here.

ExtensionMessageBubbleView is a non-specific class that knows how to render a
bubble. Having MaybeShow defined here implies that it knows about all types of
instances of itself, and furthermore, how they interact with each other.

I think basically moving this whole function into toolbar_view is a decent
enough way to address this. That would also simplify this method since you
wouldn't need to dig the toolbar view back out of the browser (though for that
matter, you could pass it in here anyway).

[Finnur, 2013/12/12 20:25:12]

This logic doesn't belong in either place really, it belongs in some framework
that controls the priority of the bubbles and makes sure we show them in an
orderly fashion. A framework we currently need but don't have.

The toolbar view is a convenient hook to show the bubbles, but I think the
toolbar view folks would hold the opposite view (that the logic of what bubble
is shown belongs to the bubble code and not the toolbar).

[not at google - send to devlin, 2013/12/12 21:17:19]

Only the toolbar view knows that the "suspicious bubble" and the "dev mode
bubble" should be shown from it. In other cases maybe it's the hotdog menu, or
an infobar, or the back button, or the omnibox, so in terms of *constructing*
the view it should be the toolbar doing that, not here. Reusable views like this
shouldn't care where they're attached.

However, you're right that the actual logic to decide when to show toolbars
doesn't belong in either place, insofar as it's not a view concern. A sensible
way to implement this would probably be to make the Controllers aware of each
other in some way, so that the dev mode controller returns false from ShouldShow
if the suspicious bubble should show.

[Finnur, 2013/12/13 21:29:48]

I thought about that, but this gets messy as you add more bubbles. If we make
the controllers aware of each other then you need to update n code sites in code
you don't care about every time you add bubble n+1.

Bottom line: I think we can't fix this in a way everyone will be happy with
until we have some outside framework that controls the bubble priority and the
orderly showing of them.

     Browser* browser,
     views::View* anchor_view) {
-  SuspiciousExtensionBubbleController* controller =
+  // 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.

[not at google - send to devlin, 2013/12/12 18:21:38]

it's not next startup, it's next time this method is called (since after this
method is called once, suspicious_extensions->ShouldShow() will return false),
and this method might be called from anywhere in theory (unless you move it into
toolbar_view), though in any case, there is a toolbar view constructed per
window I presume so really this is "next time there is a new window"?

[Finnur, 2013/12/12 20:25:12]

Yeah, it is next startup/next window. Updated comment to reflect.

+  SuspiciousExtensionBubbleController* suspicious_extensions =
       extensions::SuspiciousExtensionBubbleController::Get(
           browser->profile());
-  if (controller->HasSuspiciousExtensions()) {
-    SuspiciousExtensionBubbleView* bubble_delegate =
-        new SuspiciousExtensionBubbleView(anchor_view, controller);
+  if (suspicious_extensions->ShouldShow()) {
+    ExtensionMessageBubbleView* bubble_delegate =
+        new ExtensionMessageBubbleView(anchor_view, suspicious_extensions);
     views::BubbleDelegateView::CreateBubble(bubble_delegate);
-    controller->Show(bubble_delegate);
+    suspicious_extensions->Show(bubble_delegate);
+    return;
+  }
+
+  DevModeBubbleController* dev_mode_extensions =
+      extensions::DevModeBubbleController::Get(
+          browser->profile());
+  if (dev_mode_extensions->ShouldShow()) {
+    // 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();

[not at google - send to devlin, 2013/12/12 18:21:38]

see comment above; you could at least pass the toolbar view (or the browser
actions container) directly into this method, but I think that moving MaybeShow
would make even more sense here (it could be an instance method on toolbar_view
and you could use "this->browser_actions()" there).

[Finnur, 2013/12/12 20:25:12]

Added toolbar_view param.

+    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->extension_id_list();

[not at google - send to devlin, 2013/12/12 18:21:38]

can you make dev_mode_extensions return an ExtensionSet rather than a list of
IDs? that would simplify this logic even further; and the less logic in the
views the better.

[Finnur, 2013/12/12 20:25:12]

Same argument applies here as with ShouldShowExtension(Extension*/Extension
ID)). We'd have to pass disabled extensions as well, which could lead to subtle
bugs.

[not at google - send to devlin, 2013/12/12 21:17:19]

Makes sense. You could include the Extension::State of the extension, and I
think that'd be nicer in this case, but still a little bit arbitrary.

See comment about the extension_id_list() function though. I didn't make it
before because I was hoping the method would entirely change, but just call
GetExtensionList() here.

[Finnur, 2013/12/13 21:29:48]

GetExtensionList returns names, not IDs. Changed it to 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.
+    }
+    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_->GetExtensions();

[not at google - send to devlin, 2013/12/12 18:21:38]

use controller->GetExtensionList() here?

oh, you don't hold onto the controller... so maybe just hold onto the controller
and (though it's a bit odd; less odd) have a delegate() method on the controller

so that would change things like:
- delegate_->ShouldShowExtensionList() ->
controller_->delegate()->ShouldShowExtensionList()
- delegate_->GetMessageBody() -> controller_->delegate()->GetMessageBody()

etc.

+    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) {
-    base::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);