views: simplify DialogDelegate close path

This change introduces a new method WidgetDelegate::WindowWillClose() to
allow WidgetDelegates to hook into the Widget lifecycle *after* the
decision that the widget will definitely close has been made but
*before* any internal state of the Widget has actually begun to destroy.
This change then has DialogDelegate use that new hook to implement the
close-callback behavior, rather than having the close callback come via
DialogClientView.

Before this change:
1. [user presses Accept on a dialog]
2. DialogClientView::ButtonPressed()
3. DialogClientView::AcceptWindow()
4. DialogDelegate::Accept() [returns true]
5. Widget::CloseWithReason()
6. NonClientView::CanClose()
7. DialogClientView::CanClose()
8. DialogDelegate::Close (!) [returns true]

After this change:
1. [user presses Accept on a dialog]
2. DialogClientView::ButtonPressed()
3. DialogDelegate::AcceptDialog()
4. DialogDelegate::Accept() [returns true]
5. Widget::CloseWithReason()
6. WidgetDelegate::WindowWillClose()

This returns ClientView::CanClose() to being a pure predicate, at least
for DialogDelegate, rather than a method that has side-effects. Other
subclasses of ClientView still have issues with mixed behavior in this
regard.

This also centralizes the logic for delivering Accept/Close/Cancel
callbacks inside DialogDelegate, which removes some complexity from
DialogClientView.

Bug: 1011446
Change-Id: I96519d6fa4c374de4f06de22c30c4ab5ac03d98e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071037Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746884}

chrome/browser/ui/views/external_protocol_dialog_browsertest.cc

