views: de-virtualize DialogDelegate::Close

This change makes DialogDelegate::Close non-virtual. It also splits
DialogTest.AcceptAndCancel into several smaller tests, to avoid it
needing to return false from Accept/Cancel/Close; this also makes the
tests easier to follow.

A followup CL will delete DialogDelegate::Close completely, replacing
it with a new method DialogDelegate::CloseDialog(), for unit tests that
want to simulate closing a dialog but have not created a Widget for it.

Bug: 1011446
Change-Id: I2cb64c3c757e7685d32c1d3b99406999a2cf626d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068299
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744044}

ui/views/widget/native_widget_mac_unittest.mm

@@ -1108,7 +1108,6 @@ class ModalDialogDelegate : public DialogDelegateView {
   explicit ModalDialogDelegate(ui::ModalType modal_type)
       : modal_type_(modal_type) {}
 
-  void set_can_close(bool value) { can_close_ = value; }
   void SetButtons(int buttons) {
     DialogDelegate::set_buttons(buttons);
     DialogModelChanged();
@@ -1116,11 +1115,9 @@ class ModalDialogDelegate : public DialogDelegateView {
 
   // DialogDelegateView:
   ui::ModalType GetModalType() const override { return modal_type_; }
-  bool Close() override { return can_close_; }
 
  private:
   const ui::ModalType modal_type_;
-  bool can_close_ = true;
 
   DISALLOW_COPY_AND_ASSIGN(ModalDialogDelegate);
 };
@@ -2361,19 +2358,6 @@ TEST_F(NativeWidgetMacViewsOrderTest, UnassociatedViewsIsAbove) {
       ]]));
 }
 
