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"
 #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()
-          ? ViewsDelegate::GetInstance()->GetDialogButtonInsets()
-          : gfx::Insets(0, kButtonHEdgeMarginNew, kButtonVEdgeMarginNew,
-                        kButtonHEdgeMarginNew);
+    : ClientView(owner, contents_view),
+      button_row_insets_(
+          ViewsDelegate::GetInstance()->GetDialogButtonInsets()) {
   // 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());
+    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 23 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.

[Peter Kasting, 2017/02/16 01:14:47]

We use MdTextButton even pre-Harmony.  Seems like this could be deleted now.

[Bret, 2017/02/16 01:31:04]

I thought that too at first, but CreateSecondaryUiButton goes to LabelButton
directly if harmony is disabled. See
https://cs.chromium.org/chromium/src/ui/views/controls/button/md_text_button....

[Peter Kasting, 2017/02/16 06:22:31]

Well that seems dumb.  Honestly I don't know why we even have MdTextButton at
all instead of just making LabelButton be styled correctly for Harmony vs. not. 
Maybe I should file a bug to resolve that :/

+  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