Allow dialogs to use a custom View as their title.

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 2dd002e5a7192c4fa1d47b4089a66f845eb4c08c..a689d74725b2eb060d4da8a482c410a1570ccdb5 100644
--- a/ui/views/bubble/bubble_frame_view.cc
+++ b/ui/views/bubble/bubble_frame_view.cc
@@ -25,10 +25,10 @@
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/vector_icons/vector_icons.h"
 #include "ui/views/bubble/bubble_border.h"
+#include "ui/views/bubble/bubble_dialog_delegate.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/image_view.h"
-#include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/layout_provider.h"
 #include "ui/views/resources/grit/views_resources.h"
@@ -81,18 +81,18 @@ BubbleFrameView::BubbleFrameView(const gfx::Insets& title_margins,
       title_margins_(title_margins),
       content_margins_(content_margins),
       title_icon_(new views::ImageView()),
-      title_(nullptr),
+      delegate_title_(nullptr),
       close_(nullptr),
       footnote_container_(nullptr),
       close_button_clicked_(false) {
   AddChildView(title_icon_);
 
-  title_ = new Label(base::string16(), style::CONTEXT_DIALOG_TITLE);
-  title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  title_->set_collapse_when_hidden(true);
-  title_->SetVisible(false);
-  title_->SetMultiLine(true);
-  AddChildView(title_);
+  default_title_ = new Label(base::string16(), style::CONTEXT_DIALOG_TITLE);
+  default_title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  default_title_->set_collapse_when_hidden(true);
+  default_title_->SetVisible(false);
+  default_title_->SetMultiLine(true);
+  AddChildView(default_title_);
 
   close_ = CreateCloseButton(this);
   close_->SetVisible(false);
@@ -174,7 +174,7 @@ int BubbleFrameView::NonClientHitTest(const gfx::Point& point) {
     sys_rect.set_origin(gfx::Point(GetMirroredXForRect(sys_rect), 0));
     if (sys_rect.Contains(point))
       return HTSYSMENU;
-    if (point.y() < title_->bounds().bottom())
+    if (point.y() < title()->bounds().bottom())
       return HTCAPTION;
   }
 
@@ -233,14 +233,34 @@ void BubbleFrameView::UpdateWindowIcon() {
 }
 
 void BubbleFrameView::UpdateWindowTitle() {
-  title_->SetText(GetWidget()->widget_delegate()->GetWindowTitle());
-  title_->SetVisible(GetWidget()->widget_delegate()->ShouldShowWindowTitle());
+  WidgetDelegate* delegate = GetWidget()->widget_delegate();
+  if (delegate_title_) {
+    default_title_->SetVisible(false);
+    BubbleDialogDelegateView* bubble_delegate =
+        delegate->AsBubbleDialogDelegate();
+    if (bubble_delegate)
+      bubble_delegate->PropagateUpdateTitleView(delegate_title_);

[sky, 2017/06/09 23:49:16]

Can you elaborate on why this path is needed? My thinking is if a client
supplies a custom title view they should be responsible for keeping it in sync.
If a client has set a title view, then BubbleFrameView::UpdateWindowTitle()
effectively does nothing.
For example, ManagePasswordsBubbleView::Refresh() is the place that calls
UpdateWindowTitle(). It could just as well internally update title_view_, right?
Doing this means you don't need to add UpdateTitleView or
PropagateUpdateTitleView to the delegate.

Also, what necessitates CreateTitleView()? Couldn't ManagePasswordsBubbleView
directly call BubbleFrameView::SetTitleView()? ManagePaswordsBubbleView can
override AddedToWidget and do this.

[Bret, 2017/06/10 00:52:01]

Hmm I guess I'm confused about the role of UpdateWindowTitle. It seems like some
external event (for example BubbleFrameView::OnThemeChanged) should be able to
tell the frame to update its title. On the other hand, it seems very unlikely
that such an event would create a real change; the subclass ought to know that
it's changing its title. Maybe BubbleFrameView::UpdateWindowTitle isn't actually
important?
I like CreateTitleView because the timing on when to create the title is a bit
subtle (i.e. not in the constructor or in Init() when all other initialization
is happening) and it's much simpler for a dialog creator to just override this
function to get it right.

[Bret, 2017/06/15 22:20:34]

Added a SetupTitleView to BubbleDialogDelegateView that the subclass can call
when it wants to recreate the title view.

[sky, 2017/06/19 15:19:25]

Exactly. I don't think UpdateWindowTitle is at all useful in the case of a
client supplied title view.
I get that the timing is tricky, but I would prefer to avoid more complexity to
the base class by way of additional functions. Can you elaborate on why
overriding AddedToWidget doesn't work in this case?

[Bret, 2017/06/21 22:37:54]

After playing around with it, I ended up removing the new base class methods and
made the subclasses override AddedToWidget directly like you wanted. I agree
that it's better because the subclasses can manage title text updates directly
instead of just destroying and re-creating the title views indirectly.

I still worry about GetWindowTitle getting out of sync with the actual title
text because it's not enforced for them to be the same. I'm dunno what to do
about that. I suppose I could add a complicated DCHECK block that downcasts
delegate_title_... but I'm not sure it's worth doing.

[sky, 2017/06/22 14:48:48]

I tend to think it isn't worth bothering either.

+  } else {
+    default_title_->SetVisible(delegate->ShouldShowWindowTitle());
+    default_title_->SetText(delegate->GetWindowTitle());
+  }
 }
 
 void BubbleFrameView::SizeConstraintsChanged() {}
 
 void BubbleFrameView::SetTitleFontList(const gfx::FontList& font_list) {
-  title_->SetFontList(font_list);
+  // TODO: resolve this
+  default_title_->SetFontList(font_list);
+}
+
+void BubbleFrameView::SetTitleView(View* title_view) {
+  DCHECK(title_view);

[sky, 2017/06/09 23:49:16]

Can you delete default_title_ here if title_view is non-null?

[Bret, 2017/06/15 22:20:34]

Done.

+  if (delegate_title_) {
+    RemoveChildView(delegate_title_);

[sky, 2017/06/09 23:49:16]

delete implicitly removes, so no need to call this first.

[Bret, 2017/06/15 22:20:34]

Done.

+    delete delegate_title_;
+  }
+  delegate_title_ = title_view;
+  AddChildView(delegate_title_);
 }
 
 const char* BubbleFrameView::GetClassName() const {
@@ -251,7 +271,7 @@ gfx::Insets BubbleFrameView::GetInsets() const {
   gfx::Insets insets = content_margins_;
 
   const int icon_height = title_icon_->GetPreferredSize().height();
-  const int label_height = title_->GetPreferredSize().height();
+  const int label_height = title()->GetPreferredSize().height();
   const bool has_title = icon_height > 0 || label_height > 0;
   const int title_padding = has_title ? title_margins_.height() : 0;
   const int title_height = std::max(icon_height, label_height) + title_padding;
@@ -304,7 +324,8 @@ gfx::Size BubbleFrameView::GetMaximumSize() const {
 void BubbleFrameView::Layout() {
   // The title margins may not be set, but make sure that's only the case when
   // there's no title.
-  DCHECK(!title_margins_.IsEmpty() || !title_->visible());
+  DCHECK(!title_margins_.IsEmpty() ||
+         (!default_title_->visible() && !delegate_title_));
 
   const gfx::Rect contents_bounds = GetContentsBounds();
   gfx::Rect bounds = contents_bounds;
@@ -320,24 +341,23 @@ void BubbleFrameView::Layout() {
                  contents_bounds.y() + close_margin));
 
   gfx::Size title_icon_pref_size(title_icon_->GetPreferredSize());
-  int padding = 0;
-  int title_height = title_icon_pref_size.height();
-
-  if (title_->visible() && !title_->text().empty()) {
-    if (title_icon_pref_size.width() > 0)
-      padding = title_margins_.left();
-
-    const int title_label_x =
-        bounds.x() + title_icon_pref_size.width() + padding;
-    title_->SizeToFit(std::max(1, close_->x() - title_label_x));
-    title_height = std::max(title_height, title_->height());
-    title_->SetPosition(gfx::Point(
-        title_label_x, bounds.y() + (title_height - title_->height()) / 2));
-  }
+  const int title_icon_padding =
+      title_icon_pref_size.width() > 0 ? title_margins_.left() : 0;
+  const int title_label_x =
+      bounds.x() + title_icon_pref_size.width() + title_icon_padding;
+  const int title_available_width = std::max(1, close_->x() - title_label_x);
+  const int title_preferred_height =
+      title()->GetHeightForWidth(title_available_width);
+  const int title_height =
+      std::max(title_icon_pref_size.height(), title_preferred_height);
+  title()->SetBounds(title_label_x,
+                     bounds.y() + (title_height - title_preferred_height) / 2,
+                     title_available_width, title_height);
 
   title_icon_->SetBounds(bounds.x(), bounds.y(), title_icon_pref_size.width(),
                          title_height);
-  bounds.set_width(title_->bounds().right() - bounds.x());
+  const int title_right = title()->bounds().right();
+  bounds.set_width(title_right - bounds.x());
   bounds.set_height(title_height);
 
   if (footnote_container_) {
@@ -529,7 +549,7 @@ gfx::Size BubbleFrameView::GetSizeForClientSize(
   // Accommodate the width of the title bar elements.
   int title_bar_width = title_margins_.width() + border()->GetInsets().width();
   gfx::Size title_icon_size = title_icon_->GetPreferredSize();
-  gfx::Size title_label_size = title_->GetPreferredSize();
+  gfx::Size title_label_size = title()->GetPreferredSize();

[sky, 2017/06/09 23:49:16]

In Layout code you use GetHeightForWidth(), and you use the max of the icon
height and the title's preferred height. I don't think the two match. You need
to use GetHeightForWidth() here too to get a consistent preferred size.

[Bret, 2017/06/10 00:52:01]

I think in practice it matches, because when either Label or StyledLabel is laid
out it figures out how tall it is and uses that for its preferred size. I think
having a variable preferred size is a no-no anyway, but that's a different
issue. I'll change this.

[Bret, 2017/06/15 22:20:34]

I investigated this a little more (and wrote a test) and I think this is okay
as-is. The title width depends on the bounds width, which isn't known at this
point, so I'm not sure what else we would do here. The height does end up being
max(title height, icon height) via GetInsets. It all seems actually correct so
I'd rather not change it in this patch.

   if (title_icon_size.width() > 0 && title_label_size.width() > 0)
     title_bar_width += title_margins_.left();
   title_bar_width += title_icon_size.width();