Sad tab redesign for Windows, Linux and CrOS

chrome/browser/ui/views/sad_tab_view.cc

Index: chrome/browser/ui/views/sad_tab_view.cc
diff --git a/chrome/browser/ui/views/sad_tab_view.cc b/chrome/browser/ui/views/sad_tab_view.cc
index 173455ecb87ba8aed454c7f4ceb5db9b65843fd6..5b89180d729d356d047e6891266c8585ec0ddf9d 100644
--- a/chrome/browser/ui/views/sad_tab_view.cc
+++ b/chrome/browser/ui/views/sad_tab_view.cc
@@ -9,6 +9,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"

[msw, 2015/05/14 23:50:20]

Is this needed?

[edwardjung, 2015/05/18 12:02:36]

Removed

 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/chrome_pages.h"
@@ -22,11 +23,14 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/background.h"
+#include "ui/views/controls/button/blue_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
+#include "ui/views/controls/scroll_view.h"

[msw, 2015/05/14 23:50:20]

Is this needed?

[edwardjung, 2015/05/18 12:02:36]

Removed. Had experimented with having a scroll view for the content area.

+#include "ui/views/controls/styled_label.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/widget/widget.h"
 
@@ -35,11 +39,16 @@ using content::WebContents;
 
 namespace {
 
-const int kPadding = 20;
-const float kMessageSize = 0.65f;
-const SkColor kTextColor = SK_ColorWHITE;
-const SkColor kCrashColor = SkColorSetRGB(35, 48, 64);
-const SkColor kKillColor = SkColorSetRGB(57, 48, 88);
+const int kPadding = 24;

[msw, 2015/05/14 23:50:20]

Find corresponding (or close enough) ui/views/layout/layout_constants.h values.
Magic numbers are generally discouraged.

[edwardjung, 2015/05/18 12:02:35]

Switched to the layout constants.

+const int kParagraphPadding = 16;
+const int kTopPadding = 50;
+const int kIconBottomMargin = 40;
+const int kLineHeight = 24;
+const int kMaxContentWidth = 600;
+const int kMinColumnWidth = 120;
+const SkColor kTextColor = SkColorSetRGB(100, 100, 100);

[msw, 2015/05/14 23:50:20]

Let's see a screenshot with these colors versus my suggested themed text and
window colors. I suggest using the default label color instead of kTitleColor,
using the default color or maybe kColorId_LabelDisabledColor instead of
kTextColor, and using kColorId_DialogBackground instead of kBackgroundColor.

+const SkColor kTitleColor = SkColorSetRGB(51, 51, 51);
+const SkColor kBackgroundColor = SkColorSetRGB(247, 247, 247);
 
 const char kCategoryTagCrash[] = "Crash";
 
@@ -52,7 +61,8 @@ SadTabView::SadTabView(WebContents* web_contents, chrome::SadTabKind kind)
       message_(NULL),
       help_link_(NULL),
       feedback_link_(NULL),
-      reload_button_(NULL) {
+      reload_button_(NULL),
+      title_(NULL) {

[msw, 2015/05/14 23:50:19]

Init the other new members too and use nullptr instead throughout.

[edwardjung, 2015/05/18 12:02:35]

Done.

   DCHECK(web_contents);
 
   // Sometimes the user will never see this tab, so keep track of the total
@@ -84,9 +94,11 @@ SadTabView::SadTabView(WebContents* web_contents, chrome::SadTabKind kind)
       NOTREACHED();
   }
 
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  gfx::FontList body_font_list_ = rb.GetFontList(ui::ResourceBundle::BaseFont);

[msw, 2015/05/14 23:50:20]

Don't use the underscore postfix for stack locals.
This probably isn't needed at all, BaseFont is the default.

[edwardjung, 2015/05/18 12:02:36]

Removed.

+
   // Set the background color.
-  set_background(views::Background::CreateSolidBackground(
-      (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? kCrashColor : kKillColor));
+  set_background(views::Background::CreateSolidBackground(kBackgroundColor));
 
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
@@ -94,79 +106,110 @@ SadTabView::SadTabView(WebContents* web_contents, chrome::SadTabKind kind)
   const int column_set_id = 0;
   views::ColumnSet* columns = layout->AddColumnSet(column_set_id);
   columns->AddPaddingColumn(1, kPadding);
-  columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER,
-                     0, views::GridLayout::USE_PREF, 0, 0);
+  columns->AddColumn(views::GridLayout::LEADING, views::GridLayout::LEADING, 0,
+                     views::GridLayout::USE_PREF, 0, kMinColumnWidth);
+  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::LEADING, 0,
+                     views::GridLayout::USE_PREF, 0, kMinColumnWidth);
   columns->AddPaddingColumn(1, kPadding);
 
   views::ImageView* image = new views::ImageView();
-  image->SetImage(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-      (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDR_SAD_TAB : IDR_KILLED_TAB));
-  layout->StartRowWithPadding(0, column_set_id, 1, kPadding);
-  layout->AddView(image);
-
-  views::Label* title = CreateLabel(l10n_util::GetStringUTF16(
-      (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
-          IDS_SAD_TAB_TITLE : IDS_KILLED_TAB_TITLE));
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  title->SetFontList(rb.GetFontList(ui::ResourceBundle::MediumFont));
-  layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-  layout->AddView(title);
+  image->SetImage(
+      ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(IDR_SAD_TAB));
+  layout->AddPaddingRow(1, kTopPadding);
+  layout->StartRow(0, column_set_id);
+  layout->AddView(image, 2, 1);
+
+  title_ = CreateLabel(l10n_util::GetStringUTF16(
+      (kind_ == chrome::SAD_TAB_KIND_CRASHED) ? IDS_SAD_TAB_TITLE

[msw, 2015/05/14 23:50:20]

nit: maybe cache a local const bool crashed for here and elsewhere.

[edwardjung, 2015/05/18 12:02:35]

Done.

+                                              : IDS_KILLED_TAB_TITLE));
+  title_->SetFontList(rb.GetFontList(ui::ResourceBundle::LargeFont));
+  title_->SetMultiLine(true);

[msw, 2015/05/14 23:50:20]

I wonder if "Aw Snap" or "He's Dead Jim" really need to wrap.

[edwardjung, 2015/05/18 12:02:36]

In my testing of Russian and Arabic, He's Dead Jim is pretty long and would need
wrapping at the narrowest window width.

[msw, 2015/05/18 19:24:50]

Acknowledged.

+  title_->SetEnabledColor(kTitleColor);
+  title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  layout->StartRowWithPadding(0, column_set_id, 0, kIconBottomMargin);
+  layout->AddView(title_, 2, 1);
 
   message_ = CreateLabel(l10n_util::GetStringUTF16(
       (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
           IDS_SAD_TAB_MESSAGE : IDS_KILLED_TAB_MESSAGE));
+  message_->SetFontList(body_font_list_);
   message_->SetMultiLine(true);
-  layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-  layout->AddView(message_);
+  message_->SetEnabledColor(kTextColor);
+  message_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  message_->SetLineHeight(kLineHeight);
+
+  layout->StartRowWithPadding(0, column_set_id, 0, kParagraphPadding);
+  layout->AddView(message_, 2, 1, views::GridLayout::LEADING,
+                  views::GridLayout::LEADING);
 
   if (web_contents_) {
-    layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-    reload_button_ = new views::LabelButton(
-        this,
-        l10n_util::GetStringUTF16(IDS_SAD_TAB_RELOAD_LABEL));
-    reload_button_->SetStyle(views::Button::STYLE_BUTTON);
-    // Always render the reload button with chrome style borders; never rely on
-    // native styles.
-    reload_button_->SetBorder(scoped_ptr<views::Border>(
-        new views::LabelButtonBorder(reload_button_->style())));
-    layout->AddView(reload_button_);
-
-    help_link_ = CreateLink(l10n_util::GetStringUTF16(
-        (kind_ == chrome::SAD_TAB_KIND_CRASHED) ?
-            IDS_SAD_TAB_HELP_LINK : IDS_LEARN_MORE));
+    reload_button_ = new views::BlueButton(
+        this, l10n_util::GetStringUTF16(IDS_SAD_TAB_RELOAD_LABEL));
+    reload_button_->SetFontList(body_font_list_);
+    reload_button_->SetMinSize(gfx::Size(90, 36));

[msw, 2015/05/14 23:50:20]

What does the button look like if this isn't specified?

[edwardjung, 2015/05/18 12:02:36]

Looks fine. Remove.

 
     if (kind_ == chrome::SAD_TAB_KIND_CRASHED) {
       size_t offset = 0;
       base::string16 help_text(
           l10n_util::GetStringFUTF16(IDS_SAD_TAB_HELP_MESSAGE,
                                      base::string16(), &offset));
-      views::Label* help_prefix = CreateLabel(help_text.substr(0, offset));
-      views::Label* help_suffix = CreateLabel(help_text.substr(offset));
-
-      const int help_column_set_id = 1;
-      views::ColumnSet* help_columns = layout->AddColumnSet(help_column_set_id);
-      help_columns->AddPaddingColumn(1, kPadding);
-      // Center three middle columns for the help's [prefix][link][suffix].
-      for (size_t column = 0; column < 3; column++)
-        help_columns->AddColumn(views::GridLayout::CENTER,
-            views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
-      help_columns->AddPaddingColumn(1, kPadding);
-
-      layout->StartRowWithPadding(0, help_column_set_id, 0, kPadding);
-      layout->AddView(help_prefix);
-      layout->AddView(help_link_);
-      layout->AddView(help_suffix);
+
+      base::string16 link_text =
+          l10n_util::GetStringUTF16(IDS_SAD_TAB_HELP_LINK);
+
+      base::string16 help_prefix = help_text.substr(0, offset);
+      base::string16 help_suffix = help_text.substr(offset);
+      base::string16 help_message_string = help_prefix;
+      help_message_string.append(link_text).append(help_suffix);
+
+      help_message_ = new views::StyledLabel(help_message_string, this);
+
+      views::StyledLabel::RangeStyleInfo link_style =
+          views::StyledLabel::RangeStyleInfo::CreateForLink();
+      link_style.font_style = gfx::Font::UNDERLINE;
+      link_style.color = kTextColor;
+
+      views::StyledLabel::RangeStyleInfo normal_style =
+          views::StyledLabel::RangeStyleInfo::CreateForLink();

[msw, 2015/05/14 23:50:20]

This doesn't seem correct, why use CreateForLink for the non-link default style?

[edwardjung, 2015/05/18 12:02:35]

Fixed.

+      normal_style.font_style = gfx::Font::NORMAL;
+      normal_style.color = kTextColor;
+      normal_style.is_link = false;
+
+      help_message_->SetBaseFontList(body_font_list_);
+      help_message_->SetDefaultStyle(normal_style);
+      help_message_->SetLineHeight(kLineHeight);
+
+      help_message_->AddStyleRange(
+          gfx::Range(help_prefix.length(), help_message_string.length() - 1),

[msw, 2015/05/14 23:50:19]

Using |help_message_string.length() - 1| is incorrect. I think this assumes the
untranslated/unlocalized text, which just ends in a period. This should be
|help_prefix.length() + link_text.length()|.

[edwardjung, 2015/05/18 12:02:35]

Good point. Not all languages will have a punctuation at the end.

+          link_style);
+
+      layout->StartRowWithPadding(0, column_set_id, 0, kParagraphPadding);
+      layout->AddView(help_message_, 2, 1, views::GridLayout::LEADING,
+                      views::GridLayout::TRAILING);
+      layout->StartRowWithPadding(0, column_set_id, 0, kTopPadding);
+      layout->SkipColumns(1);
     } else {
-      layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-      layout->AddView(help_link_);
+      feedback_link_ =
+          CreateLink(l10n_util::GetStringUTF16(IDS_KILLED_TAB_FEEDBACK_LINK));
+      feedback_link_->SetFontList(body_font_list_);
+      feedback_link_->SetLineHeight(kLineHeight);
+      feedback_link_->SetMultiLine(true);
+      feedback_link_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
 
-      feedback_link_ = CreateLink(
-          l10n_util::GetStringUTF16(IDS_KILLED_TAB_FEEDBACK_LINK));
       layout->StartRowWithPadding(0, column_set_id, 0, kPadding);
-      layout->AddView(feedback_link_);
+      layout->AddView(feedback_link_, 2, 1, views::GridLayout::LEADING,
+                      views::GridLayout::LEADING);
+
+      help_link_ = CreateLink(l10n_util::GetStringUTF16(IDS_LEARN_MORE));
+      help_link_->SetFontList(body_font_list_);
+      layout->StartRowWithPadding(0, column_set_id, 0, kTopPadding);
+      layout->AddView(help_link_, 1, 1, views::GridLayout::LEADING,
+                      views::GridLayout::CENTER);
     }
+    layout->AddView(reload_button_, 1, 1, views::GridLayout::TRAILING,
+                    views::GridLayout::LEADING);
   }
-  layout->AddPaddingRow(1, kPadding);
+  layout->AddPaddingRow(5, kPadding);
 }
 
 SadTabView::~SadTabView() {}
@@ -188,6 +231,14 @@ void SadTabView::LinkClicked(views::Link* source, int event_flags) {
   }
 }
 
+void SadTabView::StyledLabelLinkClicked(const gfx::Range& range,
+                                        int event_flags) {
+  GURL help_url(chrome::kCrashReasonURL);

[msw, 2015/05/14 23:50:19]

As I said in my earlier comment... Don't copy code, refactor to share the code,
or call LinkClicked(help_link_)

[edwardjung, 2015/05/18 12:02:35]

Switched to call LinkClicked.

+  OpenURLParams params(help_url, content::Referrer(), CURRENT_TAB,
+                       ui::PAGE_TRANSITION_LINK, false);
+  web_contents_->OpenURL(params);
+}
+
 void SadTabView::ButtonPressed(views::Button* sender,
                                const ui::Event& event) {
   DCHECK(web_contents_);
@@ -197,7 +248,16 @@ void SadTabView::ButtonPressed(views::Button* sender,
 
 void SadTabView::Layout() {
   // Specify the maximum message width explicitly.
-  message_->SizeToFit(static_cast<int>(width() * kMessageSize));
+  int messageWidth = width() - kPadding * 2;

[msw, 2015/05/14 23:50:20]

nit: use the unix_hacker naming convention.

[edwardjung, 2015/05/18 12:02:35]

Done.

+  int width = messageWidth > kMaxContentWidth ? kMaxContentWidth : messageWidth;

[msw, 2015/05/14 23:50:19]

Just do const int width = std::min(width() - kPadding * 2, kMaxContentWidth)

[edwardjung, 2015/05/18 12:02:36]

Done.

+  message_->SizeToFit(width);
+  title_->SizeToFit(width);
+
+  if (kind_ == chrome::SAD_TAB_KIND_KILLED) {

[msw, 2015/05/14 23:50:20]

Just check if |feedback_link_| and |help_message_| are not nullptr instead of
checking for the |kind_|.

[edwardjung, 2015/05/18 12:02:36]

Done.

+    feedback_link_->SizeToFit(width);
+  } else if (kind_ == chrome::SAD_TAB_KIND_CRASHED) {
+    help_message_->SizeToFit(width);
+  }
   View::Layout();
 }