Convert ToolbarActionsBarBubbleViews to BubbleDialogDelegate

chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc

Index: chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
index b27c51536c72207ba8caafcd591b45b81d9c4e0a..a5530f685d9a8b8aab8b0a09e9e4b261498fac42 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.cc
@@ -11,7 +11,7 @@
 #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/layout/box_layout.h"
 #include "ui/views/layout/layout_constants.h"
 
 namespace {
@@ -21,16 +21,11 @@ const int kListPadding = 10;
 ToolbarActionsBarBubbleViews::ToolbarActionsBarBubbleViews(
     views::View* anchor_view,
     std::unique_ptr<ToolbarActionsBarBubbleDelegate> delegate)
-    : views::BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
-      anchor_view_(anchor_view),
+    : views::BubbleDialogDelegateView(anchor_view,
+                                      views::BubbleBorder::TOP_RIGHT),
       delegate_(std::move(delegate)),
-      heading_label_(nullptr),
-      content_label_(nullptr),
       item_list_(nullptr),
-      learn_more_button_(nullptr),
-      dismiss_button_(nullptr),
-      action_button_(nullptr),
-      acknowledged_(false) {
+      learn_more_button_(nullptr) {
   set_close_on_deactivate(delegate_->ShouldCloseOnDeactivate());
 }
 
@@ -41,129 +36,82 @@ void ToolbarActionsBarBubbleViews::Show() {
   GetWidget()->Show();
 }
 
+views::View* ToolbarActionsBarBubbleViews::CreateExtraView() {
+  base::string16 text = delegate_->GetLearnMoreButtonText();
+  if (text.empty())
+    return nullptr;
+
+  learn_more_button_ = new views::Link(text);
+  learn_more_button_->set_listener(this);
+  return learn_more_button_;
+}
+
+base::string16 ToolbarActionsBarBubbleViews::GetWindowTitle() const {
+  return delegate_->GetHeadingText();
+}
+
+bool ToolbarActionsBarBubbleViews::Cancel() {
+  delegate_->OnBubbleClosed(
+      ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION);
+  return true;
+}
+
+bool ToolbarActionsBarBubbleViews::Accept() {
+  delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_EXECUTE);
+  return true;
+}
+
+bool ToolbarActionsBarBubbleViews::Close() {
+  if (delegate_) {
+    delegate_->OnBubbleClosed(
+        ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION);

[Devlin, 2016/04/13 22:36:53]

The comments on DialogDelegate::Close make me a little nervous about treating
this as deactivation.  In particular, it says it can be called "by pressing the
close button on the window or using a window manager gesture".  Those seem a
little to user-actiony to me.

I wonder if we can instead add the close_reason logic from BubbleDelegate to
BubbleDialogDelegate, so that we only assign CLOSE_DISMISS_DEACTIVATION when the
reason is deactivation?  It even looks like BubbleDialogDelegate has the
CloseReason enum, though it's not used anywhere.

[Evan Stade, 2016/04/14 00:37:08]

Well, there is no close button on this bubble, but if there were, you could
figure out if it were clicked with GetBubbleFrameView()->close_button_clicked().

I'm specifically trying to get rid of CloseReason because no bubble I've found
(and I've looked at tens of them) actually needs it. The reason that enum still
exists is I forgot to delete it.

[Devlin, 2016/04/14 01:41:41]

This bubble needs it. :)  It, and the now-consumed ExtensionMessageBubbleView,
are the reasons it was added.  My concern here is that this is called directly
from Widget::Close(), which is, in turn, called from, well, everywhere.  So
unless the BubbleDialogDelegateView is sure to call into Cancel() first, we'll
be likely to miscategorize a close as due to dismissal.  If it were me, I might
recommend pulling the functionality to check for dismissal (i.e.,
OnWidgetActivationChanged()) into this class.  But you're probably more familiar
with the bubble code than I am, so if you're confident that Cancel() is called
for user action close cases, I'll trust you on it.

[Evan Stade, 2016/04/14 02:54:24]

But is "everywhere" more likely to be like deactivation or like explicit close?
It depends on what you mean by user action closes, but for instances where users
actually interact with the bubble and tell it to close, yes.

+  }
+  return true;
+}
+
 void ToolbarActionsBarBubbleViews::Init() {
-  views::GridLayout* layout = new views::GridLayout(this);
-  layout->SetInsets(views::kPanelVertMargin, views::kPanelHorizMargin,
-                    views::kPanelVertMargin, views::kPanelHorizMargin);
-  SetLayoutManager(layout);
-
-  enum ColumnSetId {
-    HEADER_AND_BODY_COLUMN_SET = 0,
-    LIST_COLUMN_SET,
-    BUTTON_STRIP_COLUMN_SET,
-  };
-
-  views::ColumnSet* header_and_body_cs =
-      layout->AddColumnSet(HEADER_AND_BODY_COLUMN_SET);
-  header_and_body_cs->AddColumn(views::GridLayout::LEADING,
-                                views::GridLayout::LEADING, 1,
-                                views::GridLayout::USE_PREF, 0, 0);
-  views::ColumnSet* buttons_cs = layout->AddColumnSet(BUTTON_STRIP_COLUMN_SET);
-  buttons_cs->AddColumn(views::GridLayout::LEADING, views::GridLayout::CENTER,
-                        0, views::GridLayout::USE_PREF, 0, 0);
-  buttons_cs->AddPaddingColumn(1, 0);
-  buttons_cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
-                        0, views::GridLayout::USE_PREF, 0, 0);
-  buttons_cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing);
-  buttons_cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
-                        0, views::GridLayout::USE_PREF, 0, 0);
-
-  // Add a header.
-  layout->StartRow(0, HEADER_AND_BODY_COLUMN_SET);
-  heading_label_ = new views::Label(delegate_->GetHeadingText());
-  heading_label_->SetFontList(
-      ui::ResourceBundle::GetSharedInstance().GetFontList(
-          ui::ResourceBundle::MediumFont));
-  heading_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  layout->AddView(heading_label_);
-  layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
-  int width = views::Widget::GetLocalizedContentsWidth(
-      IDS_EXTENSION_TOOLBAR_REDESIGN_NOTIFICATION_BUBBLE_WIDTH_CHARS);
+  SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
+                                        views::kRelatedControlVerticalSpacing));
 
   // Add the content string.
