Cleanups for DialogClientView, DialogClientViewTest.

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/base/material_design/material_design_controller.h"
@@ skipping 75 matching lines @@
 
 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()->Close();
   }
 }
 
 void DialogClientView::UpdateDialogButtons() {
-  const int buttons = GetDialogDelegate()->GetDialogButtons();
-
-  if (buttons & ui::DIALOG_BUTTON_OK) {
-    if (!ok_button_) {
-      ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
-      AddChildView(ok_button_);
-    }
-
-    GetDialogDelegate()->UpdateButton(ok_button_, ui::DIALOG_BUTTON_OK);
-  } else if (ok_button_) {
-    delete ok_button_;
-    ok_button_ = nullptr;
-  }
-
-  if (buttons & ui::DIALOG_BUTTON_CANCEL) {
-    if (!cancel_button_) {
-      cancel_button_ = CreateDialogButton(ui::DIALOG_BUTTON_CANCEL);
-      AddChildView(cancel_button_);
-    }
-
-    GetDialogDelegate()->UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
-  } else if (cancel_button_) {
-    delete cancel_button_;
-    cancel_button_ = nullptr;
-  }
-
+  SetupViews();
   SetupFocusChain();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // 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* DialogClientView::AsDialogClientView() {
   return this;
 }
 
 const DialogClientView* DialogClientView::AsDialogClientView() const {
   return this;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DialogClientView, View overrides:
 
 gfx::Size DialogClientView::GetPreferredSize() const {
   // Initialize the size to fit the buttons and extra view row.
-  int extra_view_padding = 0;
-  if (!GetDialogDelegate()->GetExtraViewPadding(&extra_view_padding))
-    extra_view_padding =
-        ViewsDelegate::GetInstance()->GetDialogRelatedButtonHorizontalSpacing();
   gfx::Size size(
       (ok_button_ ? ok_button_->GetPreferredSize().width() : 0) +

[sky, 2017/02/22 17:29:48]

IMO this code would be more readable if it was a bunch of conditionals, e.g.:

int width = 0;
if (ok_button_)
  width += ok_button_->GetPreferredSize().width();
...

It will likely span more lines, but I think it results in easier to read code. I
think it makes the last line easier to read too, e.g.:

if (ShouldShow(extra_view_)) {
  width += extra_view_->GetPreferredSize().width + GetExtraViewSpacing();
}

[tapted, 2017/02/22 23:20:43]

I'm just going to delete it all in https://codereview.chromium.org/2706423002/ -
it will be glorious :). (i.e. GridLayout is much better equipped to calculate
GetPreferredSize() - `GetExtraViewSpacing` was the only thing I still needed)

           (cancel_button_ ? cancel_button_->GetPreferredSize().width() : 0) +
           (cancel_button_ && ok_button_
                ? ViewsDelegate::GetInstance()
                      ->GetDialogRelatedButtonHorizontalSpacing()
                : 0) +
           (ShouldShow(extra_view_) ? extra_view_->GetPreferredSize().width()
                                    : 0) +
-          (ShouldShow(extra_view_) && has_dialog_buttons() ? extra_view_padding
-                                                           : 0),
+          GetExtraViewSpacing(),
       0);
 
   int buttons_height = GetButtonsAndExtraViewRowHeight();
   if (buttons_height != 0) {
     size.Enlarge(0, buttons_height);
     // Inset the buttons and extra view.
     const gfx::Insets insets = GetButtonRowInsets();
     size.Enlarge(insets.width(), insets.height());
   }
 
@@ skipping 64 matching lines @@
 
 bool DialogClientView::AcceleratorPressed(const ui::Accelerator& accelerator) {
   DCHECK_EQ(accelerator.key_code(), ui::VKEY_ESCAPE);
 
   GetWidget()->Close();
   return true;
 }
 
 void DialogClientView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
+  View* const child = details.child;
+
+  // Dialogs must add children to contents_view(), not client_view().
+  if (details.is_add && details.parent == this) {
+    DCHECK(child == contents_view() || child == ok_button_ ||
+           child == cancel_button_ || child == extra_view_);
+  }
+
   ClientView::ViewHierarchyChanged(details);
-  if (details.is_add && details.child == this) {
+  if (details.is_add && child == this) {
     UpdateDialogButtons();
-    CreateExtraView();
-  } else if (!details.is_add && details.child != this) {
-    if (details.child == ok_button_)
+  } else if (!details.is_add && child != this) {

[sky, 2017/02/22 17:29:48]

child != this isn't really needed here.

[tapted, 2017/02/22 23:20:44]

Removed (follow-up doesn't need it either)

+    if (child == ok_button_)
       ok_button_ = nullptr;
-    else if (details.child == cancel_button_)
+    else if (child == cancel_button_)
       cancel_button_ = nullptr;
-    else if (details.child == extra_view_)
+    else if (child == extra_view_)
       extra_view_ = nullptr;
   }
 }
 
 void DialogClientView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   // The old dialog style needs an explicit background color, while the new
   // dialog style simply inherits the bubble's frame view color.
   const DialogDelegate* dialog = GetDialogDelegate();
 
   if (dialog && !dialog->ShouldUseCustomFrame()) {
@@ skipping 12 matching lines @@
 
   if (sender == ok_button_)
     AcceptWindow();
   else if (sender == cancel_button_)
     CancelWindow();
   else
     NOTREACHED();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// DialogClientView, protected:
-
-DialogClientView::DialogClientView(View* contents_view)
-    : ClientView(nullptr, contents_view),
-      ok_button_(nullptr),
-      cancel_button_(nullptr),
-      extra_view_(nullptr),
-      delegate_allowed_close_(false) {}
+// DialogClientView, private:
 
 DialogDelegate* DialogClientView::GetDialogDelegate() const {
   return GetWidget()->widget_delegate()->AsDialogDelegate();
 }
 
-void DialogClientView::CreateExtraView() {
-  if (extra_view_)
-    return;
-
-  extra_view_ = GetDialogDelegate()->CreateExtraView();
-  if (extra_view_) {
-    extra_view_->SetGroup(kButtonGroup);
-    AddChildView(extra_view_);
-    SetupFocusChain();
-  }
-}
-
 void DialogClientView::ChildPreferredSizeChanged(View* child) {
   if (child == extra_view_)
     Layout();
 }
 
 void DialogClientView::ChildVisibilityChanged(View* child) {
   ChildPreferredSizeChanged(child);
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// DialogClientView, private:
-
 LabelButton* DialogClientView::CreateDialogButton(ui::DialogButton type) {
   const base::string16 title = GetDialogDelegate()->GetDialogButtonLabel(type);
   LabelButton* button = nullptr;
 
   const bool is_default =
       GetDialogDelegate()->GetDefaultDialogButton() == type &&
       (type != ui::DIALOG_BUTTON_CANCEL ||
        PlatformStyle::kDialogDefaultButtonCanBeCancel);
 
   // The default button is always blue in Harmony.
@@ skipping 62 matching lines @@
   child_views.erase(
       std::remove(child_views.begin(), child_views.end(), nullptr),
       child_views.end());
 
   // Setup focus by reordering views. It is not safe to use SetNextFocusableView
   // since child views may be added externally to this view.
   for (size_t i = 0; i < child_views.size(); i++)
     ReorderChildView(child_views[i], i);
 }
 
+int DialogClientView::GetExtraViewSpacing() const {
+  if (!ShouldShow(extra_view_) || !has_dialog_buttons())
+    return 0;
+
+  int extra_view_padding = 0;
+  if (GetDialogDelegate()->GetExtraViewPadding(&extra_view_padding))
+    return extra_view_padding;
+
+  return ViewsDelegate::GetInstance()
+      ->GetDialogRelatedButtonHorizontalSpacing();
+}
+
+void DialogClientView::SetupViews() {
+  const int buttons = GetDialogDelegate()->GetDialogButtons();
+
+  if (buttons & ui::DIALOG_BUTTON_OK) {
+    if (!ok_button_) {
+      ok_button_ = CreateDialogButton(ui::DIALOG_BUTTON_OK);
+      AddChildView(ok_button_);
+    }
+
+    GetDialogDelegate()->UpdateButton(ok_button_, ui::DIALOG_BUTTON_OK);
+  } else if (ok_button_) {
+    delete ok_button_;
+    ok_button_ = nullptr;
+  }
+
+  if (buttons & ui::DIALOG_BUTTON_CANCEL) {
+    if (!cancel_button_) {
+      cancel_button_ = CreateDialogButton(ui::DIALOG_BUTTON_CANCEL);
+      AddChildView(cancel_button_);
+    }
+
+    GetDialogDelegate()->UpdateButton(cancel_button_, ui::DIALOG_BUTTON_CANCEL);
+  } else if (cancel_button_) {
+    delete cancel_button_;
+    cancel_button_ = nullptr;
+  }
+
+  if (extra_view_)
+    return;
+
+  extra_view_ = GetDialogDelegate()->CreateExtraView();
+  if (extra_view_) {
+    extra_view_->SetGroup(kButtonGroup);
+    AddChildView(extra_view_);
+  }
+}
+
 }  // namespace views