Use GridLayout for DialogClientView's button row.

ui/views/window/dialog_client_view.h

Index: ui/views/window/dialog_client_view.h
diff --git a/ui/views/window/dialog_client_view.h b/ui/views/window/dialog_client_view.h
index 3602812832829fe28371f772bb36f6e6bf6b8800..f4f83b42923dde3125aebf550ba37b07bd1335df 100644
--- a/ui/views/window/dialog_client_view.h
+++ b/ui/views/window/dialog_client_view.h
@@ -51,6 +51,9 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
 
   // View implementation:
   gfx::Size GetPreferredSize() const override;
+  gfx::Size GetMinimumSize() const override;
+  gfx::Size GetMaximumSize() const override;
+
   void Layout() override;
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
   void ViewHierarchyChanged(
@@ -67,7 +70,7 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   void set_minimum_size(const gfx::Size& size) { minimum_size_ = size; }
 
  private:
-  bool has_dialog_buttons() const { return ok_button_ || cancel_button_; }
+  class ButtonRowContainer;
 
   // Returns the DialogDelegate for the window.
   DialogDelegate* GetDialogDelegate() const;
@@ -76,34 +79,21 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   void ChildPreferredSizeChanged(View* child) override;
   void ChildVisibilityChanged(View* child) override;
 
-  // Create a dialog button of the appropriate type.
-  LabelButton* CreateDialogButton(ui::DialogButton type);
-
-  // Update |button|'s text and enabled state according to the delegate's state.
-  void UpdateButton(LabelButton* button, ui::DialogButton type);
-
-  // Returns the height of the buttons.
-  int GetButtonHeight() const;
-
-  // Returns the height of the extra view.
-  int GetExtraViewHeight() const;
-
-  // Returns the height of the row containing the buttons and the extra view.
-  int GetButtonsAndExtraViewRowHeight() const;
-
-  // Returns the insets for the buttons and extra view, including the vertical
-  // padding between them and the contents view.
-  gfx::Insets GetButtonRowInsets() const;
-
-  // Sets up the focus chain for the child views. This is required since the
-  // delegate may choose to add/remove views at any time.
-  void SetupFocusChain();
+  // Update the dialog button |member| according to the DialogDelegate.

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

Nit: Updates

Updates like how?

Do you need to say what |type| is used for?

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

Done.

+  void UpdateDialogButton(LabelButton** member, ui::DialogButton type);
 
   // Returns the spacing between the extra view and the ok/cancel buttons. 0 if
   // no extra view. Otherwise uses GetExtraViewPadding() or the default padding.
   int GetExtraViewSpacing() const;
 
+  // Returns the 0-3 Views in the order they should appear in the button row.

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

Nit: the 0-3 Views -> any button views

(Seems best not to explicitly say "0-3" here even though in practice that's
true.)

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

Done.

+  std::vector<View*> ButtonRowViews();

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

Nit: GetButtonRowViews()?

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

Done.

+
+  // Installs and configures the LayoutManager for |button_row_container_|.
+  void SetupLayout();
+
   // Creates or deletes any buttons that are required. Updates data members.
+  // All buttons are removed from the hierarchy to be added by the layout.

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

Nit: I don't really understand this sentence.  Not enough context without
explicitly reading the code.

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

Changed to "After calling this, no button row Views will be in the view
hierarchy."

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

Is that accurate, though?  Maybe you mean "...any necessary button row Views
will be children of |button_row_container_| rather than |this|."?  (They're
still in the hierarchy in that case, just indirectly.)

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

Nah, in this case SetupLayout() is relying on SetupViews() to leave everything
out of the view heirarchy, so it can add them all to the Layout (GridLayout
always adds the child view to the layout host as well as adding the view to the
layout, and DCHECKs if you try it any other way).

(note that only SetupLayout() invokes SetupViews() -- it could all just be
inlined there, but functional decomposition is nice too :).

   void SetupViews();
 
   // How much to inset the button row.
@@ -120,12 +110,18 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   // The extra view shown in the row of buttons; may be NULL.
   View* extra_view_ = nullptr;
 
+  // Container view for the button row.
+  ButtonRowContainer* button_row_container_ = nullptr;
+
   // True if we've notified the delegate the window is closing and the delegate
   // allowed the close. In some situations it's possible to get two closes (see
   // http://crbug.com/71940). This is used to avoid notifying the delegate
   // twice, which can have bad consequences.
   bool delegate_allowed_close_ = false;
 
+  // Prevents ViewHierarchyChanged() from clearing out data members.

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

Nit: "When true, ..."

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

Done.

+  bool in_setup_views_ = false;

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

Nit: Seems like this should be named by what we want it do (e.g.
|preserve_data_members_|) instead of where it's set.

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

Done.

+
   DISALLOW_COPY_AND_ASSIGN(DialogClientView);
 };