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

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

 // Copyright (c) 2013 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/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"
 #include "ui/views/controls/button/label_button.h"
 #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 {
 
 // Layout constants.
 const int kExtensionListPadding = 10;
 const int kInsetBottomRight = 13;
 const int kInsetLeft = 14;
 const int kInsetTop = 9;
 const int kHeadlineMessagePadding = 4;
 const int kHeadlineRowPadding = 10;
 const int kMessageBubblePadding = 11;
 
 // How many extensions to show in the bubble (max).
 const size_t kMaxExtensionsToShow = 7;
 
 // How long to wait until showing the bubble (in seconds).
 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);
   set_close_on_esc(true);
 
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(gfx::Insets(5, 0, 5, 0));
 }
 
 // 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
   // the bubble and also solves the keyboard accessibility problem that comes
   // with focus being lost (we don't have a good generic mechanism of injecting
   // bubbles into the focus cycle). Another benefit of delaying the show is
   // that fade-in works (the fade-in isn't apparent if the the bubble appears at
   // 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);
   layout->SetInsets(kInsetTop, kInsetLeft,
                     kInsetBottomRight, kInsetBottomRight);
   SetLayoutManager(layout);
 
   const int headline_column_set_id = 0;
   views::ColumnSet* top_columns = layout->AddColumnSet(headline_column_set_id);
   top_columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::CENTER,
                          0, views::GridLayout::USE_PREF, 0, 0);
   top_columns->AddPaddingColumn(1, 0);
   layout->StartRow(0, headline_column_set_id);
 
   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);
 
   const int text_column_set_id = 1;
   views::ColumnSet* upper_columns = layout->AddColumnSet(text_column_set_id);
   upper_columns->AddColumn(
       views::GridLayout::LEADING, views::GridLayout::LEADING,
       0, views::GridLayout::USE_PREF, 0, 0);
   layout->StartRow(0, text_column_set_id);
 
   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);
+  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);
+    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> 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]);
+    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 =
       layout->AddColumnSet(action_row_column_set_id);
   bottom_columns->AddColumn(views::GridLayout::LEADING,
       views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
   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);
 }
 
 }  // namespace extensions