Harmony - convert hung renderer dialog.

chrome/browser/ui/views/hung_renderer_view.cc

Index: chrome/browser/ui/views/hung_renderer_view.cc
diff --git a/chrome/browser/ui/views/hung_renderer_view.cc b/chrome/browser/ui/views/hung_renderer_view.cc
index 94bfedc9f3a3634a9b0c4a28c8ffee58191b8777..737519e3f1d1bd5ef4b2797c9981df7ea09f597d 100644
--- a/chrome/browser/ui/views/hung_renderer_view.cc
+++ b/chrome/browser/ui/views/hung_renderer_view.cc
@@ -14,6 +14,8 @@
 #include "chrome/browser/ui/chrome_web_modal_dialog_manager_delegate.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_iterator.h"
+#include "chrome/browser/ui/views/harmony/layout_delegate.h"
+#include "chrome/browser/ui/views/layout_utils.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/crash_keys.h"
 #include "chrome/common/logging_chrome.h"
@@ -34,7 +36,6 @@
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/grid_layout.h"
-#include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/client_view.h"
 
@@ -176,11 +177,6 @@ gfx::ImageSkia* HungRendererDialogView::frozen_icon_ = NULL;
 static const int kTableViewWidth = 300;
 static const int kTableViewHeight = 100;
 
-// Padding space in pixels between frozen icon to the info label, hung pages
-// list table view and the Kill pages button.
-static const int kCentralColumnPadding =
-    views::kUnrelatedControlLargeHorizontalSpacing;
-
 ///////////////////////////////////////////////////////////////////////////////
 // HungRendererDialogView, public:
 
@@ -233,11 +229,7 @@ bool HungRendererDialogView::IsFrameActive(WebContents* contents) {
 }
 
 HungRendererDialogView::HungRendererDialogView()
-    : info_label_(nullptr),
-      hung_pages_table_(nullptr),
-      kill_button_(nullptr),
-      initialized_(false),
-      kill_button_clicked_(false) {
+    : info_label_(nullptr), hung_pages_table_(nullptr), initialized_(false) {
   InitClass();
 }
 
@@ -334,54 +326,22 @@ void HungRendererDialogView::WindowClosing() {
 }
 
 int HungRendererDialogView::GetDialogButtons() const {
-  return ui::DIALOG_BUTTON_CANCEL;
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;
 }
 
 base::string16 HungRendererDialogView::GetDialogButtonLabel(
     ui::DialogButton button) const {
-  DCHECK_EQ(ui::DIALOG_BUTTON_CANCEL, button);
-  return l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_WAIT);
-}
-
-views::View* HungRendererDialogView::CreateExtraView() {
-  DCHECK(!kill_button_);
-  kill_button_ = views::MdTextButton::CreateSecondaryUiButton(this,
-      l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_END));
-  return kill_button_;
+  DCHECK_NE(button, ui::DIALOG_BUTTON_NONE);

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

Nit: (expected, actual)

But honestly I'd probably just kill this DCHECK and assume no caller is dumb
enough to pass NONE for the button, since it makes no sense.

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

Done (deleted).

+  if (button == ui::DIALOG_BUTTON_OK)
+    return l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_WAIT);
+  return l10n_util::GetStringUTF16(IDS_BROWSER_HANGMONITOR_RENDERER_END);

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

Nit: Less code duplication:

  return l10n_util::GetStringUTF16((button == ui::DIALOG_BUTTON_OK)
                                       ? IDS_BROWSER_HANGMONITOR_RENDERER_WAIT
                                       : IDS_BROWSER_HANGMONITOR_RENDERER_END);

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

