Fix layout of BubbleFrameView when there's only a close button in the

ui/views/window/dialog_delegate_unittest.cc

 // Copyright 2013 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 <stddef.h>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/base/hit_test.h"
 #include "ui/events/event_processor.h"
@@ skipping 10 matching lines @@
 #if defined(OS_MACOSX)
 #include "ui/base/test/scoped_fake_full_keyboard_access.h"
 #endif
 
 namespace views {
 
 namespace {
 
 class TestDialog : public DialogDelegateView {
  public:
-  TestDialog()
-      : input_(new views::Textfield()),
-        canceled_(false),
-        accepted_(false),
-        closed_(false),
-        closeable_(false),
-        should_handle_escape_(false) {
-    AddChildView(input_);
-  }
+  TestDialog() : input_(new views::Textfield()) { AddChildView(input_); }
   ~TestDialog() override {}
 
   void Init() {
     // Add the accelerator before being added to the widget hierarchy (before
     // DCV has registered its accelerator) to make sure accelerator handling is
     // not dependent on the order of AddAccelerator calls.
     EXPECT_FALSE(GetWidget());
     AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
   }
 
   // WidgetDelegate overrides:
   bool ShouldShowWindowTitle() const override {
     return !title_.empty();
   }
 
+  bool ShouldShowCloseButton() const override { return show_close_button_; }

[Peter Kasting, 2017/04/13 22:36:29]

Nit: Maybe no blank line above this

[Evan Stade, 2017/04/13 23:39:17]

Done.

[Peter Kasting, 2017/04/13 23:47:34]

You didn't, but it's not important :)

Same for the other nit below.

[Evan Stade, 2017/04/13 23:51:24]

oi, I seem to be in too much of a hurry to do anything right today. Uploaded
correct patch.

+
   // DialogDelegateView overrides:
   bool Cancel() override {
     canceled_ = true;
     return closeable_;
   }
   bool Accept() override {
     accepted_ = true;
     return closeable_;
   }
   bool Close() override {
     closed_ = true;
     return closeable_;
   }
 
-  // DialogDelegateView overrides:
   gfx::Size GetPreferredSize() const override { return gfx::Size(200, 200); }
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override {
     return should_handle_escape_;
   }
   base::string16 GetWindowTitle() const override { return title_; }
   View* GetInitiallyFocusedView() override { return input_; }
   bool ShouldUseCustomFrame() const override { return true; }
 
   void CheckAndResetStates(bool canceled,
                            bool accepted,
                            bool closed) {
     EXPECT_EQ(canceled, canceled_);
     canceled_ = false;
     EXPECT_EQ(accepted, accepted_);
     accepted_ = false;
     EXPECT_EQ(closed, closed_);
     closed_ = false;
   }
 
   void TearDown() {
     closeable_ = true;
     GetWidget()->Close();
   }
 
   void set_title(const base::string16& title) { title_ = title; }
+  void set_show_close_button(bool show_close) {
+    show_close_button_ = show_close;
+  }
   void set_should_handle_escape(bool should_handle_escape) {
     should_handle_escape_ = should_handle_escape;
   }
 
   views::Textfield* input() { return input_; }
 
  private:
   views::Textfield* input_;
-  bool canceled_;
-  bool accepted_;
-  bool closed_;
+  bool canceled_ = false;
+  bool accepted_ = false;
+  bool closed_ = false;
   // Prevent the dialog from closing, for repeated ok and cancel button clicks.
   bool closeable_;
   base::string16 title_;
-  bool should_handle_escape_;
+  bool show_close_button_ = true;
+  bool should_handle_escape_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(TestDialog);
 };
 
 class DialogTest : public ViewsTestBase {
  public:
   DialogTest() : dialog_(nullptr) {}
   ~DialogTest() override {}
 
   void SetUp() override {
@@ skipping 78 matching lines @@
   delete dialog()->GetDialogClientView()->cancel_button();
 }
 
 TEST_F(DialogTest, HitTest_HiddenTitle) {
   // Ensure that BubbleFrameView hit-tests as expected when the title is hidden.
   const NonClientView* view = dialog()->GetWidget()->non_client_view();
   BubbleFrameView* frame = static_cast<BubbleFrameView*>(view->frame_view());
   const int border = frame->bubble_border()->GetBorderThickness();
 
   struct {
-    const int point;
-    const int hit;
+    int point;

[Peter Kasting, 2017/04/13 22:36:29]

Nit: I suppose these could be const like the old ones were, but I don't care
much (2 places)

[Evan Stade, 2017/04/13 23:39:17]

Done.

+    int hit;
+  } cases[] = {
+      {border, HTSYSMENU},
+      {border + 10, HTSYSMENU},
+      {border + 20, HTNOWHERE},
+      {border + 50, HTCLIENT /* Space is reserved for the close button. */},
+      {border + 60, HTCLIENT},
+      {1000, HTNOWHERE},
+  };
+
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    gfx::Point point(cases[i].point, cases[i].point);
+    EXPECT_EQ(cases[i].hit, frame->NonClientHitTest(point))
+        << " case " << i << " with border: " << border << ", at point "
+        << cases[i].point;
+  }
+}
+
+TEST_F(DialogTest, HitTest_HiddenTitleNoCloseButton) {
+  auto* dialog = new TestDialog();
+  dialog->set_show_close_button(false);

[Peter Kasting, 2017/04/13 22:36:28]

I don't suppose there's an easy way to toggle this off after showing, so we
don't need to recreate DialogTest::SetUp()/TearDown() in this test?

If not, I suggest making DialogTest have a one-arg constructor that causes it to
set_show_close_button(...) on the dialog it creates, then creating a DialogTest
subclass whose null constructor calls that constructor.  Then this test case can
use that test class instead of this one.

This is more lines of code on net, but it's easier to read the test itself, and
it's more lines of boilerplate instead of lines of actual copied code that's
sort of magic (e.g. I just spent a few minutes looking at the memory management
of TestDialog and determining that what's happening here is correct, but kinda
scary).

[Evan Stade, 2017/04/13 23:39:18]

I tried a few ways of doing that but to no avail. I decided to leave that can of
worms alone.
we already do this (create a dialog inside a test) for BoundsAccommodateTitle. I
looked at following your suggestion and my biggest gripe is we get a different
test class name (DialogTest vs DialogTestWithNoCloseButton, which doesn't seem
attractive or scalable to more parameters). So I exposed a way to recreate the
dialog and tweak it as appropriate.

[Peter Kasting, 2017/04/13 23:41:29]

I went to look, but I don't see the patch doing this uploaded?

The idea you describe sounds reasonable.

+  dialog->Init();
+  DialogDelegate::CreateDialogWidget(dialog, GetContext(), nullptr)->Show();
+
+  const NonClientView* view = dialog->GetWidget()->non_client_view();
+  BubbleFrameView* frame = static_cast<BubbleFrameView*>(view->frame_view());
+  const int border = frame->bubble_border()->GetBorderThickness();
+
+  struct {
+    int point;
+    int hit;
   } cases[] = {
     { border,      HTSYSMENU },
     { border + 10, HTSYSMENU },
     { border + 20, HTCLIENT  },
     { border + 50, HTCLIENT  },
     { border + 60, HTCLIENT  },
     { 1000,        HTNOWHERE },
   };
 
   for (size_t i = 0; i < arraysize(cases); ++i) {
     gfx::Point point(cases[i].point, cases[i].point);
     EXPECT_EQ(cases[i].hit, frame->NonClientHitTest(point))
         << " case " << i << " with border: " << border << ", at point "
         << cases[i].point;
   }
+
+  dialog->TearDown();
 }
 
 TEST_F(DialogTest, HitTest_WithTitle) {
   // Ensure that BubbleFrameView hit-tests as expected when the title is shown.
   const NonClientView* view = dialog()->GetWidget()->non_client_view();
   dialog()->set_title(base::ASCIIToUTF16("Title"));
   dialog()->GetWidget()->UpdateWindowTitle();
   BubbleFrameView* frame = static_cast<BubbleFrameView*>(view->frame_view());
   const int border = frame->bubble_border()->GetBorderThickness();
 
@@ skipping 108 matching lines @@
   // On showing the dialog, the initially focused View will be the OK button.
   // Since it is no longer focusable, focus should advance to the next focusable
   // View, which is |textfield|.
   dialog_widget->Show();
   EXPECT_TRUE(textfield->HasFocus());
   EXPECT_EQ(textfield, dialog->GetFocusManager()->GetFocusedView());
   dialog_widget->CloseNow();
 }
 
 }  // namespace views