Use standard icon/title in global error bubble

ui/views/bubble/bubble_frame_view.cc

Index: ui/views/bubble/bubble_frame_view.cc
diff --git a/ui/views/bubble/bubble_frame_view.cc b/ui/views/bubble/bubble_frame_view.cc
index 88628fbb75137774127b2bb6f31e4bc0cac9f532..e564d4e436c4bcc344f1a35243c17df59fd41b89 100644
--- a/ui/views/bubble/bubble_frame_view.cc
+++ b/ui/views/bubble/bubble_frame_view.cc
@@ -15,6 +15,7 @@
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/bubble/bubble_border.h"
 #include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 #include "ui/views/window/client_view.h"
@@ -27,6 +28,9 @@ const int kTitleLeftInset = 19;
 const int kTitleBottomInset = 12;
 const int kTitleRightInset = 7;
 
+// The horizontal padding between the title and the icon.
+const int kTitleHorizontalPadding = 5;
+
 // Get the |vertical| or horizontal amount that |available_bounds| overflows
 // |window_bounds|.
 int GetOffScreenLength(const gfx::Rect& available_bounds,
@@ -60,10 +64,14 @@ const char BubbleFrameView::kViewClassName[] = "BubbleFrameView";
 BubbleFrameView::BubbleFrameView(const gfx::Insets& content_margins)
     : bubble_border_(NULL),

[msw, 2015/02/19 23:56:14]

nit: please update these to nullptr.

[xiaoling, 2015/02/20 19:43:21]

Done.

       content_margins_(content_margins),
+      title_icon_(NULL),
       title_(NULL),
       close_(NULL),
       titlebar_extra_view_(NULL) {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  title_icon_ = new views::ImageView();

[msw, 2015/02/19 23:56:14]

nit: inline this in the initializer list.

[xiaoling, 2015/02/20 19:43:21]

Done.

+  AddChildView(title_icon_);
+
   title_ = new Label(base::string16(),
                      rb.GetFontList(ui::ResourceBundle::MediumFont));
   title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
@@ -164,6 +172,11 @@ void BubbleFrameView::ResetWindowControls() {
 void BubbleFrameView::UpdateWindowIcon() {}
 
 void BubbleFrameView::UpdateWindowTitle() {
+  gfx::ImageSkia image;

[msw, 2015/02/19 23:56:14]

Move this into UpdateWindowIcon(), but you may need to call that from here,
because views::Widget::Init() only calls UpdateWindowTitle(), not
UpdateWindowIcon(). That's probably a bug that can be addressed separately.

[xiaoling, 2015/02/20 19:43:21]

Done. I made views::Widget::Init() calls UpdateWindowIcon() as well. Please let
me know if this is not correct.

[msw, 2015/02/20 20:03:44]

It should be correct, thanks.

+  if (GetWidget()->widget_delegate()->ShouldShowWindowIcon())
+    image = GetWidget()->widget_delegate()->GetWindowIcon();
+  title_icon_->SetImage(&image);
+
   title_->SetText(GetWidget()->widget_delegate()->ShouldShowWindowTitle() ?
       GetWidget()->widget_delegate()->GetWindowTitle() : base::string16());
   // Update the close button visibility too, otherwise it's not intialized.
@@ -178,8 +191,13 @@ void BubbleFrameView::SetTitleFontList(const gfx::FontList& font_list) {
 
 gfx::Insets BubbleFrameView::GetInsets() const {
   gfx::Insets insets = content_margins_;
-  const int title_height = title_->text().empty() ? 0 :
-      title_->GetPreferredSize().height() + kTitleTopInset + kTitleBottomInset;
+
+  const int icon_height = title_icon_->GetPreferredSize().height();
+  const int label_height =
+      title_->text().empty() ? 0 : title_->GetPreferredSize().height();

[msw, 2015/02/19 23:56:14]

Maybe you can title_->set_collapse_when_hidden(true); instead.

[xiaoling, 2015/02/20 19:43:21]

Done.

+  const bool has_title = icon_height > 0 || label_height > 0;
+  const int title_padding = has_title ? kTitleTopInset + kTitleBottomInset : 0;
+  const int title_height = std::max(icon_height, label_height) + title_padding;
   const int close_height = close_->visible() ? close_->height() : 0;
   insets += gfx::Insets(std::max(title_height, close_height), 0, 0, 0);
   return insets;
@@ -203,11 +221,31 @@ void BubbleFrameView::Layout() {
   close_->SetPosition(gfx::Point(bounds.right() - close_->width(),
                                  bounds.y() - 5));
 
-  gfx::Size title_size(title_->GetPreferredSize());
-  const int title_width = std::max(0, close_->x() - bounds.x());
-  title_size.SetToMin(gfx::Size(title_width, title_size.height()));
-  bounds.set_size(title_size);

[msw, 2015/02/19 23:56:14]

nit: the old code updated |bounds| to have the size of the title for layout of
the |titlebar_extra_view_|... You'll need to update |bounds| in the new code to
ensure that's sized and placed correctly.

[xiaoling, 2015/02/20 19:43:21]

Ahh, I missed that. Done.

-  title_->SetBoundsRect(bounds);
+  gfx::Size title_icon_size(title_icon_->GetPreferredSize());
+  gfx::Size title_label_size(title_->GetPreferredSize());

[msw, 2015/02/19 23:56:14]

Wouldn't this have the same defect: non-zero height with empty text?

[xiaoling, 2015/02/20 19:43:21]

Right, I inherited the defect:(

+  const int title_height = std::max(title_icon_size.height(),
+                                    title_label_size.height());
+
+  const int title_icon_width = std::max(0, close_->x() - bounds.x());
+  title_icon_size.SetToMin(gfx::Size(title_icon_width, title_height));
+  gfx::Rect title_icon_bounds(
+      bounds.x(),
+      bounds.y() + (title_height - title_icon_size.height()) / 2,

[msw, 2015/02/19 23:56:14]

Did you fix vertical centering? Please post screenshots to the bug.
(maybe set the height of the label and the icon to the max of each, and rely on
them centering their contents vertically instead of using this naive centering)

[xiaoling, 2015/02/20 19:43:21]

I think yes. My naive way seems to have the same result of your suggestion, but
your way results in cleaner code. I don't know that by default the contents are
center v-aligned and I cannot find a way to set v-alignment. Thanks for the tip.

+      title_icon_size.width(),
+      title_icon_size.height());
+  title_icon_->SetBoundsRect(title_icon_bounds);
+
+  const int title_label_x =
+      title_icon_->bounds().right() + kTitleHorizontalPadding;
+  const int title_label_width = std::max(0, close_->x() - title_label_x);
+  title_label_size.SetToMin(gfx::Size(title_label_width,
+                                      title_label_size.height()));
+  gfx::Rect title_label_bounds(
+      title_label_x,
+      bounds.y() + (title_height - title_label_size.height()) / 2,
+      title_label_size.width(),
+      title_label_size.height());
+  title_->SetBoundsRect(title_label_bounds);
 
   if (titlebar_extra_view_) {
     const int extra_width = close_->x() - title_->bounds().right();
@@ -371,8 +409,15 @@ gfx::Size BubbleFrameView::GetSizeForClientSize(
     const gfx::Size& client_size) const {
   // Accommodate the width of the title bar elements.
   int title_bar_width = GetInsets().width() + border()->GetInsets().width();
+  gfx::Size title_icon_size = title_icon_->GetPreferredSize();
+  if (title_icon_size.width() > 0 || !title_->text().empty())
+    title_bar_width += kTitleLeftInset;
+  if (title_icon_size.width() > 0 && !title_->text().empty())
+    title_bar_width += kTitleHorizontalPadding;
+  if (title_icon_size.width() > 0)
+    title_bar_width += title_icon_size.width();
   if (!title_->text().empty())
-    title_bar_width += kTitleLeftInset + title_->GetPreferredSize().width();
+    title_bar_width += title_->GetPreferredSize().width();
   if (close_->visible())
     title_bar_width += close_->width() + 1;
   if (titlebar_extra_view_ != NULL)