@@ -110,7 +110,7 @@ class ExternalProtocolDialogBrowserTest
 
 IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestAccept) {
   ShowUi(std::string());
-  EXPECT_TRUE(dialog_->Accept());
+  dialog_->AcceptDialog();
   EXPECT_EQ(blocked_state_, BlockState::UNKNOWN);
   EXPECT_TRUE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
@@ -122,7 +122,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
                        TestAcceptWithChecked) {
   ShowUi(std::string());
   SetChecked(true);
-  EXPECT_TRUE(dialog_->Accept());
+  dialog_->AcceptDialog();
   EXPECT_EQ(blocked_scheme_, "telnet");
   EXPECT_EQ(blocked_state_, BlockState::DONT_BLOCK);
   EXPECT_TRUE(url_did_launch_);
@@ -138,7 +138,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
   ShowUi(std::string());
   SetChecked(true);  // |remember_| must be true for the segfault to occur.
   browser()->tab_strip_model()->CloseAllTabs();
-  EXPECT_TRUE(dialog_->Accept());
+  dialog_->AcceptDialog();
   EXPECT_FALSE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
       ExternalProtocolHandler::kHandleStateMetric,
@@ -147,7 +147,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestCancel) {
   ShowUi(std::string());
-  EXPECT_TRUE(dialog_->Cancel());
+  dialog_->CancelDialog();
   EXPECT_EQ(blocked_state_, BlockState::UNKNOWN);
   EXPECT_FALSE(url_did_launch_);
   histogram_tester_.ExpectBucketCount(
@@ -159,7 +159,7 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest,
                        TestCancelWithChecked) {
   ShowUi(std::string());
   SetChecked(true);
-  EXPECT_TRUE(dialog_->Cancel());
+  dialog_->CancelDialog();
   // Cancel() should not enforce the remember checkbox.
   EXPECT_EQ(blocked_state_, BlockState::UNKNOWN);
   EXPECT_FALSE(url_did_launch_);

ui/views/widget/widget.cc

@@ -616,6 +616,9 @@ void Widget::CloseWithReason(ClosedReason closed_reason) {
   for (WidgetObserver& observer : observers_)
     observer.OnWidgetClosing(this);
 
+  if (widget_delegate_)
+    widget_delegate_->WindowWillClose();
+
   native_widget_->Close();
 }
 

ui/views/widget/widget_delegate.h

@@ -132,13 +132,22 @@ class VIEWS_EXPORT WidgetDelegate {
   // Default is true.
   virtual bool ShouldRestoreWindowSize() const;
 
-  // Called when the window closes. The delegate MUST NOT delete itself during
-  // this call, since it can be called afterwards. See DeleteDelegate().
+  // Hooks for the end of the Widget/Window lifecycle. As of this writing, these
+  // callbacks happen like so:
+  //   1. Client code calls Widget::CloseWithReason()
+  //   2. WidgetDelegate::WindowWillClose() is called
+  //   3. NativeWidget teardown (maybe async) starts OR the operating system
+  //      abruptly closes the backing native window
+  //   4. WidgetDelegate::WindowClosing() is called
+  //   5. NativeWidget teardown completes, Widget teardown starts
+  //   6. WidgetDelegate::DeleteDelegate() is called
+  //   7. Widget teardown finishes, Widget is deleted
+  // At step 3, the "maybe async" is controlled by whether the close is done via
+  // Close() or CloseNow().
+  // Important note: for OS-initiated window closes, steps 1 and 2 don't happen
+  // - i.e, WindowWillClose() is never invoked.
+  virtual void WindowWillClose() {}
   virtual void WindowClosing() {}
-
-  // Called when the window is destroyed. No events must be sent or received
-  // after this point. The delegate can use this opportunity to delete itself at
-  // this time if necessary.
   virtual void DeleteDelegate() {}
 
   // Called when the user begins/ends to change the bounds of the window.

ui/views/window/dialog_client_view.cc

@@ -86,41 +86,12 @@ DialogClientView::~DialogClientView() {
     dialog->RemoveObserver(this);
 }
 
-void DialogClientView::AcceptWindow() {
-  // Only notify the delegate once. See |delegate_allowed_close_|'s comment.
-  if (!delegate_allowed_close_ && GetDialogDelegate()->Accept()) {
-    delegate_allowed_close_ = true;
-    GetWidget()->CloseWithReason(
-        views::Widget::ClosedReason::kAcceptButtonClicked);
-  }
-}
-
-void DialogClientView::CancelWindow() {
-  // Only notify the delegate once. See |delegate_allowed_close_|'s comment.
-  if (!delegate_allowed_close_ && GetDialogDelegate()->Cancel()) {
-    delegate_allowed_close_ = true;
-    GetWidget()->CloseWithReason(
-        views::Widget::ClosedReason::kCancelButtonClicked);
-  }
-}
-
 void DialogClientView::SetButtonRowInsets(const gfx::Insets& insets) {
   button_row_insets_ = insets;
   if (GetWidget())
     UpdateDialogButtons();
 }
 
-///////////////////////////////////////////////////////////////////////////////
-// DialogClientView, ClientView overrides:
-
-bool DialogClientView::CanClose() {
-  // If the dialog is closing but no Accept or Cancel action has been performed
-  // before, it's a Close action.
-  if (!delegate_allowed_close_)
-    delegate_allowed_close_ = GetDialogDelegate()->Close();
-  return delegate_allowed_close_;
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // DialogClientView, View overrides:
 
@@ -235,9 +206,9 @@ void DialogClientView::ButtonPressed(Button* sender, const ui::Event& event) {
     return;
 
   if (sender == ok_button_)
-    AcceptWindow();
+    GetDialogDelegate()->AcceptDialog();
   else if (sender == cancel_button_)
-    CancelWindow();
+    GetDialogDelegate()->CancelDialog();
   else
     NOTREACHED();
 }

ui/views/window/dialog_client_view.h

@@ -43,10 +43,6 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   DialogClientView(Widget* widget, View* contents_view);
   ~DialogClientView() override;
 
-  // Accept or Cancel the dialog.
-  void AcceptWindow();
-  void CancelWindow();
-
   // Accessors in case the user wishes to adjust these buttons.
   LabelButton* ok_button() const { return ok_button_; }
   LabelButton* cancel_button() const { return cancel_button_; }
@@ -54,9 +50,6 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
 
   void SetButtonRowInsets(const gfx::Insets& insets);
 
-  // ClientView implementation:
-  bool CanClose() override;
-
   // View implementation:
   gfx::Size CalculatePreferredSize() const override;
   gfx::Size GetMinimumSize() const override;
@@ -136,12 +129,6 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   // Container view for the button row.
   ButtonRowContainer* button_row_container_ = nullptr;
 
-  // True if we've notified the delegate the window is closing and the delegate
-  // allowed the close. In some situations it's possible to get two closes (see
-  // http://crbug.com/71940). This is used to avoid notifying the delegate
-  // twice, which can have bad consequences.
-  bool delegate_allowed_close_ = false;
-
   // Used to prevent unnecessary or potentially harmful changes during
   // SetupLayout(). Everything will be manually updated afterwards.
   bool adding_or_removing_views_ = false;

ui/views/window/dialog_delegate.cc

@@ -137,52 +137,22 @@ bool DialogDelegate::IsDialogButtonEnabled(ui::DialogButton button) const {
 }
 
 bool DialogDelegate::Cancel() {
+  DCHECK(!already_started_close_);
   if (cancel_callback_)
     RunCloseCallback(std::move(cancel_callback_));
   return true;
 }
 
 bool DialogDelegate::Accept() {
+  DCHECK(!already_started_close_);
   if (accept_callback_)
     RunCloseCallback(std::move(accept_callback_));
   return true;
 }
 
-bool DialogDelegate::Close() {
-  // Test for callback_delivered_ here, because it indicates that:
-  // 1) A new-style callback used to be present in one of these slots
-  // 2) It has already been invoked and therefore the slot is now empty
-  if (callback_delivered_ || close_callback_ || cancel_callback_ ||
-      accept_callback_) {
-    if (close_callback_)
-      RunCloseCallback(std::move(close_callback_));
-    return true;
-  } else {
-    return DefaultClose();
-  }
-}
-
-bool DialogDelegate::DefaultClose() {
-  DCHECK(!close_callback_);
-  DCHECK(!cancel_callback_);
-  DCHECK(!accept_callback_);
-  int buttons = GetDialogButtons();
-  if ((buttons & ui::DIALOG_BUTTON_CANCEL) ||
-      (buttons == ui::DIALOG_BUTTON_NONE)) {
-    return Cancel();
-  }
-  return Accept();
-}
-
 void DialogDelegate::RunCloseCallback(base::OnceClosure callback) {
-  // TODO(ellyjones): It would be better if this instead was:
-  //   DCHECK(!callback_delivered_);
-  // i.e., it was enforced by Views that client code does not cause the
-  // DialogDelegate to be closed from within a closure handler. Unfortunately a
-  // lot of client code does not behave that way right now.
-  if (callback_delivered_)
-    return;
-  callback_delivered_ = true;
+  DCHECK(!already_started_close_);
+  already_started_close_ = true;
   std::move(callback).Run();
 }
 
@@ -223,6 +193,36 @@ NonClientFrameView* DialogDelegate::CreateNonClientFrameView(Widget* widget) {
   return WidgetDelegate::CreateNonClientFrameView(widget);
 }
 
+void DialogDelegate::WindowWillClose() {
+  if (already_started_close_)
+    return;
+
+  bool new_callback_present =
+      close_callback_ || cancel_callback_ || accept_callback_;
+
+  if (close_callback_)
+    RunCloseCallback(std::move(close_callback_));
+
+  if (new_callback_present)
+    return;
+
+  // Old-style close behavior: if the only button was Ok, call Accept();
+  // otherwise call Cancel(). Note that in this case the window is already going
+  // to close, so the return values of Accept()/Cancel(), which normally say
+  // whether the window should close, are ignored.
+  int buttons = GetDialogButtons();
+  if (buttons == ui::DIALOG_BUTTON_OK)
+    Accept();
+  else
+    Cancel();
+
+  // This is set here instead of before the invocations of Accept()/Cancel() so
+  // that those methods can DCHECK that !already_started_close_. Otherwise,
+  // client code could (eg) call Accept() from inside the cancel callback, which
+  // could lead to multiple callbacks being delivered from this class.
+  already_started_close_ = true;
+}
+
 // static
 NonClientFrameView* DialogDelegate::CreateDialogFrameView(Widget* widget) {
   LayoutProvider* provider = LayoutProvider::Get();
@@ -325,12 +325,9 @@ std::unique_ptr<View> DialogDelegate::DisownExtraView() {
   return std::move(extra_view_);
 }
 
-void DialogDelegate::CancelDialog() {
-  GetDialogClientView()->CancelWindow();
-}
-
-void DialogDelegate::AcceptDialog() {
-  GetDialogClientView()->AcceptWindow();
+bool DialogDelegate::Close() {
+  WindowWillClose();
+  return true;
 }
 
 void DialogDelegate::ResetViewShownTimeStampForTesting() {
@@ -341,6 +338,24 @@ void DialogDelegate::SetButtonRowInsets(const gfx::Insets& insets) {
   GetDialogClientView()->SetButtonRowInsets(insets);
 }
 
+void DialogDelegate::AcceptDialog() {
+  if (already_started_close_ || !Accept())
+    return;
+
+  already_started_close_ = true;
+  GetWidget()->CloseWithReason(
+      views::Widget::ClosedReason::kAcceptButtonClicked);
+}
+
+void DialogDelegate::CancelDialog() {
+  if (already_started_close_ || !Cancel())
+    return;
+
+  already_started_close_ = true;
+  GetWidget()->CloseWithReason(
+      views::Widget::ClosedReason::kCancelButtonClicked);
+}
+
 DialogDelegate::~DialogDelegate() {
   UMA_HISTOGRAM_LONG_TIMES("Dialog.DialogDelegate.Duration",
                            base::TimeTicks::Now() - creation_time_);

ui/views/window/dialog_delegate.h

@@ -129,6 +129,7 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   DialogDelegate* AsDialogDelegate() override;
   ClientView* CreateClientView(Widget* widget) override;
   NonClientFrameView* CreateNonClientFrameView(Widget* widget) override;
+  void WindowWillClose() override;
 
   static NonClientFrameView* CreateDialogFrameView(Widget* widget);
 
@@ -225,16 +226,19 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   //    lazy layout system in View::InvalidateLayout
   std::unique_ptr<View> DisownExtraView();
 
-  // Externally or accept the dialog. These methods:
+  // Accept or cancel the dialog, as though the user had pressed the
+  // Accept/Cancel buttons. These methods:
   // 1) Invoke the DialogDelegate's Cancel or Accept methods
   // 2) Depending on their return value, close the dialog's widget.
   // Neither of these methods can be called before the dialog has been
   // initialized.
-  void CancelDialog();
   void AcceptDialog();
+  void CancelDialog();
 
-  // Deprecated, for compatibility with a few remaining unit tests.
-  // TODO(https://crbug.com/1011446): Delete this method.
+  // This method invokes the behavior that *would* happen if this dialog's
+  // containing widget were closed. It is present only as a compatibility shim
+  // for unit tests; do not add new calls to it.
+  // TODO(https://crbug.com/1011446): Delete this.
   bool Close();
 
   // Reset the dialog's shown timestamp, for tests that are subject to the
@@ -269,10 +273,6 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   const DialogClientView* GetDialogClientView() const;
   DialogClientView* GetDialogClientView();
 
-  // Implements the default close behavior, as described in the comment on
-  // Close() above.
-  bool DefaultClose();
-
   // Runs a close callback, ensuring that at most one close callback is ever
   // run.
   void RunCloseCallback(base::OnceClosure callback);
@@ -303,8 +303,9 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   base::OnceClosure cancel_callback_;
   base::OnceClosure close_callback_;
 
-  // Whether any of the three callbacks just above has been delivered yet.
-  bool callback_delivered_ = false;
+  // Whether any of the three callbacks just above has been delivered yet, *or*
+  // one of the Accept/Cancel methods have been called and returned true.
+  bool already_started_close_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(DialogDelegate);
 };

ui/views/window/dialog_delegate_unittest.cc

@@ -17,6 +17,7 @@
 #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/test/widget_test.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 
@@ -466,4 +467,41 @@ TEST_F(DialogDelegateCloseTest,
   EXPECT_FALSE(closed);
 }
 
+class TestDialogDelegateView : public DialogDelegateView {
+ public:
+  TestDialogDelegateView(bool* accepted, bool* cancelled)
+      : accepted_(accepted), cancelled_(cancelled) {}
+  ~TestDialogDelegateView() override = default;
+
+ private:
+  bool Accept() override {
+    *(accepted_) = true;
+    return true;
+  }
+  bool Cancel() override {
+    *(cancelled_) = true;
+    return true;
+  }
+
+  bool* accepted_;
+  bool* cancelled_;
+};
+
+TEST_F(DialogDelegateCloseTest, OldClosePathDoesNotDoubleClose) {
+  bool accepted = false;
+  bool cancelled = false;
+
+  auto* dialog = new TestDialogDelegateView(&accepted, &cancelled);
+  Widget* widget =
+      DialogDelegate::CreateDialogWidget(dialog, GetContext(), nullptr);
+  widget->Show();
+
+  views::test::WidgetDestroyedWaiter destroyed_waiter(widget);
+  dialog->AcceptDialog();
+  destroyed_waiter.Wait();
+
+  EXPECT_TRUE(accepted);
+  EXPECT_FALSE(cancelled);
+}
+
 }  // namespace views