views: remove old DialogDelegate default close behavior

This change removes the legacy close behavior of DialogDelegate,
which was as follows: if a dialog was closed without either
accepting or cancelling, if the dialog had only an ok button, the
Accept() method would be called; otherwise, the Cancel() method
would be called.

This behavior was only depended on by a single dialog - the
collected cookies dialog - so this change migrates the collected
cookies dialog over to the new callback system.

This change will allow for introducing a useful invariant: that the
DialogDelegate::Close() callback always runs after
WidgetObservers observe that the dialog has begun to close.

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

chrome/browser/ui/views/collected_cookies_views.cc

@@ -280,20 +280,6 @@ base::string16 CollectedCookiesViews::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_COLLECTED_COOKIES_DIALOG_TITLE);
 }
 
-bool CollectedCookiesViews::Accept() {
-  // If the user closes our parent tab while we're still open, this method will
-  // (eventually) be called in response to a WebContentsDestroyed() call from
-  // the WebContentsImpl to its observers.  But since the InfoBarService is also
-  // torn down in response to WebContentsDestroyed(), it may already be null.
-  // Since the tab is going away anyway, we can just omit showing an infobar,
-  // which prevents any attempt to access a null InfoBarService.
-  if (status_changed_ && !web_contents_->IsBeingDestroyed()) {
-    CollectedCookiesInfoBarDelegate::Create(
-        InfoBarService::FromWebContents(web_contents_));
-  }
-  return true;
-}
-
 ui::ModalType CollectedCookiesViews::GetModalType() const {
   return ui::MODAL_TYPE_CHILD;
 }
@@ -367,6 +353,11 @@ CollectedCookiesViews::CollectedCookiesViews(content::WebContents* web_contents)
       SetLayoutManager(std::make_unique<views::GridLayout>());
   ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
 
+  SetAcceptCallback(base::BindOnce(&CollectedCookiesViews::OnDialogClosed,
+                                   base::Unretained(this)));
+  SetCloseCallback(base::BindOnce(&CollectedCookiesViews::OnDialogClosed,
+                                  base::Unretained(this)));
+
   // Add margin above the content. The left, right, and bottom margins are added
   // by the content itself.
   SetBorder(views::CreateEmptyBorder(
@@ -414,6 +405,19 @@ CollectedCookiesViews::CollectedCookiesViews(content::WebContents* web_contents)
   ShowCookieInfo();
 }
 
+void CollectedCookiesViews::OnDialogClosed() {
+  // If the user closes our parent tab while we're still open, this method will
+  // (eventually) be called in response to a WebContentsDestroyed() call from
+  // the WebContentsImpl to its observers.  But since the InfoBarService is also
+  // torn down in response to WebContentsDestroyed(), it may already be null.
+  // Since the tab is going away anyway, we can just omit showing an infobar,
+  // which prevents any attempt to access a null InfoBarService.
+  if (status_changed_ && !web_contents_->IsBeingDestroyed()) {
+    CollectedCookiesInfoBarDelegate::Create(
+        InfoBarService::FromWebContents(web_contents_));
+  }
+}
+
 std::unique_ptr<views::View> CollectedCookiesViews::CreateAllowedPane() {
   // This captures a snapshot of the allowed cookies of the current page so we
   // are fine using WebContents::GetMainFrame() here

chrome/browser/ui/views/collected_cookies_views.h

@@ -51,7 +51,6 @@ class CollectedCookiesViews
 
   // views::DialogDelegate:
   base::string16 GetWindowTitle() const override;
-  bool Accept() override;
   ui::ModalType GetModalType() const override;
   bool ShouldShowCloseButton() const override;
   void DeleteDelegate() override;
@@ -74,8 +73,9 @@ class CollectedCookiesViews
 
   explicit CollectedCookiesViews(content::WebContents* web_contents);
 
-  std::unique_ptr<views::View> CreateAllowedPane();
+  void OnDialogClosed();
 
+  std::unique_ptr<views::View> CreateAllowedPane();
   std::unique_ptr<views::View> CreateBlockedPane();
 
   // Creates and returns the "buttons pane", which is the view in the

ui/views/window/dialog_delegate.cc

@@ -213,16 +213,6 @@ void DialogDelegate::WindowWillClose() {
   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

ui/views/window/dialog_delegate.h

@@ -119,19 +119,16 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
   virtual bool IsDialogButtonEnabled(ui::DialogButton button) const;
 
   // For Dialog boxes, if there is a "Cancel" button or no dialog button at all,
-  // this is called when the user presses the "Cancel" button.
-  // It can also be called on a close action if |Close| has not been
-  // overridden. This function should return true if the window can be closed
-  // after it returns, or false if it must remain open. By default, return true
-  // without doing anything.
+  // this is called when the user presses the "Cancel" button.  This function
+  // should return true if the window can be closed after it returns, or false
+  // if it must remain open. By default, return true without doing anything.
   // DEPRECATED: use |SetCancelCallback| instead.
   virtual bool Cancel();
 
-  // For Dialog boxes, this is called when the user presses the "OK" button,
-  // or the Enter key. It can also be called on a close action if |Close|
-  // has not been overridden. This function should return true if the window
-  // can be closed after it returns, or false if it must remain open. By
-  // default, return true without doing anything.
+  // For Dialog boxes, this is called when the user presses the "OK" button, or
+  // the Enter key. This function should return true if the window can be closed
+  // after it returns, or false if it must remain open. By default, return true
+  // without doing anything.
   // DEPRECATED: use |SetAcceptCallback| instead.
   virtual bool Accept();