Remove DialogDelegate::OnClosed() which is redundant with

ui/views/window/dialog_client_view.cc

 // Copyright (c) 2012 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 "ui/views/window/dialog_client_view.h"
 
 #include <algorithm>
 
 #include "build/build_config.h"
 #include "ui/events/keycodes/keyboard_codes.h"
@@ skipping 40 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 // DialogClientView, public:
 
 DialogClientView::DialogClientView(Widget* owner, View* contents_view)
     : ClientView(owner, contents_view),
       ok_button_(NULL),
       cancel_button_(NULL),
       extra_view_(NULL),
       footnote_view_(NULL),
-      notified_delegate_(false) {
-}
+      delegate_allowed_close_(false) {}
 
 DialogClientView::~DialogClientView() {
 }
 
 void DialogClientView::AcceptWindow() {
-  // Only notify the delegate once. See |notified_delegate_|'s comment.
-  if (!notified_delegate_ && GetDialogDelegate()->Accept(false)) {
-    notified_delegate_ = true;
-    Close();
+  // Only notify the delegate once. See |delegate_allowed_close_|'s comment.
+  if (!delegate_allowed_close_ && GetDialogDelegate()->Accept(false)) {
+    delegate_allowed_close_ = true;
+    GetWidget()->Close();
   }
 }
 
 void DialogClientView::CancelWindow() {
-  // Only notify the delegate once. See |notified_delegate_|'s comment.
-  if (!notified_delegate_ && GetDialogDelegate()->Cancel()) {
-    notified_delegate_ = true;
-    Close();
+  // Only notify the delegate once. See |delegate_allowed_close_|'s comment.
+  if (!delegate_allowed_close_ && GetDialogDelegate()->Cancel()) {
+    delegate_allowed_close_ = true;
+    GetWidget()->Close();
   }
 }
 
 void DialogClientView::UpdateDialogButtons() {
   const int buttons = GetDialogDelegate()->GetDialogButtons();
   ui::Accelerator escape(ui::VKEY_ESCAPE, ui::EF_NONE);
 
   if (buttons & ui::DIALOG_BUTTON_OK) {
     if (!ok_button_) {
       ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
@@ skipping 25 matching lines @@
   if (!has_dialog_buttons())
     AddAccelerator(escape);
   else
     ResetAccelerators();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DialogClientView, ClientView overrides:
 
 bool DialogClientView::CanClose() {
-  if (notified_delegate_)
-    return true;
-
-  // The dialog is closing but no Accept or Cancel action has been performed
-  // before: it's a Close action.
-  if (GetDialogDelegate()->Close()) {
-    notified_delegate_ = true;
-    GetDialogDelegate()->OnClosed();
-    return true;
-  }
-  return false;
+  // If the dialog is closing but no Accept or Cancel action has been performed
+  // before, it's a Close action.
+  delegate_allowed_close_ =

[msw, 2016/02/22 18:44:04]

nit: |=

[Evan Stade, 2016/02/22 22:10:18]

I don't think that has the same behavior. We specifically want to avoid calling
GetDialogDelegate()->Close() if |delegate_allowed_close_| is already true. Since
perhaps that is subtle, I changed this code slightly to make it more obvious.

+      delegate_allowed_close_ || GetDialogDelegate()->Close();
+  return delegate_allowed_close_;
 }
 
 DialogClientView* DialogClientView::AsDialogClientView() {
   return this;
 }
 
 const DialogClientView* DialogClientView::AsDialogClientView() const {
   return this;
 }
 
@@ skipping 86 matching lines @@
 
   // Layout the contents view to the top and side edges of the contents bounds.
   // NOTE: The local insets do not apply to the contents view sides or top.
   const gfx::Rect contents_bounds = GetContentsBounds();
   contents_view()->SetBounds(contents_bounds.x(), contents_bounds.y(),
       contents_bounds.width(), bounds.bottom() - contents_bounds.y());
 }
 
 bool DialogClientView::AcceleratorPressed(const ui::Accelerator& accelerator) {
   DCHECK_EQ(accelerator.key_code(), ui::VKEY_ESCAPE);
-  Close();
+  GetWidget()->Close();
   return true;
 }
 
 void DialogClientView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   ClientView::ViewHierarchyChanged(details);
   if (details.is_add && details.child == this) {
     UpdateDialogButtons();
     CreateExtraView();
     CreateFootnoteView();
@@ skipping 34 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // DialogClientView, protected:
 
 DialogClientView::DialogClientView(View* contents_view)
     : ClientView(NULL, contents_view),
       ok_button_(NULL),
       cancel_button_(NULL),
       extra_view_(NULL),
       footnote_view_(NULL),
-      notified_delegate_(false) {}
+      delegate_allowed_close_(false) {}
 
 DialogDelegate* DialogClientView::GetDialogDelegate() const {
   return GetWidget()->widget_delegate()->AsDialogDelegate();
 }
 
 void DialogClientView::CreateExtraView() {
   if (extra_view_)
     return;
 
   extra_view_ = GetDialogDelegate()->CreateExtraView();
@@ skipping 60 matching lines @@
   return std::max(extra_view_height, buttons_height);
 }
 
 gfx::Insets DialogClientView::GetButtonRowInsets() const {
   // NOTE: The insets only apply to the buttons, extra view, and footnote view.
   return GetButtonsAndExtraViewRowHeight() == 0 ? gfx::Insets() :
       gfx::Insets(0, kButtonHEdgeMarginNew,
                   kButtonVEdgeMarginNew, kButtonHEdgeMarginNew);
 }
 
-void DialogClientView::Close() {
-  GetWidget()->Close();
-  GetDialogDelegate()->OnClosed();
-}
-
 }  // namespace views