Switch GlobalErrorBubbleView to a BubbleDialogDelegate.

chrome/browser/ui/views/global_error_bubble_view.cc

Index: chrome/browser/ui/views/global_error_bubble_view.cc
diff --git a/chrome/browser/ui/views/global_error_bubble_view.cc b/chrome/browser/ui/views/global_error_bubble_view.cc
index 7d46d5474fae069fa312a904c099ccd7b80fbbe4..660062f16e0d8a0ad204c3502af14d84b9b4e07c 100644
--- a/chrome/browser/ui/views/global_error_bubble_view.cc
+++ b/chrome/browser/ui/views/global_error_bubble_view.cc
@@ -25,24 +25,15 @@
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
+#include "ui/views/window/dialog_client_view.h"
 
 namespace {
 
-enum {
-  TAG_ACCEPT_BUTTON = 1,
-  TAG_CANCEL_BUTTON,
-};
-
-const int kMaxBubbleViewWidth = 262;
+const int kMaxBubbleViewWidth = 362;
 
 // The vertical inset of the app bubble anchor from the app menu button.
 const int kAnchorVerticalInset = 5;
 
-const int kBubblePadding = 19;
-
-// Spacing between bubble text and buttons.
-const int kLabelToButtonVerticalSpacing = 14;
-
 }  // namespace
 
 // GlobalErrorBubbleViewBase ---------------------------------------------------
@@ -55,7 +46,7 @@ GlobalErrorBubbleViewBase* GlobalErrorBubbleViewBase::ShowStandardBubbleView(
   views::View* app_menu_button = browser_view->toolbar()->app_menu_button();
   GlobalErrorBubbleView* bubble_view = new GlobalErrorBubbleView(
       app_menu_button, views::BubbleBorder::TOP_RIGHT, browser, error);
-  views::BubbleDelegateView::CreateBubble(bubble_view);
+  views::BubbleDialogDelegateView::CreateBubble(bubble_view);
   bubble_view->GetWidget()->Show();
   return bubble_view;
 }
@@ -67,13 +58,36 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
     views::BubbleBorder::Arrow arrow,
     Browser* browser,
     const base::WeakPtr<GlobalErrorWithStandardBubble>& error)
-    : BubbleDelegateView(anchor_view, arrow),
+    : BubbleDialogDelegateView(anchor_view, arrow),
       browser_(browser),
