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..50b804e473ae925187b5184bb57ff61f46d3a494 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_size,
+                          const gfx::Size& preferred_size,
+                          const gfx::Size& max_size) {
+    min_size_ = min_size;
+    preferred_size_ = preferred_size;
+    max_size_ = max_size;
   }
 
   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));
@@ -248,31 +267,80 @@ TEST_F(DialogClientViewTest, LayoutWithButtons) {
 
   EXPECT_LT(GetContentsView()->bounds().bottom(),
             client_view()->bounds().bottom());
-  gfx::Size no_extra_view_size = client_view()->bounds().size();
+  const gfx::Size no_extra_view_size = client_view()->bounds().size();
 
   View* extra_view = new StaticSizedView(gfx::Size(200, 200));
   SetExtraView(extra_view);
   CheckContentsIsSetToPreferredSize();
   EXPECT_GT(client_view()->bounds().height(), no_extra_view_size.height());
-  int width_of_dialog = client_view()->bounds().width();
-  int width_of_extra_view = extra_view->bounds().width();
+  const int width_of_dialog_small_padding = client_view()->width();
 
   // Try with an adjusted padding for the extra view.
   SetExtraViewPadding(250);
   CheckContentsIsSetToPreferredSize();
-  EXPECT_GT(client_view()->bounds().width(), width_of_dialog);
+  EXPECT_GT(client_view()->bounds().width(), width_of_dialog_small_padding);
+
+  const gfx::Size with_extra_view_size = client_view()->size();
+  EXPECT_NE(no_extra_view_size, with_extra_view_size);
 
-  // Visibility of extra view is respected.
+  // Hiding the extra view removes it as well as the extra padding.
   extra_view->SetVisible(false);
   CheckContentsIsSetToPreferredSize();
-  EXPECT_EQ(no_extra_view_size.height(), client_view()->bounds().height());
-  EXPECT_EQ(no_extra_view_size.width(), client_view()->bounds().width());
+  EXPECT_EQ(no_extra_view_size, client_view()->size());
 
-  // Try with a reduced-size dialog.
+  // Making it visible again adds it all back.
   extra_view->SetVisible(true);
-  client_view()->SetBoundsRect(gfx::Rect(gfx::Point(0, 0), no_extra_view_size));
-  client_view()->Layout();
-  EXPECT_GT(width_of_extra_view, extra_view->bounds().width());
+  CheckContentsIsSetToPreferredSize();
+  EXPECT_EQ(with_extra_view_size, client_view()->size());
+
+  // Leave |extra_view| hidden. It should still have a parent, to ensure it is
+  // owned by a View hierarchy and gets deleted.
+  extra_view->SetVisible(false);
+  EXPECT_TRUE(extra_view->parent());
+}
+
+// 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_size = client_view()->GetPreferredSize();
+  EXPECT_FALSE(buttons_size.IsEmpty());
+
+  // When the contents view has no preference, just fit the buttons. The
+  // maximum size should be unconstrained in both directions.
+  EXPECT_EQ(buttons_size, client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(), client_view()->GetMaximumSize());
+
+  // Ensure buttons are between these widths, for the constants below.
+  EXPECT_LT(20, buttons_size.width());
+  EXPECT_GT(300, buttons_size.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_size.width(), 15 + buttons_size.height()),
+            client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(buttons_size.width(), 25 + buttons_size.height()),
+            client_view()->GetPreferredSize());
+  EXPECT_EQ(gfx::Size(300, 350 + buttons_size.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_size.height()),
+            client_view()->GetMinimumSize());
+  EXPECT_EQ(gfx::Size(500, 550 + buttons_size.height()),
+            client_view()->GetPreferredSize());
+  EXPECT_EQ(gfx::Size(600, 650 + buttons_size.height()),
+            client_view()->GetMaximumSize());
 }
 
 }  // namespace views

[Bret, 2017/03/01 00:29:50]

no new test for linking the button sizes?

[tapted, 2017/03/01 10:44:58]

Done.