Harmony - convert hung renderer dialog.

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"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/blue_button.h"
 #include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/layout/layout_constants.h"

[Peter Kasting, 2017/02/13 23:47:31]

Nit: Can this go away?

[Bret, 2017/02/16 00:25:02]

Yes, done.

 #include "ui/views/style/platform_style.h"
 #include "ui/views/views_delegate.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 
 namespace views {
 
 namespace {
 
 // The group used by the buttons.  This name is chosen voluntarily big not to
@@ skipping 19 matching lines @@
   if (!button)
     return;
 
   const gfx::Size size = button->GetPreferredSize();
   row_bounds->set_width(row_bounds->width() - size.width());
   DCHECK_LE(button_height, row_bounds->height());
   button->SetBounds(
       row_bounds->right(),
       row_bounds->y() + (row_bounds->height() - button_height) / 2,
       size.width(), button_height);
-  int spacing = ViewsDelegate::GetInstance()
-                    ? ViewsDelegate::GetInstance()
-                          ->GetDialogRelatedButtonHorizontalSpacing()
-                    : kRelatedButtonHSpacing;
+  const int spacing =
+      ViewsDelegate::GetInstance()->GetDialogRelatedButtonHorizontalSpacing();
   row_bounds->set_width(row_bounds->width() - spacing);
 }
 
 }  // namespace
 
 ///////////////////////////////////////////////////////////////////////////////
 // DialogClientView, public:
 
 DialogClientView::DialogClientView(Widget* owner, View* contents_view)
     : ClientView(owner, contents_view),
+      button_row_insets_(ViewsDelegate::GetInstance()->GetDialogButtonInsets()),
       ok_button_(nullptr),
       cancel_button_(nullptr),
       extra_view_(nullptr),
       delegate_allowed_close_(false) {
-  button_row_insets_ =
-      ViewsDelegate::GetInstance()
-          ? ViewsDelegate::GetInstance()->GetDialogButtonInsets()
-          : gfx::Insets(0, kButtonHEdgeMarginNew, kButtonVEdgeMarginNew,
-                        kButtonHEdgeMarginNew);
   // Doing this now ensures this accelerator will have lower priority than
   // one set by the contents view.
   AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
-
-  if (ViewsDelegate::GetInstance())
-    button_row_insets_ = ViewsDelegate::GetInstance()->GetDialogButtonInsets();
 }
 
 DialogClientView::~DialogClientView() {
 }
 
 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()->Close();
@@ skipping 57 matching lines @@
   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()
-                             ? ViewsDelegate::GetInstance()
-                                   ->GetDialogRelatedButtonHorizontalSpacing()
-                             : kRelatedButtonHSpacing;
+    extra_view_padding =
+        ViewsDelegate::GetInstance()->GetDialogRelatedButtonHorizontalSpacing();
   gfx::Size size(
       (ok_button_ ? ok_button_->GetPreferredSize().width() : 0) +
           (cancel_button_ ? cancel_button_->GetPreferredSize().width() : 0) +
           (cancel_button_ && ok_button_
-           ? (ViewsDelegate::GetInstance()
-                  ? ViewsDelegate::GetInstance()
-                        ->GetDialogRelatedButtonHorizontalSpacing()
-                  : kRelatedButtonHSpacing) : 0) +
-      (ShouldShow(extra_view_) ? extra_view_->GetPreferredSize().width() : 0) +
-      (ShouldShow(extra_view_) && has_dialog_buttons() ? extra_view_padding
-                                                       : 0),
+               ? ViewsDelegate::GetInstance()
+                     ->GetDialogRelatedButtonHorizontalSpacing()
+               : 0) +
+          (ShouldShow(extra_view_) ? extra_view_->GetPreferredSize().width()
+                                   : 0) +
+          (ShouldShow(extra_view_) && has_dialog_buttons() ? extra_view_padding
+                                                           : 0),
       0);
 
   int buttons_height = GetButtonsAndExtraViewRowHeight();
   if (buttons_height != 0) {
-    size.Enlarge(0, buttons_height + GetButtonsAndExtraViewRowTopPadding());

[Peter Kasting, 2017/02/13 23:47:31]

The changes to eliminate this function and its callers below make this insane
code more sane.  But are they correct?  It makes my head hurt to try and think
through this, so I'm punting and just asking whether you thought really
carefully through both Harmony and pre-Harmony layout to ensure that either this
is identical to the previous behavior, or else it actually fixes bugs in the
previous layout.

(I could believe, for example, that pre-Harmony layout used to double-space the
gap between content and button row, and this single-spaces it, but we want
that.)

[Bret, 2017/02/16 00:25:02]

Yes, this is pretty wild, which is why I tried to punt on it earlier by just
returning 0 in harmony. But having actually gone through it carefully I'm pretty
sure this is correct.

There are basically only two cases to consider: the button_row_insets_.top() ==
0 and button_row_insets_.top() > 0. Since the former case is the default that
means the actual top margin will be RELATED_CONTROL_VERTICAL_SPACING in almost
every case. In the very small minority of cases where the subclass explicitly
sets button_row_insets_.top() to >0 we would apply that inset twice, and I think
it was actually introduced when we started refactoring for harmony so I highly
doubt it was intentional.

As for the changes in Layout, the only reason we were adding
GetButtonsAndExtraViewRowTopPadding at the end was because it wasn't accounted
for in GetButtonsAndExtraViewRowHeight, and now it is.

+    size.Enlarge(0, buttons_height);
     // Inset the buttons and extra view.
     const gfx::Insets insets = GetButtonRowInsets();
     size.Enlarge(insets.width(), insets.height());
   }
 
   // Increase the size as needed to fit the contents view.
   // NOTE: The contents view is not inset on the top or side client view edges.
   gfx::Size contents_size = contents_view()->GetPreferredSize();
   size.Enlarge(0, contents_size.height());
   size.set_width(std::max(size.width(), contents_size.width()));