-      error_(error) {
-  // Set content margins to left-align the bubble text with the title.
-  // BubbleFrameView adds enough padding below title, no top padding needed.
-  set_margins(gfx::Insets(0, kBubblePadding, kBubblePadding, kBubblePadding));
+      error_(error) {}
+
+GlobalErrorBubbleView::~GlobalErrorBubbleView() {
+  // |elevation_icon_setter_| references |accept_button_|, so make sure it is

[msw, 2016/04/04 17:13:18]

nit: there is no |accept_button_| member, update comment.

[Evan Stade, 2016/04/04 22:59:07]

verified this is no longer needed, removed.

+  // destroyed before |accept_button_|.
+  elevation_icon_setter_.reset();
+}
+
+base::string16 GlobalErrorBubbleView::GetWindowTitle() const {
+  return error_->GetBubbleViewTitle();
+}
+
+gfx::ImageSkia GlobalErrorBubbleView::GetWindowIcon() {
+  gfx::Image image = error_->GetBubbleViewIcon();
+  DCHECK(!image.IsEmpty());
+  return *image.ToImageSkia();
+}
+
+bool GlobalErrorBubbleView::ShouldShowWindowIcon() const {
+  return true;
+}
 
+void GlobalErrorBubbleView::WindowClosing() {
+  if (error_)
+    error_->BubbleViewDidClose(browser_);
+}
+
+void GlobalErrorBubbleView::Init() {
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_view_insets(
       gfx::Insets(kAnchorVerticalInset, 0, kAnchorVerticalInset, 0));
@@ -83,50 +97,17 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
   for (size_t i = 0; i < message_strings.size(); ++i) {
     views::Label* message_label = new views::Label(message_strings[i]);
     message_label->SetMultiLine(true);
-    message_label->SizeToFit(kMaxBubbleViewWidth);
     message_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     message_labels.push_back(message_label);
   }
 
-  base::string16 accept_string(error_->GetBubbleViewAcceptButtonLabel());
-  scoped_ptr<views::BlueButton> accept_button(
-      new views::BlueButton(this, accept_string));
-  accept_button->SetIsDefault(true);
-  accept_button->set_tag(TAG_ACCEPT_BUTTON);
-
-  if (error_->ShouldAddElevationIconToAcceptButton())
-    elevation_icon_setter_.reset(
-        new ElevationIconSetter(
-            accept_button.get(),
-            base::Bind(&GlobalErrorBubbleView::SizeToContents,
-                       base::Unretained(this))));
-
-  base::string16 cancel_string(error_->GetBubbleViewCancelButtonLabel());
-  scoped_ptr<views::LabelButton> cancel_button;
-  if (!cancel_string.empty()) {
-    cancel_button.reset(new views::LabelButton(this, cancel_string));
-    cancel_button->SetStyle(views::Button::STYLE_BUTTON);
-    cancel_button->set_tag(TAG_CANCEL_BUTTON);
-  }
-
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   // First row, message labels.
   views::ColumnSet* cs = layout->AddColumnSet(0);
-  cs->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL,
-                1, views::GridLayout::USE_PREF, 0, 0);
-
-  // Second row, accept and cancel button.
-  cs = layout->AddColumnSet(1);
-  cs->AddPaddingColumn(1, views::kRelatedControlHorizontalSpacing);
-  cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
-                0, views::GridLayout::USE_PREF, 0, 0);
-  if (cancel_button.get()) {
-    cs->AddPaddingColumn(0, views::kRelatedButtonHSpacing);
-    cs->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING,
-                  0, views::GridLayout::USE_PREF, 0, 0);
-  }
+  cs->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
+                views::GridLayout::FIXED, kMaxBubbleViewWidth, 0);
 
   for (size_t i = 0; i < message_labels.size(); ++i) {
     layout->StartRow(1, 0);
@@ -134,16 +115,6 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
     if (i < message_labels.size() - 1)
       layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
   }
-  layout->AddPaddingRow(0, kLabelToButtonVerticalSpacing);
-
-  layout->StartRow(0, 1);
-  layout->AddView(accept_button.release());
-  if (cancel_button.get())
-    layout->AddView(cancel_button.release());
-
-  // Adjust the message label size in case buttons are too long.
-  for (size_t i = 0; i < message_labels.size(); ++i)
-    message_labels[i]->SizeToFit(layout->GetPreferredSize(this).width());
 
   // These bubbles show at times where activation is sporadic (like at startup,
   // or a new window opening). Make sure the bubble doesn't disappear before the
@@ -151,46 +122,51 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
   set_close_on_deactivate(error_->ShouldCloseOnDeactivate());
 }
 
