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"
@@ skipping 24 matching lines @@
 const bool kIsOkButtonOnLeftSide = false;
 #endif
 
 // Returns true if the given view should be shown (i.e. exists and is
 // visible).
 bool ShouldShow(View* view) {
   return view && view->visible();
 }
 
 // Do the layout for a button.
-void LayoutButton(LabelButton* button,
-                  gfx::Rect* row_bounds,
-                  int button_height) {
+void LayoutButton(View* button, gfx::Rect* row_bounds, int button_height) {
   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);
@@ skipping 16 matching lines @@
       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())

[Bret, 2017/02/01 00:05:30]

This is redundant with the first line of the function.

-    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 74 matching lines @@
           (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),
       0);
 
-  int buttons_height = GetButtonsAndExtraViewRowHeight();
-  if (buttons_height != 0) {
-    size.Enlarge(0, buttons_height + GetButtonsAndExtraViewRowTopPadding());
+  int buttons_row_height = GetButtonsAndExtraViewRowHeight();
+  if (buttons_row_height != 0) {
+    // Harmony does not use the extra padding.
+    if (!ui::MaterialDesignController::IsSecondaryUiMaterial())
+      buttons_row_height += GetButtonsAndExtraViewRowTopPadding();
+    size.Enlarge(0, buttons_row_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 24 matching lines @@
     }
     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();
         row_bounds.set_width(row_bounds.width() - custom_padding);
       }
-      row_bounds.set_width(std::min(row_bounds.width(),
-          extra_view_->GetPreferredSize().width()));
-      extra_view_->SetBoundsRect(row_bounds);
+      if (GetDialogDelegate()->GroupExtraViewWithButtons()) {
+        LayoutButton(extra_view_, &row_bounds, button_height);
+      } else {
+        row_bounds.set_width(std::min(row_bounds.width(),
+                                      extra_view_->GetPreferredSize().width()));
+        extra_view_->SetBoundsRect(row_bounds);
+      }
     }
 
     if (height > 0)
       bounds.Inset(0, 0, 0, height + GetButtonsAndExtraViewRowTopPadding());
   }
 
   // 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(),
@@ skipping 98 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));
+  const int minimum_button_width =
+      ViewsDelegate::GetInstance()
+          ? ViewsDelegate::GetInstance()->GetDialogButtonMinimumWidth()

[tapted, 2017/02/01 00:52:48]

I feel like this isn't the best place to bake in the minimum width.. (i.e. it's
easy for custom dialogs under src/chrome to "forget" about this) E.g. there is
`kMinWidth = 48` in md_text_button.cc, which is used when
MdTextButton::CreateSecondaryUIButton detects harmony and uses an MdTextButton*
rather than a BlueButton* or LabelButton*

Are there use cases where we have an MdTextButton* that is not a "dialog"
button? (can we just override the default min width there?)

[Bret, 2017/02/01 02:21:46]

Oh you're right, that's odd. I was told to make the minimum width 70 (in
crbug.com/652507#c4) so I'm not sure what the right answer is. Maybe Peter can
shed some light? If not I'll have to ask a designer. But either way you're right
it should probably go into md_text_button. I'll update and retest this when I
have the right value.

[Peter Kasting, 2017/02/02 00:32:30]

The spec I have (
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
) says buttons are minimum 64 DIP wide (16 DIP "boundary" on each end, and 32
DIP minimum for text), but can be wider if the text is wider than 32 DIP.

I'm not sure where Jayson got the 70 value.  I don't have a specific mock for
this dialog in the tracking spreadsheet (the mock linked in comment 0 of the bug
is out of date).  Separately, note that the button set here probably is affected
by https://bugs.chromium.org/p/chromium/issues/detail?id=671820 , but I imagine
that will get fixed separately.

The 48 value in md_text_button.cc comes from
https://chromium.googlesource.com/chromium/src/+/0faeac167d0af1df458fd5edf988...
, which was for https://bugs.chromium.org/p/chromium/issues/detail?id=571500 ,
which was working off the spec in comment 0 of that bug, which is from a very
early version of secondary UI MD.  I think that should be replaced with the 64
value here (and probably not hardcoded into that file).

Maybe the ViewsDelegate method should be more like
"GetLabelButtonMinimumWidth()" instead of "GetDialogButtonMinimumWidth()" and be
applied inside all LabelButtons?

[Bret, 2017/02/03 01:47:17]

I think I understand. I changed the minimum width to 64 and the wait button is
still 70px wide, so the string "Wait" is just long enough to be above minimum. I
guess Jayson was saying that the buttons should be the same size in that case.

Since that logic is in progress elsewhere I just changed the minimum to 64px in
md_text_button. I left it hardcoded because it's not actually needed anywhere
else yet; I'd rather wait for the plumbing to become necessary to refactor.

That also means the GetDialogButtonMinimumWidth method I added isn't necessary
any more either.

+          : kDialogMinimumButtonWidth;
+  button->SetMinSize(gfx::Size(minimum_button_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);
 }
 
@@ skipping 43 matching lines @@
       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