Harmony - convert hung renderer dialog.

ui/views/window/dialog_client_view.cc

Index: ui/views/window/dialog_client_view.cc
diff --git a/ui/views/window/dialog_client_view.cc b/ui/views/window/dialog_client_view.cc
index ee88f8654e741d8370cbb236fb3f1f24aeef8efb..d68730d56f08772b49346458611710adfd7c480e 100644
--- a/ui/views/window/dialog_client_view.cc
+++ b/ui/views/window/dialog_client_view.cc
@@ -42,9 +42,7 @@ bool ShouldShow(View* view) {
 }
 
 // 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;
 
@@ -81,9 +79,6 @@ DialogClientView::DialogClientView(Widget* owner, View* contents_view)
   // 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() {
@@ -178,9 +173,12 @@ gfx::Size DialogClientView::GetPreferredSize() const {
                                                        : 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());
@@ -225,9 +223,13 @@ void DialogClientView::Layout() {
         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)
@@ -346,8 +348,11 @@ LabelButton* DialogClientView::CreateDialogButton(ui::DialogButton type) {
     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;
 }