Use GridLayout for DialogClientView's button row.

ui/views/window/dialog_client_view.cc

Index: ui/views/window/dialog_client_view.cc
diff --git a/ui/views/window/dialog_client_view.cc b/ui/views/window/dialog_client_view.cc
index deff7dc0a4423055a6afe1d258425abbdd327cff..2bacf40d2c51c507094b0cb09b710f49abfb9a73 100644
--- a/ui/views/window/dialog_client_view.cc
+++ b/ui/views/window/dialog_client_view.cc
@@ -15,6 +15,7 @@
 #include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/md_text_button.h"
+#include "ui/views/layout/grid_layout.h"
 #include "ui/views/style/platform_style.h"
 #include "ui/views/views_delegate.h"
 #include "ui/views/widget/widget.h"
@@ -40,27 +41,36 @@ bool ShouldShow(View* view) {
   return view && view->visible();
 }
 
-// Do the layout for a button.
-void LayoutButton(LabelButton* button,
-                  gfx::Rect* row_bounds,
-                  int button_height) {
-  if (!button)
-    return;
-
-  const gfx::Size size = button->GetPreferredSize();
-  row_bounds->set_width(row_bounds->width() - size.width());
-  DCHECK_LE(button_height, row_bounds->height());
-  button->SetBounds(
-      row_bounds->right(),
-      row_bounds->y() + (row_bounds->height() - button_height) / 2,
-      size.width(), button_height);
-  const int spacing =
-      ViewsDelegate::GetInstance()->GetDialogRelatedButtonHorizontalSpacing();
-  row_bounds->set_width(row_bounds->width() - spacing);
+// Arrange two sizes vertically and return the bounding box.

[Peter Kasting, 2017/02/25 06:04:07]

Nit: Descriptive ("Arranges"), not imperative.

Maybe "Returns the bounding box required to contain |above| and |below|, placed
one atop the other."

This also suggests the function should be GetBoundingSizeForVerticalStack() or
something similar that implies what it actually returns, and doesn't imply it
positions something.

[tapted, 2017/02/27 10:04:19]

Done.

+gfx::Size ArrangeVertically(const gfx::Size& above, const gfx::Size& below) {

[Peter Kasting, 2017/02/25 06:04:07]

Nit: Trivial, but it technically doesn't matter that one is above and one is
below; they could be in the other order.  I'd maybe name them |size1| and
|size2| :)

[tapted, 2017/02/27 10:04:19]

Done.

+  return gfx::Size(std::max(above.width(), below.width()),
+                   above.height() + below.height());
 }
 
 }  // namespace
 
+// Simple container to bubble child view changes up the view hierarchy.
+class DialogClientView::ButtonRowContainer : public View {
+ public:
+  ButtonRowContainer() {}
+
+  // Downcast so that ButtonRowContainer can call protected methods as a friend.
+  DialogClientView* parent() {
+    return static_cast<DialogClientView*>(View::parent());

[Peter Kasting, 2017/02/25 06:04:07]

Nit: It would be safer to take the DialogClientView* that parents us as a
constructor param and store it.  That would prevent anyone from changing layout
in a way that makes the ButtonRowContainer a child of a child of the
DialogClientView, or something, leading to really weird failures.

This would also be useful for my next thought.

[tapted, 2017/02/27 10:04:19]

Done.

+  }
+
+  // View:
+  void ChildPreferredSizeChanged(View* child) override {
+    parent()->ChildPreferredSizeChanged(child);

[Peter Kasting, 2017/02/25 06:04:07]

Nit: Optional, but seems like the only reason we need to bubble these two events
is for the parent to do "if (child_ == extra_view) SomethingInvolvingLayout();"

I wonder if that functionality shouldn't be in here instead.  The idea would be
that the parts of layout affecting the non-button-row, which would be our
parent's responsibility, wouldn't need to run when this changed; whereas only
the layout for this container itself would be affected when something about the
extra view changes.

Really, the question here is whether we can cleave more functionality out of the
parent and put it here instead (basically, almost everything you're adding in
this CL).

[tapted, 2017/02/27 10:04:19]

This crossed my mind too.. but I decided not to attempt it because all the other
lifetime management is the responsibility of DialogClientView - e.g. DCV
provides accessors for ok_button/cancel_button. I'm tempted to try this in a
follow-up, but I'm not certain it will improve things :/

[Peter Kasting, 2017/02/28 03:33:23]

I'm fine leaving it to a followup.  I think it's worth trying, but it will be a
fairly large change.

I don't expect there to be less code overall when it's done, but I do think it
would be nice for the scope of various overrides to be clearer (DialogClientView
itself will be doing less, which will make it easier to reason about what it
needs to do).

[tapted, 2017/02/28 06:52:42]

Acknowledged.

+  }
+  void ChildVisibilityChanged(View* child) override {
+    parent()->ChildVisibilityChanged(child);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ButtonRowContainer);
+};
+
 ///////////////////////////////////////////////////////////////////////////////
 // DialogClientView, public:
 
