Improve session crashed bubble appearance/behavior.

* Sets dialog margins correctly.
* Sets body text context correctly.
* Replaces Grid with Box since no one likes GridLayout; this fixes
  bug 1119305 as a side effect since BoxLayout lays out StyledLabel at
  times/widths more like what it wants.
* Uses "v* = AddChildView(std::make_unique<V>(...));" idiom.
* Lays out the "checkbox" contents more like a real checkbox: with the
  border around the outside of everything.  This removes the need for a
  label vertical alignment hack and corrects the excessive spacing
  between checkbox and label.
* Makes Checkbox overrides of public methods public; this was necessary
  anyway to do the above.

Bug: 1119305
Change-Id: Ia7afa6ffeaeeab9ad407acbacaacc937f1fb842d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2377066
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802255}

chrome/browser/ui/views/session_crashed_bubble_view.cc

@@ -41,12 +41,12 @@
 #include "ui/base/buildflags.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/views/controls/button/checkbox.h"
+#include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/controls/styled_label.h"
 #include "ui/views/layout/box_layout.h"
-#include "ui/views/layout/grid_layout.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
@@ -157,6 +157,13 @@ void SessionCrashedBubbleView::Show(
     RecordBubbleHistogramValue(SESSION_CRASHED_BUBBLE_ALREADY_UMA_OPTIN);
 }
 
+gfx::Size SessionCrashedBubbleView::CalculatePreferredSize() const {
+  const int width = ChromeLayoutProvider::Get()->GetDistanceMetric(
+                        DISTANCE_BUBBLE_PREFERRED_WIDTH) -
+                    margins().width();
+  return gfx::Size(width, GetHeightForWidth(width));
+}
+
 ax::mojom::Role SessionCrashedBubbleView::GetAccessibleWindowRole() {
   return ax::mojom::Role::kAlertDialog;
 }
@@ -174,6 +181,9 @@ SessionCrashedBubbleView::SessionCrashedBubbleView(views::View* anchor_view,
   SetShowCloseButton(true);
   SetTitle(l10n_util::GetStringUTF16(IDS_SESSION_CRASHED_BUBBLE_TITLE));
 
+  set_margins(ChromeLayoutProvider::Get()->GetDialogInsetsForContentType(
+      views::TEXT, offer_uma_optin_ ? views::CONTROL : views::TEXT));
+
   // Allow unit tests to leave out Browser.
   const SessionStartupPref session_startup_pref =
       browser_ ? SessionStartupPref::GetStartupPref(browser_->profile())
@@ -218,14 +228,10 @@ void SessionCrashedBubbleView::Init() {
 
   // Description text label.
   auto text_label = std::make_unique<views::Label>(
-      l10n_util::GetStringUTF16(IDS_SESSION_CRASHED_VIEW_MESSAGE));
+      l10n_util::GetStringUTF16(IDS_SESSION_CRASHED_VIEW_MESSAGE),
+      CONTEXT_BODY_TEXT_LARGE);
   text_label->SetMultiLine(true);
-  text_label->SetLineHeight(20);
   text_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  text_label->SizeToFit(
-      provider->GetDistanceMetric(
-          ChromeDistanceMetric::DISTANCE_BUBBLE_PREFERRED_WIDTH) -
-      margins().width());
   AddChildView(std::move(text_label));
 
   if (offer_uma_optin_)
@@ -235,15 +241,37 @@ void SessionCrashedBubbleView::Init() {
 std::unique_ptr<views::View> SessionCrashedBubbleView::CreateUmaOptInView() {
   RecordBubbleHistogramValue(SESSION_CRASHED_BUBBLE_OPTIN_BAR_SHOWN);
 
+  // Create a view that will function like a views::Checkbox, but with a
+  // StyledLabel instead of the normal Label.
+  auto uma_view = std::make_unique<views::View>();
+  auto* uma_layout =
+      uma_view->SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
+          ChromeLayoutProvider::Get()->GetDistanceMetric(
+              views::DISTANCE_RELATED_LABEL_HORIZONTAL)));
+  uma_layout->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kStart);
+
+  // The checkbox itself.
+  uma_option_ = uma_view->AddChildView(
+      std::make_unique<views::Checkbox>(base::string16()));
+  uma_option_->SetChecked(false);
+
+  // Move the checkbox border up to |uma_view|.
+  uma_view->SetBorder(uma_option_->CreateDefaultBorder());
+  uma_option_->SetBorder(nullptr);
+
   // The text to the right of the checkbox.
   size_t offset;
   base::string16 link_text =
       l10n_util::GetStringUTF16(IDS_SESSION_CRASHED_BUBBLE_UMA_LINK_TEXT);
-  auto uma_label = std::make_unique<views::StyledLabel>(this);
   base::string16 uma_text = l10n_util::GetStringFUTF16(
       IDS_SESSION_CRASHED_VIEW_UMA_OPTIN,
       link_text,
       &offset);
+
+  auto* uma_label =
+      uma_view->AddChildView(std::make_unique<views::StyledLabel>(this));
   uma_label->SetText(uma_text);
   uma_label->AddStyleRange(gfx::Range(offset, offset + link_text.length()),
                            views::StyledLabel::RangeStyleInfo::CreateForLink());