@@ skipping 21 matching lines @@
     } else {
       LayoutButton(ok_button_, &row_bounds, button_height);
       LayoutButton(cancel_button_, &row_bounds, button_height);
     }
     if (extra_view_) {
       int custom_padding = 0;
       if (has_dialog_buttons() &&
           GetDialogDelegate()->GetExtraViewPadding(&custom_padding)) {
         // The call to LayoutButton() will already have accounted for some of
         // the padding.
-        custom_padding -= GetButtonsAndExtraViewRowTopPadding();
+        custom_padding -= GetButtonRowInsets().top();
         row_bounds.set_width(row_bounds.width() - custom_padding);
       }
       row_bounds.set_width(std::min(row_bounds.width(),
-          extra_view_->GetPreferredSize().width()));
+                                    extra_view_->GetPreferredSize().width()));
       extra_view_->SetBoundsRect(row_bounds);
     }
 
     if (height > 0)
-      bounds.Inset(0, 0, 0, height + GetButtonsAndExtraViewRowTopPadding());
+      bounds.Inset(0, 0, 0, height);
   }
 
   // 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) {
@@ skipping 94 matching lines @@
        PlatformStyle::kDialogDefaultButtonCanBeCancel);
 
   // The default button is always blue in Harmony.
   if (is_default && (ui::MaterialDesignController::IsSecondaryUiMaterial() ||
                      GetDialogDelegate()->ShouldDefaultButtonBeBlue())) {
     button = MdTextButton::CreateSecondaryUiBlueButton(this, title);
   } else {
     button = MdTextButton::CreateSecondaryUiButton(this, title);
   }
 
-  const int kDialogMinButtonWidth = 75;
-  button->SetMinSize(gfx::Size(kDialogMinButtonWidth, 0));
+  // TODO(bsep): Setting the minimum size is redundant with MdTextButton, so
+  // this can be deleted when harmony is always on.
+  const int minimum_width =
+      ViewsDelegate::GetInstance()->GetDialogButtonMinimumWidth();
+  button->SetMinSize(gfx::Size(minimum_width, 0));
+
   button->SetGroup(kButtonGroup);
   return button;
 }
 
 int DialogClientView::GetButtonHeight() const {
   return std::max(
       ok_button_ ? ok_button_->GetPreferredSize().height() : 0,
       cancel_button_ ? cancel_button_->GetPreferredSize().height() : 0);
 }
 
 int DialogClientView::GetExtraViewHeight() const {
   return ShouldShow(extra_view_) ? extra_view_->GetPreferredSize().height() : 0;
 }
 
 int DialogClientView::GetButtonsAndExtraViewRowHeight() const {
   return std::max(GetExtraViewHeight(), GetButtonHeight());
 }
 
 gfx::Insets DialogClientView::GetButtonRowInsets() const {
-  return GetButtonsAndExtraViewRowHeight() == 0 ? gfx::Insets()
-                                                : button_row_insets_;
-}
+  if (GetButtonsAndExtraViewRowHeight() == 0)
+    return gfx::Insets();
 
-int DialogClientView::GetButtonsAndExtraViewRowTopPadding() const {
-  int spacing = button_row_insets_.top();
   // Some subclasses of DialogClientView, in order to do their own layout, set
   // button_row_insets_ to gfx::Insets(). To avoid breaking behavior of those
   // dialogs, supplying 0 for the top inset of the row falls back to
-  // ViewsDelegate::GetRelatedControlVerticalSpacing or
-  // kRelatedControlVerticalSpacing.
-  if (!spacing)
-    spacing = ViewsDelegate::GetInstance()
-                  ? ViewsDelegate::GetInstance()
-                        ->GetDialogRelatedControlVerticalSpacing()
-                  : kRelatedControlVerticalSpacing;
-  return spacing;
+  // ViewsDelegate::GetRelatedControlVerticalSpacing.
+  // TODO(bsep): The top inset should never be 0 when harmony is enabled.
+  const int top = button_row_insets_.top() == 0
+                      ? ViewsDelegate::GetInstance()
+                            ->GetDialogRelatedControlVerticalSpacing()
+                      : button_row_insets_.top();
+  return gfx::Insets(top, button_row_insets_.left(),
+                     button_row_insets_.bottom(), button_row_insets_.right());
 }
 
 void DialogClientView::SetupFocusChain() {
   // Create a vector of child views in the order of intended focus.
   std::vector<View*> child_views;
   child_views.push_back(contents_view());
   child_views.push_back(extra_view_);
   if (kIsOkButtonOnLeftSide) {
     child_views.push_back(ok_button_);
     child_views.push_back(cancel_button_);
   } else {
     child_views.push_back(cancel_button_);
     child_views.push_back(ok_button_);
   }
 
   // Remove all null views from the vector.
   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);
 }
 
 }  // namespace views