Cleanups for DialogClientView, DialogClientViewTest.

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"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/style/platform_style.h"
 #include "ui/views/test/test_views.h"
-#include "ui/views/test/views_test_base.h"
+#include "ui/views/test/widget_test.h"
 #include "ui/views/widget/widget.h"
-#include "ui/views/window/dialog_client_view.h"
 #include "ui/views/window/dialog_delegate.h"
 
 namespace views {
 
-class TestDialogClientView : public DialogClientView {
+// Base class for tests. Also acts as the dialog delegate and contents view for
+// TestDialogClientView.
+class DialogClientViewTest : public test::WidgetTest,
+                             public DialogDelegateView {
  public:
-  explicit TestDialogClientView(DialogDelegateView* dialog_delegate_view)
-      : DialogClientView(dialog_delegate_view),
-        dialog_delegate_view_(dialog_delegate_view) {}
-  ~TestDialogClientView() override {}
+  DialogClientViewTest() {}
 
-  // DialogClientView implementation.
-  DialogDelegate* GetDialogDelegate() const override {
-    return dialog_delegate_view_;
+  // testing::Test:
+  void SetUp() override {
+    WidgetTest::SetUp();
+
+    // Note: not using DialogDelegate::CreateDialogWidget(..), since that can
+    // alter the frame type according to the platform.
+    widget_ = new views::Widget;
+    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+    params.delegate = this;
+    widget_->Init(params);
+    EXPECT_EQ(this, GetContentsView());
   }
 
-  View* GetContentsView() { return contents_view(); }
-
-  void CreateExtraViews() {
-    CreateExtraView();
+  void TearDown() override {
+    widget_->CloseNow();
+    WidgetTest::TearDown();
   }
 
- private:
-  DialogDelegateView* dialog_delegate_view_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestDialogClientView);
-};
-
-// Base class for tests. Also acts as the dialog delegate and contents view for
-// TestDialogClientView.
-class DialogClientViewTest : public ViewsTestBase,
-                             public DialogDelegateView {
- public:
-  DialogClientViewTest()
-      : dialog_buttons_(ui::DIALOG_BUTTON_NONE),
-        extra_view_(nullptr) {}
-  ~DialogClientViewTest() override {}
-
-  // testing::Test implementation.
-  void SetUp() override {
-    dialog_buttons_ = ui::DIALOG_BUTTON_NONE;
-    client_view_.reset(new TestDialogClientView(this));
-    // Add this (i.e. the contents view) as a child of |client_view_|. This is
-    // generally done when the client view is added to the view hierarchy.
-    client_view_->AddChildViewAt(this, 0);
-    ViewsTestBase::SetUp();
+  // DialogDelegateView:
+  ClientView* CreateClientView(Widget* widget) override {
+    client_view_ = new DialogClientView(widget, this);
+    return client_view_;
   }
 
-  // DialogDelegateView implementation.
-  View* CreateExtraView() override { return extra_view_; }
+  bool ShouldUseCustomFrame() const override { return false; }
+
+  void DeleteDelegate() override {
+    // DialogDelegateView would delete this, but |this| is owned by the test.
+  }
+
+  View* CreateExtraView() override { return next_extra_view_.release(); }
+
   bool GetExtraViewPadding(int* padding) override {
     if (extra_view_padding_)
       *padding = *extra_view_padding_;
     return extra_view_padding_.get() != nullptr;
   }
+
   int GetDialogButtons() const override { return dialog_buttons_; }
 
  protected:
   gfx::Rect GetUpdatedClientBounds() {
     client_view_->SizeToPreferredSize();
     client_view_->Layout();
     return client_view_->bounds();
   }
 
   // Makes sure that the content view is sized correctly. Width must be at least
   // the requested amount, but height should always match exactly.
   void CheckContentsIsSetToPreferredSize() {
     const gfx::Rect client_bounds = GetUpdatedClientBounds();
     const gfx::Size preferred_size = this->GetPreferredSize();
     EXPECT_EQ(preferred_size.height(), this->bounds().height());
     EXPECT_LE(preferred_size.width(), this->bounds().width());
-    EXPECT_EQ(this->origin(), client_bounds.origin());
-    EXPECT_EQ(this->bounds().right(), client_bounds.right());
+    EXPECT_EQ(gfx::Point(), this->origin());
+    EXPECT_EQ(client_bounds.width(), this->width());

[tapted, 2017/02/22 11:01:17]

Using a Widget means it gets a NonClientFrameView, which encounters some
platform-specific code. When a non-native frame gets used, the first two checks
above pass, but other two fail because the client view is not at the origin.

For the origin, it was always just checking that (0,0) == (0,0) which is not
very interesting. Made that explicit, and made it check the width instead of
.right().

   }
 
   // Sets the buttons to show in the dialog and refreshes the dialog.
   void SetDialogButtons(int dialog_buttons) {
     dialog_buttons_ = dialog_buttons;
     client_view_->UpdateDialogButtons();
   }
 
-  // Sets the extra view.
+  // Sets the view to provide to CreateExtraView() and updates the dialog. This
+  // can only be called a single time because DialogClientView caches the result
+  // of CreateExtraView() and never calls it again.
   void SetExtraView(View* view) {
-    DCHECK(!extra_view_);
-    extra_view_ = view;
-    client_view_->CreateExtraViews();
+    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();
   }
 
-  TestDialogClientView* client_view() { return client_view_.get(); }
+  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 DialogClientView that's being tested.
-  std::unique_ptr<TestDialogClientView> client_view_;
+  // 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_;
-  View* extra_view_;  // weak
-  std::unique_ptr<int> extra_view_padding_;  // Null by default.
+  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_;
 
   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 41 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
 
   // Initially the dialog client view only contains the content view.
-  EXPECT_EQ(nullptr, client_view()->GetContentsView()->GetNextFocusableView());
+  EXPECT_EQ(nullptr, GetContentsView()->GetNextFocusableView());
 
   // Add OK and cancel buttons.
   SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
 
   if (kIsOkButtonOnLeftSide) {
     EXPECT_EQ(client_view()->ok_button(),
-              client_view()->GetContentsView()->GetNextFocusableView());
+              GetContentsView()->GetNextFocusableView());
     EXPECT_EQ(client_view()->cancel_button(),
               client_view()->ok_button()->GetNextFocusableView());
     EXPECT_EQ(nullptr, client_view()->cancel_button()->GetNextFocusableView());
   } else {
     EXPECT_EQ(client_view()->cancel_button(),
-              client_view()->GetContentsView()->GetNextFocusableView());
+              GetContentsView()->GetNextFocusableView());
     EXPECT_EQ(client_view()->ok_button(),
               client_view()->cancel_button()->GetNextFocusableView());
     EXPECT_EQ(nullptr, client_view()->ok_button()->GetNextFocusableView());
   }
 
   // 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,
-            client_view()->GetContentsView()->GetNextFocusableView());
+  EXPECT_EQ(extra_view, GetContentsView()->GetNextFocusableView());
   EXPECT_EQ(client_view()->cancel_button(), extra_view->GetNextFocusableView());
   EXPECT_EQ(nullptr, client_view()->cancel_button()->GetNextFocusableView());
 
-  // Add a dummy view to the 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));
-  client_view()->AddChildView(dummy_view);
-  EXPECT_EQ(dummy_view, client_view()->cancel_button()->GetNextFocusableView());
+  dummy_view->SetFocusBehavior(View::FocusBehavior::ALWAYS);
+  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());
 }
 
 // Test that the contents view gets its preferred size in the basic dialog
 // configuration.
 TEST_F(DialogClientViewTest, ContentsSize) {
   CheckContentsIsSetToPreferredSize();
-  EXPECT_EQ(GetContentsView()->bounds().bottom(),
-            client_view()->bounds().bottom());
+  EXPECT_EQ(GetContentsView()->size(), client_view()->size());
+  // There's nothing in the contents view (i.e. |this|), so it should be 0x0.
+  EXPECT_EQ(gfx::Size(), client_view()->size());

[tapted, 2017/02/22 11:01:17]

This fails for a similar reason (but also it's never been very interesting since
the preferred size is 0x0 since there are no buttons). So now it checks that
instead.

 }
 
 // Test the effect of the button strip on layout.
 TEST_F(DialogClientViewTest, LayoutWithButtons) {
   SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL);
   CheckContentsIsSetToPreferredSize();
 
   EXPECT_LT(GetContentsView()->bounds().bottom(),
             client_view()->bounds().bottom());
   gfx::Size no_extra_view_size = client_view()->bounds().size();
@@ skipping 17 matching lines @@
   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();
   EXPECT_GT(width_of_extra_view, extra_view->bounds().width());
 }
 
 }  // namespace views