-GlobalErrorBubbleView::~GlobalErrorBubbleView() {
-  // |elevation_icon_setter_| references |accept_button_|, so make sure it is
-  // destroyed before |accept_button_|.
-  elevation_icon_setter_.reset();
+void GlobalErrorBubbleView::UpdateButton(views::LabelButton* button,
+                                         ui::DialogButton type) {
+  BubbleDialogDelegateView::UpdateButton(button, type);
+  if (type == ui::DIALOG_BUTTON_OK &&
+      error_->ShouldAddElevationIconToAcceptButton()) {
+    elevation_icon_setter_.reset(new ElevationIconSetter(
+        button, base::Bind(&GlobalErrorBubbleView::SizeToContents,
+                           base::Unretained(this))));
+  }
 }
 
-void GlobalErrorBubbleView::ButtonPressed(views::Button* sender,
-                                          const ui::Event& event) {
-  if (error_) {
-    if (sender->tag() == TAG_ACCEPT_BUTTON)
-      error_->BubbleViewAcceptButtonPressed(browser_);
-    else if (sender->tag() == TAG_CANCEL_BUTTON)
-      error_->BubbleViewCancelButtonPressed(browser_);
-    else
-      NOTREACHED();
-  }
-  GetWidget()->Close();
+bool GlobalErrorBubbleView::ShouldShowCloseButton() const {
+  return error_ && error_->ShouldShowCloseButton();
 }
 
-base::string16 GlobalErrorBubbleView::GetWindowTitle() const {
-  return error_->GetBubbleViewTitle();
+bool GlobalErrorBubbleView::ShouldDefaultButtonBeBlue() const {
+  return true;
 }
 
-gfx::ImageSkia GlobalErrorBubbleView::GetWindowIcon() {
-  gfx::Image image = error_->GetBubbleViewIcon();
-  DCHECK(!image.IsEmpty());
-  return *image.ToImageSkia();
+base::string16 GlobalErrorBubbleView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  return button == ui::DIALOG_BUTTON_OK
+             ? error_->GetBubbleViewAcceptButtonLabel()
+             : error_->GetBubbleViewCancelButtonLabel();
 }
 
-bool GlobalErrorBubbleView::ShouldShowWindowIcon() const {
+int GlobalErrorBubbleView::GetDialogButtons() const {
+  return ui::DIALOG_BUTTON_OK |
+         (error_->GetBubbleViewCancelButtonLabel().empty()
+              ? 0
+              : ui::DIALOG_BUTTON_CANCEL);
+}
+
+bool GlobalErrorBubbleView::Cancel() {
+  error_->BubbleViewCancelButtonPressed(browser_);
   return true;
 }
 
-void GlobalErrorBubbleView::WindowClosing() {
-  if (error_)
-    error_->BubbleViewDidClose(browser_);
+bool GlobalErrorBubbleView::Accept() {
+  error_->BubbleViewAcceptButtonPressed(browser_);
+  return true;
 }
 
-bool GlobalErrorBubbleView::ShouldShowCloseButton() const {
-  return error_ && error_->ShouldShowCloseButton();
+bool GlobalErrorBubbleView::Close() {
+  return true;

[msw, 2016/04/04 17:13:18]

I think this is slightly non-obvious and deserves a comment, like:
"Do not Accept or Cancel when closed, as most dialogs do, because..."?
I think global error bubbles were meant not to have an (x) close button, because
we wanted to encourage users to take some explicit action on them, hence the
NOTREACHED() in the old GlobalErrorBubbleView::ButtonPressed that I previously
mentioned, but my memory here is fuzzy. +CC Finnur, in case he remembers more.

[Evan Stade, 2016/04/04 22:59:07]

I don't know how to complete this sentence, I'm just maintaining the status quo,
which was hitherto undocumented.
Even if you had a close button and you pressed it, it would not have gone
through ButtonPressed because |this| was not registered as a listener on that
button.

The implementation of GlobalErrorBubbleView::ShouldShowCloseButton() above makes
it seem like some of them are intended to have close buttons.

[msw, 2016/04/05 00:25:26]

You could say "so that closing via dismissal isn't treated as an explicit accept
or cancel response." or even just "to maintain legacy behavior". I just think
overriding Close() to skip calling Accept/Cancel merits a comment.

[Evan Stade, 2016/04/05 03:13:18]

Done.

[Finnur, 2016/04/05 15:19:13]

Late to the party... 

Although I did some bubble work for extensions at the same time, I don't
remember being involved in the GlobalErrorBubbleView so I don't have any
recollections on that front. I do seem to recall that it was Yoyo who
implemented it (who left some time afterwards), but I'm not sure. 

 }
 
 void GlobalErrorBubbleView::CloseBubbleView() {