-  layout->StartRow(0, HEADER_AND_BODY_COLUMN_SET);
-  content_label_ = new views::Label(
-      delegate_->GetBodyText(anchor_view_->id() == VIEW_ID_BROWSER_ACTION));
-  content_label_->SetMultiLine(true);
-  content_label_->SizeToFit(width);
-  content_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  layout->AddView(content_label_);
+  views::Label* content_label = new views::Label(
+      delegate_->GetBodyText(GetAnchorView()->id() == VIEW_ID_BROWSER_ACTION));
+  content_label->SetMultiLine(true);
+  int width = views::Widget::GetLocalizedContentsWidth(
+        IDS_EXTENSION_TOOLBAR_REDESIGN_NOTIFICATION_BUBBLE_WIDTH_CHARS);
+  content_label->SizeToFit(width);
+  content_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  AddChildView(content_label);
 
   base::string16 item_list = delegate_->GetItemListText();
   if (!item_list.empty()) {
-    views::ColumnSet* list_cs = layout->AddColumnSet(LIST_COLUMN_SET);
-    list_cs->AddPaddingColumn(0, kListPadding);
-    list_cs->AddColumn(views::GridLayout::LEADING,
-                       views::GridLayout::CENTER,
-                       0,
-                       views::GridLayout::USE_PREF,
-                       0, 0);
-
-    layout->StartRowWithPadding(0, LIST_COLUMN_SET, 0, 0);
     item_list_ = new views::Label(item_list);
+    item_list_->SetBorder(
+        views::Border::CreateEmptyBorder(0, kListPadding, 0, 0));
     item_list_->SetMultiLine(true);
-    item_list_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     item_list_->SizeToFit(width);
-    layout->AddView(item_list_);
-  }
-
-  layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
-
-  layout->StartRow(0, BUTTON_STRIP_COLUMN_SET);
-  base::string16 learn_more_text = delegate_->GetLearnMoreButtonText();
-  if (!learn_more_text.empty()) {
-    learn_more_button_ = new views::Link(learn_more_text);
-    learn_more_button_->set_listener(this);
-    layout->AddView(learn_more_button_);
-  } else {
-    layout->SkipColumns(1);
-  }
-
-  base::string16 dismiss_button_text = delegate_->GetDismissButtonText();
-  if (!dismiss_button_text.empty()) {
-    dismiss_button_ = new views::LabelButton(this, dismiss_button_text);
-    dismiss_button_->SetStyle(views::Button::STYLE_BUTTON);
-    layout->AddView(dismiss_button_, 1, 1, views::GridLayout::TRAILING,
-                    views::GridLayout::FILL);
-  } else {
-    layout->SkipColumns(1);
+    item_list_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    AddChildView(item_list_);
   }
-
-  action_button_ =
-      new views::LabelButton(this, delegate_->GetActionButtonText());
-  action_button_->SetStyle(views::Button::STYLE_BUTTON);
-  layout->AddView(action_button_, 1, 1, views::GridLayout::TRAILING,
-                  views::GridLayout::FILL);
 }
 
-void ToolbarActionsBarBubbleViews::OnWidgetDestroying(views::Widget* widget) {
-  BubbleDelegateView::OnWidgetDestroying(widget);
-  if (!acknowledged_) {
-    ToolbarActionsBarBubbleDelegate::CloseAction close_action =
-        close_reason() == CloseReason::DEACTIVATION
-            ? ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION
-            : ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION;
-    delegate_->OnBubbleClosed(close_action);
-    acknowledged_ = true;
-  }
+int ToolbarActionsBarBubbleViews::GetDialogButtons() const {
+  int buttons = ui::DIALOG_BUTTON_OK;
+  if (!delegate_->GetDismissButtonText().empty())
+    buttons |= ui::DIALOG_BUTTON_CANCEL;
+  return buttons;
 }
 
-void ToolbarActionsBarBubbleViews::ButtonPressed(views::Button* sender,
-                                                 const ui::Event& event) {
-  if (sender == action_button_) {
-    delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_EXECUTE);
-  } else {
-    DCHECK_EQ(dismiss_button_, sender);
-    delegate_->OnBubbleClosed(
-        ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION);
-  }
-  acknowledged_ = true;
-  GetWidget()->Close();
+base::string16 ToolbarActionsBarBubbleViews::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  return button == ui::DIALOG_BUTTON_OK ? delegate_->GetActionButtonText()
+                                        : delegate_->GetDismissButtonText();
 }
 
 void ToolbarActionsBarBubbleViews::LinkClicked(views::Link* link,
                                                int event_flags) {
   delegate_->OnBubbleClosed(ToolbarActionsBarBubbleDelegate::CLOSE_LEARN_MORE);
-  acknowledged_ = true;
+  // Reset delegate so we don't send extra OnBubbleClosed()s.
+  delegate_.reset();
   GetWidget()->Close();
 }