Use GridLayout for DialogClientView's button row.

ui/views/window/dialog_client_view_unittest.cc

Index: ui/views/window/dialog_client_view_unittest.cc
diff --git a/ui/views/window/dialog_client_view_unittest.cc b/ui/views/window/dialog_client_view_unittest.cc
index e4d7eebb2024cce4e9b19a208b4c48fe5633a059..256fdd61d3977a5cdaa677cf7b0f1c34382e5844 100644
--- a/ui/views/window/dialog_client_view_unittest.cc
+++ b/ui/views/window/dialog_client_view_unittest.cc
@@ -43,6 +43,9 @@ class DialogClientViewTest : public test::WidgetTest,
   }
 
   // DialogDelegateView:
+  gfx::Size GetPreferredSize() const override { return preferred_size_; }
+  gfx::Size GetMinimumSize() const override { return min_size_; }
+  gfx::Size GetMaximumSize() const override { return max_size_; }
   ClientView* CreateClientView(Widget* widget) override {
     client_view_ = new DialogClientView(widget, this);
     return client_view_;
@@ -102,7 +105,15 @@ class DialogClientViewTest : public test::WidgetTest,
   void SetExtraViewPadding(int padding) {
     DCHECK(!extra_view_padding_);
     extra_view_padding_.reset(new int(padding));
-    client_view_->Layout();
+    client_view_->UpdateDialogButtons();
+  }
+
+  void SetSizeConstraints(const gfx::Size& min,

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

Nit: Suffix these args with _size

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

Done.

+                          const gfx::Size& preferred,
+                          const gfx::Size& max) {
+    min_size_ = min;
+    preferred_size_ = preferred;
+    max_size_ = max;
   }
 
   View* FocusableViewAfter(View* view) {
@@ -129,6 +140,10 @@ class DialogClientViewTest : public test::WidgetTest,
 
   std::unique_ptr<int> extra_view_padding_;
 
+  gfx::Size preferred_size_;
+  gfx::Size min_size_;
+  gfx::Size max_size_;
+
   DISALLOW_COPY_AND_ASSIGN(DialogClientViewTest);
 };
 
@@ -188,24 +203,27 @@ TEST_F(DialogClientViewTest, SetupFocusChain) {
   const bool kIsOkButtonOnLeftSide = false;
 #endif
 
+  GetContentsView()->SetFocusBehavior(View::FocusBehavior::ALWAYS);
   // Initially the dialog client view only contains the content view.
-  EXPECT_EQ(nullptr, GetContentsView()->GetNextFocusableView());
+  EXPECT_EQ(GetContentsView(), FocusableViewAfter(GetContentsView()));
 
   // Add OK and cancel buttons.
   SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
 
   if (kIsOkButtonOnLeftSide) {
     EXPECT_EQ(client_view()->ok_button(),
-              GetContentsView()->GetNextFocusableView());
+              FocusableViewAfter(GetContentsView()));
     EXPECT_EQ(client_view()->cancel_button(),
-              client_view()->ok_button()->GetNextFocusableView());
-    EXPECT_EQ(nullptr, client_view()->cancel_button()->GetNextFocusableView());
+              FocusableViewAfter(client_view()->ok_button()));
+    EXPECT_EQ(GetContentsView(),
+              FocusableViewAfter(client_view()->cancel_button()));
   } else {
     EXPECT_EQ(client_view()->cancel_button(),
-              GetContentsView()->GetNextFocusableView());
+              FocusableViewAfter(GetContentsView()));
     EXPECT_EQ(client_view()->ok_button(),
-              client_view()->cancel_button()->GetNextFocusableView());
-    EXPECT_EQ(nullptr, client_view()->ok_button()->GetNextFocusableView());
+              FocusableViewAfter(client_view()->cancel_button()));
+    EXPECT_EQ(GetContentsView(),
+              FocusableViewAfter(client_view()->ok_button()));
   }
 
   // Add extra view and remove OK button.
