Use standard icon/title in global error bubble

ui/views/bubble/bubble_frame_view.cc

 // Copyright (c) 2012 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 "ui/views/bubble/bubble_frame_view.h"
 
 #include <algorithm>
 
 #include "ui/base/hit_test.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/path.h"
 #include "ui/gfx/screen.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/native_theme/native_theme.h"
 #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"
 
 namespace {
 
 // Insets for the title bar views in pixels.
 const int kTitleTopInset = 12;
 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,
                        const gfx::Rect& window_bounds,
                        bool vertical) {
   if (available_bounds.IsEmpty() || available_bounds.Contains(window_bounds))
     return 0;
 
   //  window_bounds
   //  +---------------------------------+
@@ skipping 11 matching lines @@
 }
 
 }  // namespace
 
 namespace views {
 
 // static
 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);
   AddChildView(title_);
 
   close_ = CreateCloseButton(this);
   close_->SetVisible(false);
   AddChildView(close_);
 }
 
@@ skipping 80 matching lines @@
   }
 }
 
 void BubbleFrameView::ResetWindowControls() {
   close_->SetVisible(GetWidget()->widget_delegate()->ShouldShowCloseButton());
 }
 
 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.
   ResetWindowControls();
 }
 
 void BubbleFrameView::SizeConstraintsChanged() {}
 
 void BubbleFrameView::SetTitleFontList(const gfx::FontList& font_list) {
   title_->SetFontList(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;
 }
 
 gfx::Size BubbleFrameView::GetPreferredSize() const {
   return GetSizeForClientSize(GetWidget()->client_view()->GetPreferredSize());
 }
 
 gfx::Size BubbleFrameView::GetMinimumSize() const {
   return GetSizeForClientSize(GetWidget()->client_view()->GetMinimumSize());
 }
 
 void BubbleFrameView::Layout() {
   gfx::Rect bounds(GetContentsBounds());
   bounds.Inset(GetTitleInsets());
   if (bounds.IsEmpty())
     return;
 
   // The close button top inset is actually smaller than the title top inset.
   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();
     gfx::Size size = titlebar_extra_view_->GetPreferredSize();
     size.SetToMin(gfx::Size(std::max(0, extra_width), size.height()));
     gfx::Rect titlebar_extra_view_bounds(
         close_->x() - size.width(),
         bounds.y(),
         size.width(),
         bounds.height());
@@ skipping 143 matching lines @@
   bubble_border_->set_arrow_offset(
       bubble_border_->GetArrowOffset(window_bounds.size()) - offscreen_adjust);
   if (offscreen_adjust)
     SchedulePaint();
 }
 
 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)
     title_bar_width += titlebar_extra_view_->GetPreferredSize().width();
   gfx::Size size(client_size);
   size.SetToMax(gfx::Size(title_bar_width, 0));
   const gfx::Insets insets(GetInsets());
   size.Enlarge(insets.width(), insets.height());
   return size;
 }
 
 }  // namespace views