@@ -255,33 +283,8 @@ std::unique_ptr<views::View> SessionCrashedBubbleView::CreateUmaOptInView() {
   gfx::Range after_link_range(offset + link_text.length(), uma_text.length());
   if (!after_link_range.is_empty())
     uma_label->AddStyleRange(after_link_range, uma_style);
-  // Shift the text down by 3px to align with the checkbox.
-  uma_label->SetBorder(views::CreateEmptyBorder(3, 0, 0, 0));
 
-  // Checkbox for metric reporting setting.
-  auto uma_option = std::make_unique<views::Checkbox>(base::string16());
-  uma_option->SetChecked(false);
-  uma_option->SetAssociatedLabel(uma_label.get());
-
-  // Create a view to hold the checkbox and the text.
-  auto uma_view = std::make_unique<views::View>();
-  views::GridLayout* uma_layout =
-      uma_view->SetLayoutManager(std::make_unique<views::GridLayout>());
-
-  const int kReportColumnSetId = 0;
-  views::ColumnSet* cs = uma_layout->AddColumnSet(kReportColumnSetId);
-  cs->AddColumn(views::GridLayout::CENTER, views::GridLayout::LEADING,
-                views::GridLayout::kFixedSize,
-                views::GridLayout::ColumnSize::kUsePreferred, 0, 0);
-  cs->AddPaddingColumn(views::GridLayout::kFixedSize,
-                       ChromeLayoutProvider::Get()->GetDistanceMetric(
-                           views::DISTANCE_RELATED_LABEL_HORIZONTAL));
-  cs->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1.0,
-                views::GridLayout::ColumnSize::kFixed, 0, 0);
-
-  uma_layout->StartRow(views::GridLayout::kFixedSize, kReportColumnSetId);
-  uma_option_ = uma_layout->AddView(std::move(uma_option));
-  uma_layout->AddView(std::move(uma_label));
+  uma_option_->SetAssociatedLabel(uma_label);
 
   return uma_view;
 }

chrome/browser/ui/views/session_crashed_bubble_view.h

@@ -42,6 +42,7 @@ class SessionCrashedBubbleView : public SessionCrashedBubble,
 
  protected:
   // views::BubbleDialogDelegateView:
+  gfx::Size CalculatePreferredSize() const override;
   ax::mojom::Role GetAccessibleWindowRole() override;
 
  private:

ui/views/controls/button/checkbox.cc

@@ -132,6 +132,24 @@ void Checkbox::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   }
 }
 
+gfx::ImageSkia Checkbox::GetImage(ButtonState for_state) const {
+  int icon_state = 0;
+  if (GetChecked())
+    icon_state |= IconState::CHECKED;
+  if (for_state != STATE_DISABLED)
+    icon_state |= IconState::ENABLED;
+  return gfx::CreateVectorIcon(GetVectorIcon(), 16,
+                               GetIconImageColor(icon_state));
+}
+
+std::unique_ptr<LabelButtonBorder> Checkbox::CreateDefaultBorder() const {
+  std::unique_ptr<LabelButtonBorder> border =
+      LabelButton::CreateDefaultBorder();
+  border->set_insets(
+      LayoutProvider::Get()->GetInsetsMetric(INSETS_CHECKBOX_RADIO_BUTTON));
+  return border;
+}
+
 void Checkbox::OnThemeChanged() {
   LabelButton::OnThemeChanged();
   UpdateImage();
@@ -156,24 +174,6 @@ SkColor Checkbox::GetInkDropBaseColor() const {
   return GetIconImageColor(IconState::ENABLED);
 }
 
-gfx::ImageSkia Checkbox::GetImage(ButtonState for_state) const {
-  int icon_state = 0;
-  if (GetChecked())
-    icon_state |= IconState::CHECKED;
-  if (for_state != STATE_DISABLED)
-    icon_state |= IconState::ENABLED;
-  return gfx::CreateVectorIcon(GetVectorIcon(), 16,
-                               GetIconImageColor(icon_state));
-}
-
-std::unique_ptr<LabelButtonBorder> Checkbox::CreateDefaultBorder() const {
-  std::unique_ptr<LabelButtonBorder> border =
-      LabelButton::CreateDefaultBorder();
-  border->set_insets(
-      LayoutProvider::Get()->GetInsetsMetric(INSETS_CHECKBOX_RADIO_BUTTON));
-  return border;
-}
-
 SkPath Checkbox::GetFocusRingPath() const {
   SkPath path;
   gfx::Rect bounds = image()->GetMirroredBounds();

ui/views/controls/button/checkbox.h

@@ -50,6 +50,8 @@ class VIEWS_EXPORT Checkbox : public LabelButton {
 
   // LabelButton:
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
+  gfx::ImageSkia GetImage(ButtonState for_state) const override;
+  std::unique_ptr<LabelButtonBorder> CreateDefaultBorder() const override;
 
  protected:
   // Bitmask constants for GetIconImageColor.
@@ -60,8 +62,6 @@ class VIEWS_EXPORT Checkbox : public LabelButton {
   std::unique_ptr<InkDrop> CreateInkDrop() override;
   std::unique_ptr<InkDropRipple> CreateInkDropRipple() const override;
   SkColor GetInkDropBaseColor() const override;
-  gfx::ImageSkia GetImage(ButtonState for_state) const override;
-  std::unique_ptr<LabelButtonBorder> CreateDefaultBorder() const override;
 
   // Returns the path to draw the focus ring around for this Checkbox.
   virtual SkPath GetFocusRingPath() const;