Use GridLayout for DialogClientView's button row.

ui/views/window/dialog_client_view_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/window/dialog_client_view.h"
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "ui/base/ui_base_types.h"
@@ skipping 25 matching lines @@
     widget_->Init(params);
     EXPECT_EQ(this, GetContentsView());
   }
 
   void TearDown() override {
     widget_->CloseNow();
     WidgetTest::TearDown();
   }
 
   // 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_;
   }
 
   bool ShouldUseCustomFrame() const override { return false; }
 
   void DeleteDelegate() override {
     // DialogDelegateView would delete this, but |this| is owned by the test.
   }
@@ skipping 39 matching lines @@
     EXPECT_FALSE(next_extra_view_);
     next_extra_view_ = base::WrapUnique(view);
     client_view_->UpdateDialogButtons();
     EXPECT_FALSE(next_extra_view_);
   }
 
   // Sets the extra view padding.
   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) {
     const bool dont_loop = false;
     const bool reverse = false;
     return GetFocusManager()->GetNextFocusableView(view, GetWidget(), reverse,
                                                    dont_loop);
   }
 
   DialogClientView* client_view() { return client_view_; }
 
  private:
   // The dialog Widget.
   Widget* widget_ = nullptr;
 
   // The DialogClientView that's being tested. Owned by |widget_|.
   DialogClientView* client_view_;
 
   // The bitmask of buttons to show in the dialog.
   int dialog_buttons_ = ui::DIALOG_BUTTON_NONE;
 
   // Set and cleared in SetExtraView().
   std::unique_ptr<View> next_extra_view_;
 
   std::unique_ptr<int> extra_view_padding_;
 
+  gfx::Size preferred_size_;
+  gfx::Size min_size_;
+  gfx::Size max_size_;
+
   DISALLOW_COPY_AND_ASSIGN(DialogClientViewTest);
 };
 
 TEST_F(DialogClientViewTest, UpdateButtons) {
   // This dialog should start with no buttons.
   EXPECT_EQ(GetDialogButtons(), ui::DIALOG_BUTTON_NONE);
   EXPECT_EQ(NULL, client_view()->ok_button());
   EXPECT_EQ(NULL, client_view()->cancel_button());
   const int height_without_buttons = GetUpdatedClientBounds().height();
 
@@ skipping 39 matching lines @@
 }
 
 // Test that views inside the dialog client view have the correct focus order.
 TEST_F(DialogClientViewTest, SetupFocusChain) {
 #if defined(OS_WIN) || defined(OS_CHROMEOS)
   const bool kIsOkButtonOnLeftSide = true;
 #else
   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.
   View* extra_view = new StaticSizedView(gfx::Size(200, 200));
   extra_view->SetFocusBehavior(View::FocusBehavior::ALWAYS);
   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));
   EXPECT_EQ(client_view()->cancel_button(), FocusableViewAfter(extra_view));
 
   // Views are added to the contents view, not the client view, so the focus
   // chain within the client view is not affected.
   EXPECT_EQ(nullptr, client_view()->cancel_button()->GetNextFocusableView());
 }
 
@@ skipping 30 matching lines @@
   // Visibility of extra view is respected.
   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());
 
   // Try with a reduced-size dialog.
   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();
}