@@ -71,9 +81,14 @@ DialogClientView::DialogClientView(Widget* owner, View* contents_view)
   // Doing this now ensures this accelerator will have lower priority than
   // one set by the contents view.
   AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
+  button_row_container_ = new ButtonRowContainer();
+  AddChildView(button_row_container_);
 }
 
 DialogClientView::~DialogClientView() {
+  // The |extra_view_| can't participate in layout when it is not visible, so it
+  // might not be in the View hierarchy. Ensure it gets deleted.
+  delete extra_view_;

[Peter Kasting, 2017/02/25 06:04:07]

I'm confused.

Visibility and being part of the view hierarachy are not normally in lockstep
(invisible views can be part of the hierarchy).

Why would we have a non-null extra_view_ that's not a child view?

[tapted, 2017/02/27 10:04:19]

This is to satisfy some existing use-cases for DialogClientView. GridLayout
doesn't consider visibility when doing layout, but the old DCV logic did. Since
DCV client code doesn't "own" the ExtraView once created, if the dialog later
decides it doesn't really want an ExtraView, it currently makes it hidden. When
that happens, the extra view shouldn't affect layout.

The alternative to just to plonk the extra view as a child (bypassing the
LayoutManager), even though it is hidden doesn't feel right. E.g. it can't
actually be added to |button_row_container_| -- the GridLayout will object with
`grid_layout.cc(766)] Check failed: host_ == host && adding_view_.` So we could
add it as a sibling of |button_row_container_|, but if we're moving it between
hierarchies we might as well just move it out.. Haven't come up with a neater
solution to this :/

[Peter Kasting, 2017/02/28 03:33:23]

Where does this happen?

Basically, I'm trying to figure out the control flow in the current code that
will allow |extra_view_| to be non-null yet not a child view.  I can't find it.

[tapted, 2017/02/28 06:52:42]

Most overrides of DCV::CreateExtraView() hold on to the View* that they return,
so at any later time they can do extra_view->SetVisible(false); That will
trigger a call into ChildVisibilityChanged(..), which does UpdateDialogButtons()
and ShouldShow(extra_view_) will now report "no", so won't add it as a child
view.

But! Writing this out just made me realise that if the client later decides to
do extra_view->SetVisible(true) This won't be picked up any more since the extra
view is no longer in the hierarchy ಠ_ಠ. Ugh.

http://crbug.com/165193 is the bug that precipitated hiding of extra_view. 

But that was for RequestAutocomplete, which was removed in
http://crrev.com/391821

So.. I thought maybe we can just remove this logic: If you provide an ExtraView,
it must remain visible (or you can just `delete` it too - that would probably
work better). But DesktopMediaPickerDialogView is now also using this
functionality for |audio_share_checkbox_|, so I think this needs to stay, but I
need to fix the sequence of SetVisible(false) -> SetVisible(true).
(Done+Tested).

 }
 
 void DialogClientView::AcceptWindow() {
@@ -93,8 +108,8 @@ void DialogClientView::CancelWindow() {
 }
 
 void DialogClientView::UpdateDialogButtons() {
-  SetupViews();
-  SetupFocusChain();
+  SetupLayout();
+  InvalidateLayout();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -120,90 +135,30 @@ const DialogClientView* DialogClientView::AsDialogClientView() const {
 // DialogClientView, View overrides:
 
 gfx::Size DialogClientView::GetPreferredSize() const {
-  // Initialize the size to fit the buttons and extra view row.
-  gfx::Size size(
-      (ok_button_ ? ok_button_->GetPreferredSize().width() : 0) +
-          (cancel_button_ ? cancel_button_->GetPreferredSize().width() : 0) +
-          (cancel_button_ && ok_button_
-               ? ViewsDelegate::GetInstance()
-                     ->GetDialogRelatedButtonHorizontalSpacing()
-               : 0) +
-          (ShouldShow(extra_view_) ? extra_view_->GetPreferredSize().width()
-                                   : 0) +
-          GetExtraViewSpacing(),
-      0);
-
-  int buttons_height = GetButtonsAndExtraViewRowHeight();
-  if (buttons_height != 0) {
-    size.Enlarge(0, buttons_height);
-    // Inset the buttons and extra view.
-    const gfx::Insets insets = GetButtonRowInsets();
-    size.Enlarge(insets.width(), insets.height());
-  }
-
-  // Increase the size as needed to fit the contents view.
-  // NOTE: The contents view is not inset on the top or side client view edges.
-  gfx::Size contents_size = contents_view()->GetPreferredSize();
-  size.Enlarge(0, contents_size.height());
-  size.set_width(std::max(size.width(), contents_size.width()));
+  return ArrangeVertically(ClientView::GetPreferredSize(),
+                           button_row_container_->GetPreferredSize());
+}
 
-  size.SetToMax(minimum_size_);
+gfx::Size DialogClientView::GetMinimumSize() const {
+  return ArrangeVertically(ClientView::GetMinimumSize(),
+                           button_row_container_->GetMinimumSize());
+}
 
-  return size;
+gfx::Size DialogClientView::GetMaximumSize() const {
+  // The button layout doesn't currently have a maximum size.

[Peter Kasting, 2017/02/25 06:04:07]

Maybe it should?  Then we could probably just use the helper here the same way
we do above, unless it's important to return 0 when the client view has an empty
size but there is a button row.

[tapted, 2017/02/27 10:04:19]

Maximum size would need slightly different logic (i.e. std::min instead of
std::max). Also there's more possibility for a constraints violation then. E.g.
We'd have to think about what happens if the contents_view() minimum width is
larger than the button row maximum width.

(there is already the possible violation where the contents_view() maximum width
is smaller than the button row minimum width: contents_view() currently gets
ignored in this case - but it's unlikely to arise: contents_views() that care
about this tend to be webview dialogs which don't have native buttons).

+  DCHECK(gfx::Size() == button_row_container_->GetMaximumSize());

[Peter Kasting, 2017/02/25 06:04:07]

Nit: DCHECK(button_row_container_->GetMaximumSize().IsEmpty())?

[tapted, 2017/02/27 10:04:19]

The maximum size can be constrained in a single direction by leaving one axis 0
- IsEmpty() would return true in this case, so gfx::Size() gives a stronger
check (note DCHECK_EQ didn't work here because ostream). Updated the comment.

+  gfx::Size max_size = ClientView::GetMaximumSize();
+  if (max_size != gfx::Size())

[Peter Kasting, 2017/02/25 06:04:07]

Nit: !IsEmpty()?

[tapted, 2017/02/27 10:04:19]

Changed this to

  if (max_size.height() != 0)

and added a comment. (the width constraint [or lack of it] should just pass
through without affecting things).

added a DLOG for the possible constraints violation.. WDYT?

[Peter Kasting, 2017/02/28 03:33:23]

I don't think we should add a DLOG, because it's not clear how you'll collect
the data or take action, and also because it makes it unclear how bad this is in
practice or whether we even expect it to occur.

So if you're really worried this could occur in practice and we need to find it
and fix it if so, CHECK, and then file a bug to remove the CHECK if it doesn't
find anything for a while.  If you think this can't occur in practice and we
need to assure readers of that, DCHECK.  If it can't occur in practice and
readers need not consider the issue, remove this entirely.

[tapted, 2017/02/28 06:52:42]

Yeah the DLOG would only be an indicator for developers. It's expensive. I think
it can occur in practice, but I don't think we have reliable control over it,
since the ContentsView() might be a webview. I'm happy to remove it. It's more
of a note to say "there are cases where constraints can't be satisfied", which
can be a comment too.

+    max_size.Enlarge(0, button_row_container_->GetPreferredSize().height());
+  return max_size;
 }
 
 void DialogClientView::Layout() {
-  gfx::Rect bounds = GetContentsBounds();
-
-  // Layout the row containing the buttons and the extra view.
-  if (has_dialog_buttons() || ShouldShow(extra_view_)) {
-    gfx::Insets button_row_insets = GetButtonRowInsets();
-    // Don't apply the top inset here because it's supposed to go above the
-    // buttons, not at the top of the dialog.
-    bounds.Inset(button_row_insets.left(), 0, button_row_insets.right(),
-                 button_row_insets.bottom());
-    const int height = GetButtonsAndExtraViewRowHeight();
-    gfx::Rect row_bounds(bounds.x(), bounds.bottom() - height,
-                         bounds.width(), height);
-    // If the |extra_view_| is a also button, then the |button_height| is the
-    // maximum height of the three buttons, otherwise it is the maximum height
-    // of the ok and cancel buttons.
-    const int button_height =
-        CustomButton::AsCustomButton(extra_view_) ? height : GetButtonHeight();
-    if (kIsOkButtonOnLeftSide) {
-      LayoutButton(cancel_button_, &row_bounds, button_height);
-      LayoutButton(ok_button_, &row_bounds, button_height);
-    } else {
-      LayoutButton(ok_button_, &row_bounds, button_height);
-      LayoutButton(cancel_button_, &row_bounds, button_height);
-    }
-    if (extra_view_) {
-      int custom_padding = 0;
-      if (has_dialog_buttons() &&
-          GetDialogDelegate()->GetExtraViewPadding(&custom_padding)) {
-        // The padding between buttons applied in LayoutButton() will already
-        // have accounted for some of the distance here.
-        custom_padding -= ViewsDelegate::GetInstance()
-                              ->GetDialogRelatedButtonHorizontalSpacing();
-        row_bounds.set_width(row_bounds.width() - custom_padding);
-      }
-      row_bounds.set_width(std::min(row_bounds.width(),
-                                    extra_view_->GetPreferredSize().width()));
-      extra_view_->SetBoundsRect(row_bounds);
-    }
-
-    if (height > 0) {
-      // Inset to the top of the buttons, plus their top padding, in order to
-      // exclude that area from the content view's bounds.
-      bounds.Inset(0, 0, 0, height + button_row_insets.top());
-    }
-  }
-
-  // Layout the contents view to the top and side edges of the contents bounds.
-  // NOTE: The local insets do not apply to the contents view sides or top.
-  const gfx::Rect contents_bounds = GetContentsBounds();
-  contents_view()->SetBounds(contents_bounds.x(), contents_bounds.y(),
-      contents_bounds.width(), bounds.bottom() - contents_bounds.y());
+  button_row_container_->SetSize(
+      gfx::Size(width(), button_row_container_->GetHeightForWidth(width())));
+  button_row_container_->SetY(height() - button_row_container_->height());
+  if (contents_view())
+    contents_view()->SetSize(gfx::Size(width(), button_row_container_->y()));
 }
 
 bool DialogClientView::AcceleratorPressed(const ui::Accelerator& accelerator) {
@@ -217,23 +172,31 @@ void DialogClientView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   View* const child = details.child;
 
-  // Dialogs must add children to contents_view(), not client_view().
-  if (details.is_add && details.parent == this) {
-    DCHECK(child == contents_view() || child == ok_button_ ||

[tapted, 2017/02/24 06:55:10]

Ah, this DCHECK I added to get a jump on dialogs that might be inserting views
in the wrong place, which would have been an issue if the layout was in |this|,
but now that the layout is in a child view it's not really relevant. (so this
can stay or go).

-           child == cancel_button_ || child == extra_view_);
-  }
-
   ClientView::ViewHierarchyChanged(details);
-  if (details.is_add && child == this) {
-    UpdateDialogButtons();
-  } else if (!details.is_add) {
-    if (child == ok_button_)
-      ok_button_ = nullptr;
-    else if (child == cancel_button_)
-      cancel_button_ = nullptr;
-    else if (child == extra_view_)
-      extra_view_ = nullptr;
+
+  if (details.is_add) {
+    if (child == this)
+      UpdateDialogButtons();
+    return;
   }
+
+  if (details.parent != button_row_container_)
+    return;
+
+  // SetupViews() removes all children, managing data members itself.
+  if (in_setup_views_)
+    return;
+
+  // Otherwise, this should only happen during teardown. Ensure there are no
+  // references to deleted Views.

[Peter Kasting, 2017/02/25 06:04:07]

Is this important?  Are there races or something that result in bustage if we
don't null these out?  It seems suspicious that we need most of this code at
all.

[tapted, 2017/02/27 10:04:19]

It looks like this trend started a long time ago while fixing
http://crbug.com/241451 . That was a use-after-free triggered from a
DialogClientView method which no longer exists (OnWillChangeFocus).

I think it is safe to remove the lines that clear out ok_button_ and
cancel_button_. It's probably OK to remove extra_view_ too -- it's dereferenced
a bit more often and DCV has less control over it. E.g. a dialog may decide to
`delete extra_view;` -- currently that works (and would even allow a new extra
view to be created..)

There is a test that would fail for this though. It does:

TEST_F(DialogClientViewTest, RemoveAndUpdateButtons) {
  // Removing buttons from another context should clear the local pointer.
  SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
  delete client_view()->ok_button();

So we would have to decide that that test case is no longer valid - maybe
something better for a follow-up.

[Peter Kasting, 2017/02/28 03:33:23]

Hmm, I didn't realize we had public accessors for these buttons.  Given that we
do, I don't think we can remove this block entirely, unless we can kill those
accessors entirely, at least for non-test code.  They're mostly used in test
code today, and maybe it would be OK to have them work unintuitively when
they're test-only, but DialogDelegate::GetInitiallyFocusedView() uses them, and
I think that use is probably both appropriate and mandates that we keep this.

|extra_view_| has to be kept too, I think; we can't avoid nulling it out here if
we're going to do things like pointer-compare it in ChildPreferredSizeChanged(),
lest we want to risk really weird bugs.

[tapted, 2017/02/28 06:52:42]

Acknowledged.

+  button_row_container_->SetLayoutManager(nullptr);
+
+  if (child == ok_button_)
+    ok_button_ = nullptr;
+  else if (child == cancel_button_)
+    cancel_button_ = nullptr;
+  else if (child == extra_view_)
+    extra_view_ = nullptr;
 }
 
 void DialogClientView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
@@ -276,21 +239,37 @@ void DialogClientView::ChildPreferredSizeChanged(View* child) {
 }
 
 void DialogClientView::ChildVisibilityChanged(View* child) {
+  // Showing or hiding |extra_view_| can alter which columns have linked sizes.
+  if (child == extra_view_)
+    UpdateDialogButtons();
   ChildPreferredSizeChanged(child);
 }
 
-LabelButton* DialogClientView::CreateDialogButton(ui::DialogButton type) {
-  const base::string16 title = GetDialogDelegate()->GetDialogButtonLabel(type);
+void DialogClientView::UpdateDialogButton(LabelButton** member,
+                                          ui::DialogButton type) {
+  DialogDelegate* const delegate = GetDialogDelegate();
+  if ((delegate->GetDialogButtons() & type) == 0) {
+    delete *member;
+    *member = nullptr;
+    return;
+  }
+
+  if (*member) {
+    delegate->UpdateButton(*member, type);
+    return;
+  }
+
+  const base::string16 title = delegate->GetDialogButtonLabel(type);
   LabelButton* button = nullptr;
 
-  const bool is_default =
-      GetDialogDelegate()->GetDefaultDialogButton() == type &&
-      (type != ui::DIALOG_BUTTON_CANCEL ||
-       PlatformStyle::kDialogDefaultButtonCanBeCancel);
+  const bool is_default = delegate->GetDefaultDialogButton() == type &&
+                          (type != ui::DIALOG_BUTTON_CANCEL ||
+                           PlatformStyle::kDialogDefaultButtonCanBeCancel);
 
   // The default button is always blue in Harmony.
-  if (is_default && (ui::MaterialDesignController::IsSecondaryUiMaterial() ||
-                     GetDialogDelegate()->ShouldDefaultButtonBeBlue())) {
+  if (is_default &&
+      (ui::MaterialDesignController::IsSecondaryUiMaterial() ||
+       delegate->ShouldDefaultButtonBeBlue())) {
     button = MdTextButton::CreateSecondaryUiBlueButton(this, title);
   } else {
     button = MdTextButton::CreateSecondaryUiButton(this, title);
@@ -303,66 +282,13 @@ LabelButton* DialogClientView::CreateDialogButton(ui::DialogButton type) {
   button->SetMinSize(gfx::Size(minimum_width, 0));
 
   button->SetGroup(kButtonGroup);
-  return button;
-}
-
-int DialogClientView::GetButtonHeight() const {
-  return std::max(
-      ok_button_ ? ok_button_->GetPreferredSize().height() : 0,
-      cancel_button_ ? cancel_button_->GetPreferredSize().height() : 0);
-}
-
-int DialogClientView::GetExtraViewHeight() const {
-  return ShouldShow(extra_view_) ? extra_view_->GetPreferredSize().height() : 0;
-}
-
-int DialogClientView::GetButtonsAndExtraViewRowHeight() const {
-  return std::max(GetExtraViewHeight(), GetButtonHeight());
-}
-
-gfx::Insets DialogClientView::GetButtonRowInsets() const {
-  if (GetButtonsAndExtraViewRowHeight() == 0)
-    return gfx::Insets();
-
-  // Some subclasses of DialogClientView, in order to do their own layout, set
-  // button_row_insets_ to gfx::Insets(). To avoid breaking behavior of those
-  // dialogs, supplying 0 for the top inset of the row falls back to
-  // ViewsDelegate::GetRelatedControlVerticalSpacing.
-  // TODO(bsep): The top inset should never be 0 when harmony is enabled.
-  const int top = button_row_insets_.top() == 0
-                      ? ViewsDelegate::GetInstance()
-                            ->GetDialogRelatedControlVerticalSpacing()
-                      : button_row_insets_.top();
-  return gfx::Insets(top, button_row_insets_.left(),
-                     button_row_insets_.bottom(), button_row_insets_.right());
-}
 
-void DialogClientView::SetupFocusChain() {
-  // Create a vector of child views in the order of intended focus.
-  std::vector<View*> child_views;
-  child_views.push_back(contents_view());
-  child_views.push_back(extra_view_);
-  if (kIsOkButtonOnLeftSide) {
-    child_views.push_back(ok_button_);
-    child_views.push_back(cancel_button_);
-  } else {
-    child_views.push_back(cancel_button_);
-    child_views.push_back(ok_button_);
-  }
-
-  // Remove all null views from the vector.
-  child_views.erase(
-      std::remove(child_views.begin(), child_views.end(), nullptr),
-      child_views.end());
-
-  // Setup focus by reordering views. It is not safe to use SetNextFocusableView
-  // since child views may be added externally to this view.
-  for (size_t i = 0; i < child_views.size(); i++)
-    ReorderChildView(child_views[i], i);
+  *member = button;
+  delegate->UpdateButton(button, type);
 }
 
 int DialogClientView::GetExtraViewSpacing() const {
-  if (!ShouldShow(extra_view_) || !has_dialog_buttons())
+  if (!ShouldShow(extra_view_) || !(ok_button_ || cancel_button_))
     return 0;
 
   int extra_view_padding = 0;
@@ -373,41 +299,129 @@ int DialogClientView::GetExtraViewSpacing() const {
       ->GetDialogRelatedButtonHorizontalSpacing();
 }
 
-void DialogClientView::SetupViews() {
-  const int buttons = GetDialogDelegate()->GetDialogButtons();
+std::vector<View*> DialogClientView::ButtonRowViews() {
+  View* first = ShouldShow(extra_view_) ? extra_view_ : nullptr;
+  View* second = cancel_button_;
+  View* third = ok_button_;
+  if (kIsOkButtonOnLeftSide)
+    std::swap(second, third);
+  std::vector<View*> views = {first, second, third};
 
-  if (buttons & ui::DIALOG_BUTTON_OK) {
-    if (!ok_button_) {
-      ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
-      AddChildView(ok_button_);
-    }
+  // Remove all null views from the vector.
+  views.erase(std::remove(views.begin(), views.end(), nullptr), views.end());
+  return views;
+}
 
-    GetDialogDelegate()->UpdateButton(ok_button_, ui::DIALOG_BUTTON_OK);
-  } else if (ok_button_) {
-    delete ok_button_;
-    ok_button_ = nullptr;
+void DialogClientView::SetupLayout() {
+  GridLayout* layout = new GridLayout(button_row_container_);
+  layout->set_minimum_size(minimum_size_);
+
+  // Clobber any existing LayoutManager since it has weak references to child
+  // Views which may be removed by SetupViews().
+  button_row_container_->SetLayoutManager(layout);
+  SetupViews();
+  const std::vector<View*> views = ButtonRowViews();
+  if (views.empty())
+    return;
+
+  gfx::Insets insets = button_row_insets_;
+  // Support dialogs that clear |button_row_insets_| to do their own layout.
+  // They expect GetDialogRelatedControlVerticalSpacing() in this case.
+  // TODO(tapted): Remove this under Harmony.
+  if (insets.top() == 0) {
+    const int top =
+        ViewsDelegate::GetInstance()->GetDialogRelatedControlVerticalSpacing();
+    insets.Set(top, insets.left(), insets.bottom(), insets.right());

[Peter Kasting, 2017/02/25 06:04:07]

Nit: We should probably just add per-side setters for gfx::Insets...

[tapted, 2017/02/27 10:04:19]

I hunted through codesearch, and did some "bruteforce" refactoring (renamed Set
to SetAll). But surprisingly.. this seems to be the only caller to Set(..) that
wants to change only one side. Other use cases are for mirroring (e.g. for rtl),
or often to set a *single* side with the rest all being set explicitly to zero
(i.e. when overriding some default Insets).

Since this occurrence has a TODO(..) deleteme, it might be a rare enough
occurrence to only want to set one side that we would rather callers have to
think what they really should do with the other sides as well..

[Peter Kasting, 2017/02/28 03:33:23]

Hmm.  I just wanted to set one side the other day, and found a way to go another
route instead because this didn't exist :(
I'm willing to live without it for now.

   }
 
-  if (buttons & ui::DIALOG_BUTTON_CANCEL) {
-    if (!cancel_button_) {
-      cancel_button_ = CreateDialogButton(ui::DIALOG_BUTTON_CANCEL);
-      AddChildView(cancel_button_);
-    }
+  // The |resize_percent| constants. There's only one stretchy column (padding
+  // to the left of ok/cancel buttons).
+  constexpr float kFixed = 0.f;
+  constexpr float kStretchy = 1.f;
+
+  // Button row is [ extra <pad+stretchy> second <pad> third ]. Ensure the <pad>
+  // column is zero width if there isn't a button on either side.
+  // GetExtraViewSpacing() handles <pad+stretchy>.
+  const int button_spacing =
+      (ok_button_ && cancel_button_)
+          ? ViewsDelegate::GetInstance()
+                ->GetDialogRelatedButtonHorizontalSpacing()
+          : 0;
+
+  constexpr int kButtonRowId = 0;
+  ColumnSet* column_set = layout->AddColumnSet(kButtonRowId);
+
+  // Rather than giving |button_row_container_| a Border, incorporate the insets
+  // into the layout. This simplifies min/max size calculations.
+  column_set->AddPaddingColumn(kFixed, insets.left());
+  column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, kFixed,
+                        GridLayout::USE_PREF, 0, 0);
+  column_set->AddPaddingColumn(kStretchy, GetExtraViewSpacing());
+  column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, kFixed,
+                        GridLayout::USE_PREF, 0, 0);
+  column_set->AddPaddingColumn(kFixed, button_spacing);
+  column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, kFixed,
+                        GridLayout::USE_PREF, 0, 0);
+  column_set->AddPaddingColumn(kFixed, insets.right());
+
+  // Track which columns to link sizes under MD.
+  constexpr int kExtraViewColumnIndex = 1;
+  constexpr int kSecondColumnIndex = 3;
+  constexpr int kThirdColumnIndex = 5;
+  int link[] = {-1, -1, -1};
+  size_t index = 0;
+
+  layout->StartRowWithPadding(kFixed, kButtonRowId, kFixed, insets.top());
+
+  // First column is only used if there is an extra view.

[Peter Kasting, 2017/02/25 06:04:07]

I feel like with some cleverness this block and the ones below could all be put
into a loop and we could eliminate some of the constants above.  But I am tired
enough not to want to spend time figuring out how to do it in detail.  Something
about creating an array of bools like {ShouldShow(extra_view_), ok_button_ &&
cancel_button_, ???} and then just looping through these, setting the link
values to the current column in the GridLayout (which I dunno if it even has an
accessor for, but maybe it should).

[tapted, 2017/02/27 10:04:19]

I'd toyed with this a bit.. The ButtonRowViews() approach was as far as I could
go before the code made me unhappy - the logic here is subtle, so it feels good
to me to play it out in a sequence.

GridLayout doesn't really have any accessors (for _anything_ -> all you can get
is a ColumnSet* from an id, and all you can get from a ColumnSet* is its id and
the number of columns in it). Exposing a current column could be tricky because
there's some subtlety around the skipping of padding columns (e.g. does the
skipped column advance at the end of AddView() or at the start of AddView()?)

[Peter Kasting, 2017/02/28 03:33:23]

OK.
Probably moot.  But, the "current column" should always point to where the next
non-skipped item will be added, rather than wherever the last non-skipped item
was added.  So, "end", IIUC.

[tapted, 2017/02/28 06:52:42]

I got an idea for this. (extra_view_ gets in the way a bit for the reasons
above, but there is a loop now :)

+  if (ShouldShow(extra_view_)) {
+    layout->AddView(views[index]);
+    link[index] = kExtraViewColumnIndex;
+    ++index;
+  } else {
+    layout->SkipColumns(1);
+  }
 
-    GetDialogDelegate()->UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
-  } else if (cancel_button_) {
-    delete cancel_button_;
-    cancel_button_ = nullptr;
+  // Second column is only used if there are 2 buttons.
+  if (ok_button_ && cancel_button_) {
+    layout->AddView(views[index]);
+    link[index] = kSecondColumnIndex;
+    ++index;
+  } else {
+    layout->SkipColumns(1);
+  }
+
+  // Last column is only used if a view remains (either OK or Cancel).
+  if (index < views.size()) {
+    layout->AddView(views[index]);
+    link[index] = kThirdColumnIndex;
   }
 
+  if (ui::MaterialDesignController::IsSecondaryUiMaterial()) {

[Peter Kasting, 2017/02/25 06:04:08]

Should we just go ahead and link the column sizes in pre-MD too?

Yeah, it's a behavior change, but seems like one we could maybe just make.

[tapted, 2017/02/27 10:04:19]

Hm - that scares me a little. It's not just that some dialogs may get suddenly
wide due to long strings. But also some dialogs will become inconsistent with
DialogClientView-based dialogs. E.g. collected cookies view manages its own
buttons. Also more visible things like the `add bookmark` bubble return `NONE`
from DialogDelegate::GetDialogButtons() to do their own management of buttons.

(for those it should be a lot simpler though!  They are both already using
GridLayout - they just need to link columns)

[Peter Kasting, 2017/02/28 03:33:23]

Well, we're only enlarging linked widths when they're "near" in size, right?  So
hopefully the scope of the enlarging is limited.
I don't know that I follow.  Are you saying that this will cause size linking
for dialogs that use DialogClientView, but since not every dialog uses
DialogClientView, that means we'll have behavioral inconsistency?

If so, we have to fix that in Harmony anyway, right?  Either by making these
actually use DialogClientView (preferable), or manually reimplementing this
everywhere that does this (ugh).

At which point we can also Just Fix all those places for pre-Harmony.  Or not;
I'm willing to live with an inconsistency like this for a few releases
pre-Harmony, as it's minor enough I doubt any users would even notice.
Feel free to file bugs on making these dialogs be "real dialogs that don't
manage their own buttons" unless it seems like a bad idea.  I'd like to get to a
point where all our dialogs are using a single common dialog framework, and then
if we change a behavior like this in one place, it changes everywhere.

I haven't looked at these dialogs as recently as you so I don't know if maybe
there's a good reason why they simply can't do that.

[tapted, 2017/02/28 06:52:42]

That does tend to be the case. I couldn't find any _really_ long strings.
"Create a new profile" was the longest, in a dialog with 3 buttons, so it can
get quite wide. 950 pixels across in Bulgarian.

I haven't landed the ignore-buttons-wider-than-xx-px change yet. If I get that
in first this won't be so bad (but so close to branch point, I'd want to get
that other CL in first :).


I'm happy to wait for that and remove this line. Although one concern is that
there's a regression in M58 for a dialog currently, and the CL in review to fix
it at https://codereview.chromium.org/2692113002/ is probably not needed (and
has downsides) -- I think this CL fixes the root cause by being able to properly
handle min/max/preferred sizes. [haven't had time to patch this CL in on a
Windows box with a bluetooth adapter yet to verify that theory..]
yup - exactly.
sgtm
totally agree :). collected cookies [padlock -> click "XX in use"] has two
button rows, one in the centre of the dialog, so it would be a challenge.

Add bookmark bubble effectively has its "extra view" as the middle button on all
platforms, so it would need a tweak.. but that actually got me thinking about
GetButtonRowViews() again - maybe clients could override that, and/or perhaps we
could add ui::DIALOG_BUTTON_AUXILLARY - which made me think of a way we could
tighten up the logic around that.

[Peter Kasting, 2017/02/28 09:30:46]

Man, we really shouldn't have a button saying "Create a new profile".  "Create",
or maybe "Create profile".  If that's the original English string, can you file
a bug for whatever that dialog is requesting tighter wording?  We have an actual
tech writer now who can solve this sort of thing :)
Can the bottom one be the dialog's "button row" and the middle one be a custom
set of buttons the dialog manages completely on its own?
Yes, I think in general we should not use the "extra view" concept for a third
button -- I'd rather do something like your BUTTON_AUXILIARY idea (and maybe we
should rename _OK and _CANCEL to something like _DEFAULT and... uh...
_SECONDARY?), and let the couple of dialogs that want three buttons use them
that way, and use "extra view" only for the non-button case.  (Which hopefully
turns into the "never" case in Harmony, since Alan said that we should redesign
dialogs that put something in that corner as we get to them.)

That'd make it possible to make this layout code quite a bit simpler.

It's getting beyond the scope of this CL.  I don't care much what order all this
happens in.  Feel free to file bugs, or if you don't get to it I will tomorrow
if I can, since this really seems worth doing.

+    // Only link the extra view column if it is a button.
+    if (ShouldShow(extra_view_) && !CustomButton::AsCustomButton(extra_view_))
+      column_set->LinkColumnSizes(link[1], link[2], -1);
+    else
+      column_set->LinkColumnSizes(link[0], link[1], link[2], -1);
+  }
+  layout->AddPaddingRow(kFixed, insets.bottom());
+}
+
+void DialogClientView::SetupViews() {
+  {
+    base::AutoReset<bool> auto_reset(&in_setup_views_, true);
+    button_row_container_->RemoveAllChildViews(false /* delete children */);
+  }
+
+  UpdateDialogButton(&ok_button_, ui::DIALOG_BUTTON_OK);
+  UpdateDialogButton(&cancel_button_, ui::DIALOG_BUTTON_CANCEL);
+
   if (extra_view_)
     return;
 
   extra_view_ = GetDialogDelegate()->CreateExtraView();
-  if (extra_view_) {
+  if (extra_view_)
     extra_view_->SetGroup(kButtonGroup);
-    AddChildView(extra_view_);
-  }
 }
 
 }  // namespace views