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

ui/views/window/dialog_delegate_unittest.cc

Index: ui/views/window/dialog_delegate_unittest.cc
diff --git a/ui/views/window/dialog_delegate_unittest.cc b/ui/views/window/dialog_delegate_unittest.cc
index d11cfddde8f6aabf0595da8212e837a7e92e24b4..56b56ef741959c8e60d1c38b3085b6785696bfbf 100644
--- a/ui/views/window/dialog_delegate_unittest.cc
+++ b/ui/views/window/dialog_delegate_unittest.cc
@@ -28,15 +28,7 @@ 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() {
@@ -52,6 +44,8 @@ class TestDialog : public DialogDelegateView {
     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;
@@ -66,7 +60,6 @@ class TestDialog : public DialogDelegateView {
     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_;
@@ -92,6 +85,9 @@ class TestDialog : public DialogDelegateView {
   }
 
   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;
   }
@@ -100,13 +96,14 @@ class TestDialog : public DialogDelegateView {
 
  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);
 };
@@ -205,8 +202,38 @@ TEST_F(DialogTest, HitTest_HiddenTitle) {
   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 },
@@ -222,6 +249,8 @@ TEST_F(DialogTest, HitTest_HiddenTitle) {
         << " case " << i << " with border: " << border << ", at point "
         << cases[i].point;
   }
+
+  dialog->TearDown();
 }
 
 TEST_F(DialogTest, HitTest_WithTitle) {