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_; }
+
   // 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 {
     ViewsTestBase::SetUp();
-    dialog_ = new TestDialog();
-    dialog_->Init();
-    DialogDelegate::CreateDialogWidget(dialog_, GetContext(), nullptr)->Show();
+    InitializeDialog();
+    ShowDialog();
   }
 
   void TearDown() override {
     dialog_->TearDown();
     ViewsTestBase::TearDown();
   }
 
+  void InitializeDialog() {
+    if (dialog_)
+      dialog_->TearDown();
+
+    dialog_ = new TestDialog();
+    dialog_->Init();
+  }
+
+  void ShowDialog() {
+    DialogDelegate::CreateDialogWidget(dialog_, GetContext(), nullptr)->Show();
+  }
+
   void SimulateKeyEvent(const ui::KeyEvent& event) {
     ui::KeyEvent event_copy = event;
     if (dialog()->GetFocusManager()->OnKeyEvent(event_copy))
       dialog()->GetWidget()->OnKeyEvent(&event_copy);
   }
 
   TestDialog* dialog() const { return dialog_; }
 
  private:
   TestDialog* dialog_;
@@ skipping 57 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;
+    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) {
+  InitializeDialog();
+  dialog()->set_show_close_button(false);
+  ShowDialog();

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

Cool, this route is more readable than the old code.  Can we rewrite
BoundsAccommodateTitle to make use of this too?  I'm thinking:

* Capture the relevant metrics (with and without title) from the dialog, check
that without title is shorter than with
* Then use init/show to make a new dialog and set its title before showing, and
check that it's the same as the previous with-title dialog

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

honestly, would rather not let this change snowball any further.

[Peter Kasting, 2017/04/13 23:53:35]

OK.  I just figured you introduced this in part because of the needs of that
function, so it made sense to make use of it there.  If it would add too much
hassle, that's cool too.

+
+  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) {
@@ skipping 123 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