-// Test -[NSWindowDelegate windowShouldClose:].
-TEST_F(NativeWidgetMacTest, CanClose) {
-  ModalDialogDelegate* delegate = new ModalDialogDelegate(ui::MODAL_TYPE_NONE);
-  Widget* widget =
-      views::DialogDelegate::CreateDialogWidget(delegate, nullptr, nullptr);
-  NSWindow* window = widget->GetNativeWindow().GetNativeNSWindow();
-  delegate->set_can_close(false);
-  EXPECT_FALSE([[window delegate] windowShouldClose:window]);
-  delegate->set_can_close(true);
-  EXPECT_TRUE([[window delegate] windowShouldClose:window]);
-  widget->CloseNow();
-}
-
 namespace {
 
 // Returns an array of NSTouchBarItemIdentifier (i.e. NSString), extracted from

ui/views/window/dialog_delegate.h

@@ -125,17 +125,6 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   // DEPRECATED: use |set_accept_callback| instead.
   virtual bool Accept();
 
-  // Called when the user closes the window without selecting an option, e.g. by
-  // pressing the close button on the window, pressing the Esc key, or using a
-  // window manager gesture. By default, this calls Accept() if the only button
-  // in the dialog is Accept, Cancel() otherwise. This function should return
-  // true if the window can be closed after it returns, or false if it must
-  // remain open.
-  // DEPRECATED: use |set_close_callback| instead.
-  // NOTE: If **any** of the {accept,cancel,close} callbacks is set,
-  // DefaultClose() is not called and closing is unconditional.
-  virtual bool Close();
-
   // Overridden from WidgetDelegate:
   View* GetInitiallyFocusedView() override;
   DialogDelegate* AsDialogDelegate() override;
@@ -245,6 +234,10 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   void CancelDialog();
   void AcceptDialog();
 
+  // Deprecated, for compatibility with a few remaining unit tests.
+  // TODO(https://crbug.com/1011446): Delete this method.
+  bool Close();
+
   // Reset the dialog's shown timestamp, for tests that are subject to the
   // "unintended interaction" detection mechanism.
   void ResetViewShownTimeStampForTesting();

ui/views/window/dialog_delegate_unittest.cc

@@ -15,6 +15,7 @@
 #include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/style/platform_style.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
@@ -48,19 +49,6 @@ class TestDialog : public DialogDelegateView {
   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_;
-  }
-
   gfx::Size CalculatePreferredSize() const override {
     return gfx::Size(200, 200);
   }
@@ -70,17 +58,7 @@ class TestDialog : public DialogDelegateView {
   base::string16 GetWindowTitle() const override { return title_; }
   View* GetInitiallyFocusedView() override { return input_; }
 
-  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();
   }
 
@@ -96,11 +74,6 @@ class TestDialog : public DialogDelegateView {
 
  private:
   views::Textfield* input_;
-  bool canceled_ = false;
-  bool accepted_ = false;
-  bool closed_ = false;
-  // Prevent the dialog from closing, for repeated ok and cancel button clicks.
-  bool closeable_ = false;
   base::string16 title_;
   bool show_close_button_ = true;
   bool should_handle_escape_ = false;
@@ -142,6 +115,13 @@ class DialogTest : public ViewsTestBase {
 
     dialog_ = new TestDialog();
     dialog_->Init();
+
+    dialog_->set_accept_callback(
+        base::BindLambdaForTesting([&]() { accepted_ = true; }));
+    dialog_->set_cancel_callback(
+        base::BindLambdaForTesting([&]() { cancelled_ = true; }));
+    dialog_->set_close_callback(
+        base::BindLambdaForTesting([&]() { closed_ = true; }));
   }
 
   views::Widget* CreateDialogWidget(DialogDelegate* dialog) {
@@ -152,15 +132,20 @@ class DialogTest : public ViewsTestBase {
 
   void ShowDialog() { CreateDialogWidget(dialog_)->Show(); }
 
-  void SimulateKeyEvent(const ui::KeyEvent& event) {
-    ui::KeyEvent event_copy = event;
-    if (dialog()->GetFocusManager()->OnKeyEvent(event_copy))
-      dialog()->GetWidget()->OnKeyEvent(&event_copy);
+  void SimulateKeyPress(ui::KeyboardCode key) {
+    ui::KeyEvent event(ui::ET_KEY_PRESSED, key, ui::EF_NONE);
+    if (dialog()->GetFocusManager()->OnKeyEvent(event))
+      dialog()->GetWidget()->OnKeyEvent(&event);
   }
 
   TestDialog* dialog() const { return dialog_; }
   views::Widget* parent_widget() { return &parent_widget_; }
 
+ protected:
+  bool accepted_ = false;
+  bool cancelled_ = false;
+  bool closed_ = false;
+
  private:
   views::Widget parent_widget_;
   TestDialog* dialog_ = nullptr;
@@ -170,51 +155,69 @@ class DialogTest : public ViewsTestBase {
 
 }  // namespace
 
-TEST_F(DialogTest, AcceptAndCancel) {
-  LabelButton* ok_button = dialog()->GetOkButton();
-  LabelButton* cancel_button = dialog()->GetCancelButton();
-
-  // Check that return/escape accelerators accept/close dialogs.
+TEST_F(DialogTest, InputIsInitiallyFocused) {
   EXPECT_EQ(dialog()->input(), dialog()->GetFocusManager()->GetFocusedView());
+}
+
+TEST_F(DialogTest, OkButtonAccepts) {
+  EXPECT_FALSE(accepted_);
+  SimulateKeyPress(ui::VKEY_RETURN);
+  EXPECT_TRUE(accepted_);
+}
+
+TEST_F(DialogTest, EscButtonCloses) {
+  EXPECT_FALSE(closed_);
+  SimulateKeyPress(ui::VKEY_ESCAPE);
+  EXPECT_TRUE(closed_);
+}
+
+TEST_F(DialogTest, ReturnDirectedToOkButtonPlatformStyle) {
   const ui::KeyEvent return_event(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
                                   ui::EF_NONE);
-  SimulateKeyEvent(return_event);
-  dialog()->CheckAndResetStates(false, true, false);
-  const ui::KeyEvent escape_event(ui::ET_KEY_PRESSED, ui::VKEY_ESCAPE,
-                                  ui::EF_NONE);
-  SimulateKeyEvent(escape_event);
-  dialog()->CheckAndResetStates(false, false, true);
+  if (PlatformStyle::kReturnClicksFocusedControl) {
+    EXPECT_TRUE(dialog()->GetOkButton()->OnKeyPressed(return_event));
+    EXPECT_TRUE(accepted_);
+  } else {
+    EXPECT_FALSE(dialog()->GetOkButton()->OnKeyPressed(return_event));
+    EXPECT_FALSE(accepted_);
+    // If the return key press was not directed *specifically* to the Ok button,
+    // it would bubble upwards here, reach the dialog, and accept it. The
+    // OkButtonAccepts test above covers that behavior.
+  }
+}
 
-// Check ok and cancel button behavior on a directed return key event. Buttons
-// won't respond to a return key event on Mac, since it performs the default
-// action.
-#if defined(OS_MACOSX)
-  EXPECT_FALSE(ok_button->OnKeyPressed(return_event));
-  dialog()->CheckAndResetStates(false, false, false);
-  EXPECT_FALSE(cancel_button->OnKeyPressed(return_event));
-  dialog()->CheckAndResetStates(false, false, false);
-#else
-  EXPECT_TRUE(ok_button->OnKeyPressed(return_event));
-  dialog()->CheckAndResetStates(false, true, false);
-  EXPECT_TRUE(cancel_button->OnKeyPressed(return_event));
-  dialog()->CheckAndResetStates(true, false, false);
-#endif
+TEST_F(DialogTest, ReturnDirectedToCancelButtonPlatformBehavior) {
+  const ui::KeyEvent return_event(ui::ET_KEY_PRESSED, ui::VKEY_RETURN,
+                                  ui::EF_NONE);
+  if (PlatformStyle::kReturnClicksFocusedControl) {
+    EXPECT_TRUE(dialog()->GetCancelButton()->OnKeyPressed(return_event));
+    EXPECT_TRUE(cancelled_);
+  } else {
+    EXPECT_FALSE(dialog()->GetCancelButton()->OnKeyPressed(return_event));
+    EXPECT_FALSE(cancelled_);
+    // If the return key press was not directed *specifically* to the Ok button,
+    // it would bubble upwards here, reach the dialog, and accept it. The
+    // OkButtonAccepts test above covers that behavior.
+  }
+}
 
-  // Check that return accelerators cancel dialogs if cancel is focused, except
-  // on Mac where return should perform the default action.
-  cancel_button->RequestFocus();
-  EXPECT_EQ(cancel_button, dialog()->GetFocusManager()->GetFocusedView());
-  SimulateKeyEvent(return_event);
-#if defined(OS_MACOSX)
-  dialog()->CheckAndResetStates(false, true, false);
-#else
-  dialog()->CheckAndResetStates(true, false, false);
-#endif
+// Unlike the previous two tests, this test simulates a key press at the level
+// of the dialog's Widget, so the return-to-close-dialog behavior does happen.
+TEST_F(DialogTest, ReturnOnCancelButtonPlatformBehavior) {
+  dialog()->GetCancelButton()->RequestFocus();
+  SimulateKeyPress(ui::VKEY_RETURN);
+  if (PlatformStyle::kReturnClicksFocusedControl) {
+    EXPECT_TRUE(cancelled_);
+  } else {
+    EXPECT_TRUE(accepted_);
+  }
+}
 
-  // Check that escape can be overridden.
+TEST_F(DialogTest, CanOverrideEsc) {
   dialog()->set_should_handle_escape(true);
-  SimulateKeyEvent(escape_event);
-  dialog()->CheckAndResetStates(false, false, false);
+  SimulateKeyPress(ui::VKEY_ESCAPE);
+  EXPECT_FALSE(cancelled_);
+  EXPECT_FALSE(closed_);
 }
 
 TEST_F(DialogTest, RemoveDefaultButton) {