New Sad Tab strings.

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 aeaa218893a6848b3db6169c8d8573266c9e9ed4..3ecb60fa2cb4b91f3c7668873bad7336e5750c46 100644
--- a/chrome/browser/ui/views/sad_tab_view.cc
+++ b/chrome/browser/ui/views/sad_tab_view.cc
@@ -30,6 +30,7 @@ namespace {
 constexpr int kMaxContentWidth = 600;
 constexpr int kMinColumnWidth = 120;
 constexpr int kTitleBottomSpacing = 13;
+constexpr int kBulletBottomSpacing = 3;
 
 }  // namespace
 
@@ -68,16 +69,39 @@ SadTabView::SadTabView(content::WebContents* web_contents,
                               views::kPanelVerticalSpacing);
   layout->AddView(title_, 2, 1);
 
-  message_ = new views::Label(l10n_util::GetStringUTF16(GetMessage()));
+  views::Label* message =
+      new views::Label(l10n_util::GetStringUTF16(GetMessage()));
 
-  message_->SetMultiLine(true);
-  message_->SetEnabled(false);
-  message_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  message_->SetLineHeight(views::kPanelSubVerticalSpacing);
+  message->SetMultiLine(true);
+  message->SetEnabled(false);
+  message->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  message->SetLineHeight(views::kPanelSubVerticalSpacing);
 
   layout->StartRowWithPadding(0, column_set_id, 0, kTitleBottomSpacing);
-  layout->AddView(message_, 2, 1, views::GridLayout::LEADING,
+  layout->AddView(message, 2, 1, views::GridLayout::LEADING,
                   views::GridLayout::LEADING);
+  messages_.push_back(message);
+  size_t bullet = 0;
+  int bullet_string_id = GetBulletText(bullet);
+
+  while (bullet_string_id) {
+    // TODO(wfh) add bullet here.

[sky, 2017/05/14 13:46:16]

Optional: generally chrome code has a ':' after the name, e.g. 'TODO(wfh):', but
the style guide allows the format you have here, so I'm saying optional here.

[Will Harris, 2017/05/17 18:25:19]

I'm not even sure what the TODO means, I think this is all done, so I just
removed it.

+    base::string16 bullet_string = l10n_util::GetStringUTF16(bullet_string_id);
+    bullet_string = L"     \u2022    " + bullet_string;
+    message = new views::Label(bullet_string);

[sky, 2017/05/14 13:46:16]

The way you have this now, if a line wraps the text is going to line up with the
first whitespace character you are using for padding. I would expect text
wrapping to line up the first non-bullet/white-space character of the previous
line. I recommend using a column for the bullet, then a column for the text.
This will result in wrapping like you want.

[Will Harris, 2017/05/17 18:25:19]

hmm I found however I resized the window I was not able to get this wrapping,
but agree that it could happen in theory.

The reasons why I went with the 'simple' solution (spaces) were twofold:

1. The UI spec specified the layout using spaces (five before, four after) so I
stuck to that.
2. I'm not really too familiar with views code at all, and I don't feel
confident trying to work out how to lay this out

How strongly do you feel about using a column rather than spaces? if you feel
strongly I can try and find some help from a views expert on redesigning this
with a column, but I'd probably have to run that past the user interaction
designer first (or does this get done in UI implementation review?)

Thanks

[sky, 2017/05/17 19:11:41]

I would rather see you do it correctly than kick the can down the line.
Especially given some locales may have longer strings that trigger wrapping. I
also think that while they mentioned a specific number of spacing it's the size
that they care about and they could likely tell you what size they want.

+
+    message->SetMultiLine(true);
+    message->SetEnabled(false);
+    message->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    message->SetLineHeight(views::kPanelSubVerticalSpacing);
+
+    layout->StartRowWithPadding(0, column_set_id, 0, kBulletBottomSpacing);
+    layout->AddView(message, 2, 1, views::GridLayout::LEADING,
+                    views::GridLayout::LEADING);
+    messages_.push_back(message);
+    bullet++;
+    bullet_string_id = GetBulletText(bullet);
+  }
 
   action_button_ = views::MdTextButton::CreateSecondaryUiBlueButton(
       this, l10n_util::GetStringUTF16(GetButtonTitle()));
@@ -131,7 +155,9 @@ void SadTabView::Layout() {
   // Specify the maximum message width explicitly.
   const int max_width =
       std::min(width() - views::kPanelSubVerticalSpacing * 2, kMaxContentWidth);
-  message_->SizeToFit(max_width);
+  for (auto* message : messages_) {

[sky, 2017/05/14 13:46:16]

no {}.
optional: auto* -> view::Label* for clarity.

[Will Harris, 2017/05/17 18:25:19]

Done.

+    message->SizeToFit(max_width);
+  }
   title_->SizeToFit(max_width);
 
   View::Layout();