@@ -214,14 +232,15 @@ TEST_F(DialogClientViewTest, SetupFocusChain) {
   SetExtraView(extra_view);
   SetDialogButtons(ui::DIALOG_BUTTON_CANCEL);
 
-  EXPECT_EQ(extra_view, GetContentsView()->GetNextFocusableView());
-  EXPECT_EQ(client_view()->cancel_button(), extra_view->GetNextFocusableView());
-  EXPECT_EQ(nullptr, client_view()->cancel_button()->GetNextFocusableView());
+  EXPECT_EQ(extra_view, FocusableViewAfter(GetContentsView()));
+  EXPECT_EQ(client_view()->cancel_button(), FocusableViewAfter(extra_view));
+  EXPECT_EQ(GetContentsView(), FocusableViewAfter(client_view()));
 
   // Add a dummy view to the contents view. Consult the FocusManager for the
   // traversal order since it now spans different levels of the view hierarchy.
   View* dummy_view = new StaticSizedView(gfx::Size(200, 200));
   dummy_view->SetFocusBehavior(View::FocusBehavior::ALWAYS);
+  GetContentsView()->SetFocusBehavior(View::FocusBehavior::NEVER);
   GetContentsView()->AddChildView(dummy_view);
   EXPECT_EQ(dummy_view, FocusableViewAfter(client_view()->cancel_button()));
   EXPECT_EQ(extra_view, FocusableViewAfter(dummy_view));
@@ -272,7 +291,58 @@ TEST_F(DialogClientViewTest, LayoutWithButtons) {
   extra_view->SetVisible(true);
   client_view()->SetBoundsRect(gfx::Rect(gfx::Point(0, 0), no_extra_view_size));
   client_view()->Layout();
+
+  // Initially nothing happens. The dialog always incorporates the preferred
+  // size of the extra view into the layout.
+  EXPECT_EQ(width_of_extra_view, extra_view->bounds().width());
+
+  // Set a smaller size and try again (rely on ChildPreferredSizeChanged() to
+  // trigger Layout()).
+  extra_view->SetSize(gfx::Size(10, 10));
   EXPECT_GT(width_of_extra_view, extra_view->bounds().width());
 }
 
+// Ensure the minimum, maximum and preferred sizes of the contents view are
+// respected by the client view, and that the client view includes the button
+// row in its minimum and preferred size calculations.
+TEST_F(DialogClientViewTest, MinMaxPreferredSize) {
+  SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
+  const gfx::Size buttons = client_view()->GetPreferredSize();

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

Nit: Use a name like |buttons_size|, since |buttons| alone sounds like a vector.

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

Done.

+  EXPECT_FALSE(buttons.IsEmpty());
+
+  // When the contents view has no preference, just fit the buttons.
+  EXPECT_EQ(buttons, client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(), client_view()->GetMaximumSize());

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

Nit: IsEmpty()?

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

Same as the other ones - I want to check that both width and height are zero
(rather than either). Added a comment.

+
+  // Ensure buttons are between these widths, for the constants below.
+  EXPECT_LT(20, buttons.width());
+  EXPECT_GT(300, buttons.width());
+
+  // With no buttons, client view should match the contents view.
+  SetDialogButtons(ui::DIALOG_BUTTON_NONE);
+  SetSizeConstraints(gfx::Size(10, 15), gfx::Size(20, 25), gfx::Size(300, 350));
+  EXPECT_EQ(gfx::Size(10, 15), client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(20, 25), client_view()->GetPreferredSize());
+  EXPECT_EQ(gfx::Size(300, 350), client_view()->GetMaximumSize());
+
+  // With buttons, size should increase vertically only.
+  SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
+  EXPECT_EQ(gfx::Size(buttons.width(), 15 + buttons.height()),
+            client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(buttons.width(), 25 + buttons.height()),
+            client_view()->GetPreferredSize());
+  EXPECT_EQ(gfx::Size(300, 350 + buttons.height()),
+            client_view()->GetMaximumSize());
+
+  // If the contents view gets bigger, it should take over the width.
+  SetSizeConstraints(gfx::Size(400, 450), gfx::Size(500, 550),
+                     gfx::Size(600, 650));
+  EXPECT_EQ(gfx::Size(400, 450 + buttons.height()),
+            client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(500, 550 + buttons.height()),
+            client_view()->GetPreferredSize());
+  EXPECT_EQ(gfx::Size(600, 650 + buttons.height()),
+            client_view()->GetMaximumSize());
+}
+
 }  // namespace views

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

This is the dubious Layout call:

void RootView::SetContentsView(View* contents_view) {
  DCHECK(contents_view && GetWidget()->native_widget()) <<
      "Can't be called until after the native widget is created!";
  // The ContentsView must be set up _after_ the window is created so that its
  // Widget pointer is valid.
  SetLayoutManager(new FillLayout);
  if (has_children())
    RemoveAllChildViews(true);
  AddChildView(contents_view);

  // Force a layout now, since the attached hierarchy won't be ready for the
  // containing window's bounds. Note that we call Layout directly rather than
  // calling the widget's size changed handler, since the RootView's bounds may
  // not have changed, which will cause the Layout not to be done otherwise.
  Layout();
}





Here's some more cruft around the weird 1x1 contents bounds during init:

gfx::Rect NativeFrameView::GetWindowBoundsForClientBounds(
    const gfx::Rect& client_bounds) const {
#if defined(OS_WIN)
  return views::GetWindowBoundsForClientBounds(
      static_cast<View*>(const_cast<NativeFrameView*>(this)), client_bounds);
#else
  // Enforce minimum size (1, 1) in case that |client_bounds| is passed with
  // empty size.
  gfx::Rect window_bounds = client_bounds;
  if (window_bounds.IsEmpty())
    window_bounds.set_size(gfx::Size(1,1));
  return window_bounds;
#endif
}


[since http://crrev.com/271086 ].

that "window_bounds.set_size(gfx::Size(1,1));" causes views::Label in
dialog_example.cc to obtain a size of 1x1. When that happens, 


GetWindowBoundsForClientBounds is called from

gfx::Size NonClientView::GetPreferredSize() const {
  // TODO(pkasting): This should probably be made to look similar to
  // GetMinimumSize() below.  This will require implementing GetPreferredSize()
  // better in the various frame views.
  gfx::Rect client_bounds(gfx::Point(), client_view_->GetPreferredSize());
  return GetWindowBoundsForClientBounds(client_bounds).size();
}