Done.

 }
 
 bool HungRendererDialogView::Cancel() {
-  // Start waiting again for responsiveness.
-  if (!kill_button_clicked_ &&
-      hung_pages_table_model_->GetRenderViewHost()) {
-    hung_pages_table_model_->GetRenderViewHost()
-        ->GetWidget()
-        ->RestartHangMonitorTimeoutIfNecessary();
-  }
-  return true;
-}
-
-bool HungRendererDialogView::ShouldUseCustomFrame() const {
-#if defined(OS_WIN)
-  // Use the old dialog style without Aero glass, otherwise the dialog will be
-  // visually constrained to browser window bounds. See http://crbug.com/323278
-  return ui::win::IsAeroGlassEnabled();
-#else
-  return views::DialogDelegateView::ShouldUseCustomFrame();
-#endif
-}
-
-///////////////////////////////////////////////////////////////////////////////
-// HungRendererDialogView, views::ButtonListener implementation:
-
-void HungRendererDialogView::ButtonPressed(
-    views::Button* sender, const ui::Event& event) {
-  DCHECK_EQ(kill_button_, sender);
-  kill_button_clicked_ = true;
   content::RenderProcessHost* rph =
       hung_pages_table_model_->GetRenderProcessHost();
   if (!rph)

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

Nit: Up to you, but to me it makes sense in this case to do:

  if (rph) {
    ...
  }
  return true;

Since the semantic meaning is basically "if there's a RPH, shut it down; then,
unconditionally close this view."

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

Done.

-    return;
+    return true;
 #if defined(OS_WIN)
   base::StringPairs crash_keys;
 
@@ -403,6 +363,31 @@ void HungRendererDialogView::ButtonPressed(
 #else
   rph->Shutdown(content::RESULT_CODE_HUNG, false);
 #endif
+  return true;
+}
+
+bool HungRendererDialogView::Accept() {
+  // Start waiting again for responsiveness.
+  if (hung_pages_table_model_->GetRenderViewHost()) {

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

Nit: Factor to a temp for less duplication and shorter code:

  auto* rvh = hung_pages_table_model_->GetRenderViewHost();
  if (rvh)
    rvh->GetWidget()->RestartHangMonitorTimeoutIfNecessary();

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

Done.

+    hung_pages_table_model_->GetRenderViewHost()
+        ->GetWidget()
+        ->RestartHangMonitorTimeoutIfNecessary();
+  }
+  return true;
+}
+
+bool HungRendererDialogView::Close() {
+  return Accept();
+}
+
+bool HungRendererDialogView::ShouldUseCustomFrame() const {
+#if defined(OS_WIN)
+  // Use the old dialog style without Aero glass, otherwise the dialog will be
+  // visually constrained to browser window bounds. See http://crbug.com/323278
+  return ui::win::IsAeroGlassEnabled();
+#else
+  return views::DialogDelegateView::ShouldUseCustomFrame();
+#endif
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -443,14 +428,17 @@ void HungRendererDialogView::Init() {
   using views::GridLayout;
   using views::ColumnSet;
 
-  GridLayout* layout = GridLayout::CreatePanel(this);
-  SetLayoutManager(layout);
+  GridLayout* layout = layout_utils::CreatePanelLayout(this);
+  LayoutDelegate* delegate = LayoutDelegate::Get();
 
   const int double_column_set_id = 0;
   ColumnSet* column_set = layout->AddColumnSet(double_column_set_id);
   column_set->AddColumn(GridLayout::LEADING, GridLayout::LEADING, 0,
                         GridLayout::FIXED, frozen_icon_->width(), 0);
-  column_set->AddPaddingColumn(0, kCentralColumnPadding);
+  column_set->AddPaddingColumn(
+      0,
+      delegate->GetMetric(
+          LayoutDelegate::Metric::UNRELATED_CONTROL_HORIZONTAL_SPACING_LARGE));
   column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, 1,
                         GridLayout::USE_PREF, 0, 0);
 
@@ -461,7 +449,9 @@ void HungRendererDialogView::Init() {
   layout->AddView(
       info_label_, 1, 1, GridLayout::FILL, GridLayout::LEADING, 1, 0);
 
-  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+  layout->AddPaddingRow(
+      0, delegate->GetMetric(
+             LayoutDelegate::Metric::RELATED_CONTROL_VERTICAL_SPACING));
 
   layout->StartRow(0, double_column_set_id);
   layout->SkipColumns(1);