New Sad Tab strings.

chrome/browser/ui/views/sad_tab_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/sad_tab_view.h"
 
 #include <string>
 
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
@@ skipping 12 matching lines @@
 #include "ui/views/controls/link.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
 constexpr int kMaxContentWidth = 600;
 constexpr int kMinColumnWidth = 120;
 constexpr int kTitleBottomSpacing = 13;
+constexpr int kBulletBottomSpacing = 3;
 
 }  // namespace
 
 SadTabView::SadTabView(content::WebContents* web_contents,
                        chrome::SadTabKind kind)
     : SadTab(web_contents, kind) {
   set_background(views::Background::CreateThemedSolidBackground(
       this, ui::NativeTheme::kColorId_DialogBackground));
 
   views::GridLayout* layout = new views::GridLayout(this);
@@ skipping 18 matching lines @@
 
   title_ = new views::Label(l10n_util::GetStringUTF16(GetTitle()));
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   title_->SetFontList(rb.GetFontList(ui::ResourceBundle::LargeFont));
   title_->SetMultiLine(true);
   title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   layout->StartRowWithPadding(0, column_set_id, 0,
                               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()));
   help_link_ = new views::Link(l10n_util::GetStringUTF16(GetHelpLinkTitle()));
   help_link_->set_listener(this);
   layout->StartRowWithPadding(0, column_set_id, 0,
                               views::kPanelVerticalSpacing);
   layout->AddView(help_link_, 1, 1, views::GridLayout::LEADING,
                   views::GridLayout::CENTER);
   layout->AddView(action_button_, 1, 1, views::GridLayout::TRAILING,
@@ skipping 33 matching lines @@
 void SadTabView::ButtonPressed(views::Button* sender,
                                const ui::Event& event) {
   DCHECK_EQ(action_button_, sender);
   PerformAction(Action::BUTTON);
 }
 
 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();
 }
 
 void SadTabView::OnPaint(gfx::Canvas* canvas) {
   if (!painted_) {
     RecordFirstPaint();
     painted_ = true;
   }
   View::OnPaint(canvas);
 }
 
 namespace chrome {
 
 SadTab* SadTab::Create(content::WebContents* web_contents,
                        SadTabKind kind) {
   return new SadTabView(web_contents, kind);
 }
 
 }  // namespace chrome