Clean up computations of line height.

Returning the font's height is a saner default for TypographyProvider.

Having Label track the user-set line height separately from the computed
result simplifies GetLineHeight() callers and fixes at least one bug
where the caller (reasonably) assumed GetLineHeight() would always be
large enough for the font height.

StyledLabel gets similar changes, but more simplifications since what
used to be GetDefaultLineHeight() is now moved into GetLineHeight(), and
GetDefaultFontList()/GetFontListForRange() have been combined into a
sindly GetFontList().  These changes fix the layout of the
DownloadItemViews, which also assumed that GetLineHeight() was large
enough for the font height.

Bug: 1109611
TBR: antrim
Change-Id: I635a4bba3813a872b66123a5ed701bc4aebbedf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2324496Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792765}

ash/login/ui/lock_contents_view.cc

@@ -181,7 +181,7 @@ void MakeSectionBold(views::StyledLabel* label,
   auto create_style = [&](bool is_bold) {
     views::StyledLabel::RangeStyleInfo style;
     if (is_bold) {
-      style.custom_font = label->GetDefaultFontList().Derive(
+      style.custom_font = label->GetFontList().Derive(
           0, gfx::Font::FontStyle::NORMAL, gfx::Font::Weight::BOLD);
     }
     style.override_color = gfx::kGoogleGrey200;

ash/login/ui/login_expanded_public_account_view.cc

@@ -323,7 +323,7 @@ class RightPaneView : public NonAccessibleView,
     learn_more_label_ = new views::StyledLabel(text, this);
 
     views::StyledLabel::RangeStyleInfo style;
-    style.custom_font = learn_more_label_->GetDefaultFontList().Derive(
+    style.custom_font = learn_more_label_->GetFontList().Derive(
         0, gfx::Font::FontStyle::NORMAL, gfx::Font::Weight::NORMAL);
     style.override_color = SK_ColorWHITE;
     learn_more_label_->AddStyleRange(gfx::Range(0, offset), style);

ash/login/ui/login_expanded_public_account_view_unittest.cc

@@ -160,19 +160,19 @@ TEST_P(LoginExpandedPublicAccountViewTest, ShowWarningDialog) {
   LoginExpandedPublicAccountView::TestApi test_api(public_account_);
   views::StyledLabel::TestApi styled_label_test(test_api.learn_more_label());
   EXPECT_EQ(test_api.warning_dialog(), nullptr);
-  EXPECT_EQ(styled_label_test.link_targets().size(), 1U);
+  ASSERT_EQ(styled_label_test.link_targets().size(), 1U);
 
   // Tap on the learn more link.
   views::Link* link = styled_label_test.link_targets().begin()->first;
   TapOnView(link);
-  EXPECT_NE(test_api.warning_dialog(), nullptr);
+  ASSERT_NE(test_api.warning_dialog(), nullptr);
   EXPECT_TRUE(test_api.warning_dialog()->GetVisible());
 
   // When warning dialog is shown, tap outside of public account expanded view
   // should not hide it.
   TapOnView(other_view_);
   EXPECT_TRUE(public_account_->GetVisible());
-  EXPECT_NE(test_api.warning_dialog(), nullptr);
+  ASSERT_NE(test_api.warning_dialog(), nullptr);
   EXPECT_TRUE(test_api.warning_dialog()->GetVisible());
 
   // If the warning dialog is shown, escape key should close the waring dialog,
@@ -232,7 +232,7 @@ TEST_P(LoginExpandedPublicAccountViewTest, ShowLanguageAndKeyboardMenu) {
   // First language item is selected, and selected item should have focus.
   EXPECT_EQ(test_api.selected_language_item().value, kEnglishLanguageCode);
   LoginMenuView::TestApi language_test_api(test_api.language_menu_view());
-  EXPECT_EQ(2u, language_test_api.contents()->children().size());
+  ASSERT_EQ(2u, language_test_api.contents()->children().size());
   EXPECT_TRUE(language_test_api.contents()->children()[0]->HasFocus());
 
   // Select language item should close the language menu.
@@ -246,7 +246,7 @@ TEST_P(LoginExpandedPublicAccountViewTest, ShowLanguageAndKeyboardMenu) {
   // Second keyboard item is selected, and selected item should have focus.
   EXPECT_EQ(test_api.selected_keyboard_item().value, kKeyboardIdForItem2);
   LoginMenuView::TestApi keyboard_test_api(test_api.keyboard_menu_view());
-  EXPECT_EQ(2u, keyboard_test_api.contents()->children().size());
+  ASSERT_EQ(2u, keyboard_test_api.contents()->children().size());
   EXPECT_TRUE(keyboard_test_api.contents()->children()[1]->HasFocus());
 
   // Select keyboard item should close the keyboard menu.

chrome/browser/chromeos/ui/echo_dialog_view.cc

@@ -86,7 +86,7 @@ void EchoDialogView::InitForEnabledEcho(const base::string16& service_name,
   auto label = std::make_unique<views::StyledLabel>(text, nullptr);
 
   views::StyledLabel::RangeStyleInfo service_name_style;
-  gfx::FontList font_list = label->GetDefaultFontList();
+  gfx::FontList font_list = label->GetFontList();
   service_name_style.custom_font =
       font_list.DeriveWithStyle(gfx::Font::UNDERLINE);
   service_name_style.tooltip = origin;

ui/views/controls/label.cc

@@ -70,7 +70,6 @@ Label::Label(const base::string16& text,
       text_style_(text_style),
       context_menu_contents_(this) {
   Init(text, style::GetFont(text_context, text_style), directionality_mode);
-  SetLineHeight(style::GetLineHeight(text_context, text_style));
 }
 
 Label::Label(const base::string16& text, const CustomFont& font)
@@ -120,6 +119,8 @@ void Label::SetTextStyle(int style) {
     return;
 
   text_style_ = style;
+  // TODO(pkasting): Seems like potentially |full_text_|'s font list and line
+  // height should be updated here?
   UpdateColorsFromTheme();
   OnPropertyChanged(&text_style_, kPropertyEffectsPreferredSizeChanged);
 }
@@ -242,13 +243,18 @@ void Label::SetVerticalAlignment(gfx::VerticalAlignment alignment) {
 }
 
 int Label::GetLineHeight() const {
-  return full_text_->min_line_height();
+  // TODO(pkasting): If we can replace SetFontList() with context/style setter
+  // calls, we can eliminate the reference to font_list().GetHeight() here.
+  return line_height_.value_or(
+      std::max(style::GetLineHeight(text_context_, text_style_),
+               font_list().GetHeight()));
 }
 
-void Label::SetLineHeight(int height) {
-  if (GetLineHeight() == height)
+void Label::SetLineHeight(int line_height) {
+  if (line_height_ == line_height)
     return;
-  full_text_->SetMinLineHeight(height);
+  line_height_ = line_height;
+  full_text_->SetMinLineHeight(line_height);
   ClearDisplayText();
   OnPropertyChanged(&full_text_ + kLabelLineHeight,
                     kPropertyEffectsPreferredSizeChanged);
@@ -504,7 +510,7 @@ gfx::Size Label::GetMinimumSize() const {
     return gfx::Size();
 
   // Always reserve vertical space for at least one line.
-  gfx::Size size(0, std::max(font_list().GetHeight(), GetLineHeight()));
+  gfx::Size size(0, GetLineHeight());
   if (elide_behavior_ == gfx::ELIDE_HEAD ||
       elide_behavior_ == gfx::ELIDE_MIDDLE ||
       elide_behavior_ == gfx::ELIDE_TAIL ||
@@ -534,7 +540,7 @@ int Label::GetHeightForWidth(int w) const {
 
   w -= GetInsets().width();
   int height = 0;
-  int base_line_height = std::max(GetLineHeight(), font_list().GetHeight());
+  int base_line_height = GetLineHeight();
   if (!GetMultiLine() || GetText().empty() || w <= 0) {
     height = base_line_height;
   } else {
@@ -971,13 +977,14 @@ void Label::Init(const base::string16& text,
                  gfx::DirectionalityMode directionality_mode) {
   full_text_ = gfx::RenderText::CreateRenderText();
   full_text_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
-  full_text_->SetDirectionalityMode(directionality_mode);
-  // NOTE: |full_text_| should not be elided at all. This is used to keep
-  // some properties and to compute the size of the string.
-  full_text_->SetElideBehavior(gfx::NO_ELIDE);
   full_text_->SetFontList(font_list);
   full_text_->SetCursorEnabled(false);
   full_text_->SetWordWrapBehavior(gfx::TRUNCATE_LONG_WORDS);
+  full_text_->SetMinLineHeight(GetLineHeight());
+  // NOTE: |full_text_| should not be elided at all. This is used to keep
+  // some properties and to compute the size of the string.
+  full_text_->SetElideBehavior(gfx::NO_ELIDE);
+  full_text_->SetDirectionalityMode(directionality_mode);
 
   SetText(text);
 
@@ -1009,7 +1016,7 @@ void Label::MaybeBuildDisplayText() const {
 gfx::Size Label::GetTextSize() const {
   gfx::Size size;
   if (GetText().empty()) {
-    size = gfx::Size(0, std::max(GetLineHeight(), font_list().GetHeight()));
+    size = gfx::Size(0, GetLineHeight());
   } else {
     // Cancel the display rect of |full_text_|. The display rect may be
     // specified in GetHeightForWidth(), and specifying empty Rect cancels

ui/views/controls/label.h

@@ -10,6 +10,7 @@
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/render_text.h"
@@ -150,10 +151,9 @@ class VIEWS_EXPORT Label : public View,
   void SetVerticalAlignment(gfx::VerticalAlignment alignment);
 
   // Get or set the distance in pixels between baselines of multi-line text.
-  // Default is 0, indicating the distance between lines should be the standard
-  // one for the label's text, font list, and platform.
+  // Default is the height of the default font.
   int GetLineHeight() const;
-  void SetLineHeight(int height);
+  void SetLineHeight(int line_height);
 
   // Get or set if the label text can wrap on multiple lines; default is false.
   bool GetMultiLine() const;
@@ -393,6 +393,7 @@ class VIEWS_EXPORT Label : public View,
 
   const int text_context_;
   int text_style_;
+  base::Optional<int> line_height_;
 
   // An un-elided and single-line RenderText object used for preferred sizing.
   std::unique_ptr<gfx::RenderText> full_text_;

ui/views/controls/styled_label.cc

@@ -100,8 +100,9 @@ void StyledLabel::SetText(const base::string16& text) {
   OnPropertyChanged(&text_, kPropertyEffectsPreferredSizeChanged);
 }
 
-gfx::FontList StyledLabel::GetDefaultFontList() const {
-  return style::GetFont(text_context_, default_text_style_);
+gfx::FontList StyledLabel::GetFontList(const RangeStyleInfo& style_info) const {
+  return style_info.custom_font.value_or(style::GetFont(
+      text_context_, style_info.text_style.value_or(default_text_style_)));
 }
 
 void StyledLabel::AddStyleRange(const gfx::Range& range,
@@ -149,16 +150,16 @@ void StyledLabel::SetDefaultTextStyle(int text_style) {
 }
 
 int StyledLabel::GetLineHeight() const {
-  return specified_line_height_;
+  return line_height_.value_or(
+      style::GetLineHeight(text_context_, default_text_style_));
 }
 
 void StyledLabel::SetLineHeight(int line_height) {
-  if (specified_line_height_ == line_height)
+  if (line_height_ == line_height)
     return;
 
-  specified_line_height_ = line_height;
-  OnPropertyChanged(&specified_line_height_,
-                    kPropertyEffectsPreferredSizeChanged);
+  line_height_ = line_height;
+  OnPropertyChanged(&line_height_, kPropertyEffectsPreferredSizeChanged);
 }
 
 base::Optional<SkColor> StyledLabel::GetDisplayedOnBackgroundColor() const {
@@ -338,26 +339,6 @@ int StyledLabel::StartX(int excess_space) const {
                                                            : excess_space);
 }
 
-int StyledLabel::GetDefaultLineHeight() const {
-  return specified_line_height_ > 0
-             ? specified_line_height_
-             : std::max(
-                   style::GetLineHeight(text_context_, default_text_style_),
-                   GetDefaultFontList().GetHeight());
-}
-
-gfx::FontList StyledLabel::GetFontListForRange(
-    const StyleRanges::const_iterator& range) const {
-  if (range == style_ranges_.end())
-    return GetDefaultFontList();
-
-  return range->style_info.custom_font
-             ? range->style_info.custom_font.value()
-             : style::GetFont(
-                   text_context_,
-                   range->style_info.text_style.value_or(default_text_style_));
-}
-
 void StyledLabel::CalculateLayout(int width) const {
   const gfx::Insets insets = GetInsets();
   width = std::max(width, insets.width());
@@ -369,7 +350,7 @@ void StyledLabel::CalculateLayout(int width) const {
   layout_views_ = std::make_unique<LayoutViews>();
 
   const int content_width = width - insets.width();
-  const int default_line_height = GetDefaultLineHeight();
+  const int line_height = GetLineHeight();
   RangeStyleInfo default_style;
   default_style.text_style = default_text_style_;
   int max_width = 0, total_height = 0;
@@ -379,7 +360,7 @@ void StyledLabel::CalculateLayout(int width) const {
   StyleRanges::const_iterator current_range = style_ranges_.begin();
   for (base::string16 remaining_string = text_;
        content_width > 0 && !remaining_string.empty();) {
-    layout_size_info_.line_sizes.emplace_back(0, default_line_height);
+    layout_size_info_.line_sizes.emplace_back(0, line_height);
     auto& line_size = layout_size_info_.line_sizes.back();
     layout_views_->views_per_line.emplace_back();
     auto& views = layout_views_->views_per_line.back();
@@ -412,13 +393,13 @@ void StyledLabel::CalculateLayout(int width) const {
            !current_range->style_info.custom_view)) {
         const gfx::Rect chunk_bounds(line_size.width(), 0,
                                      content_width - line_size.width(),
-                                     default_line_height);
+                                     line_height);
         // If the start of the remaining text is inside a styled range, the font
         // style may differ from the base font. The font specified by the range
         // should be used when eliding text.
-        gfx::FontList text_font_list = position >= range.start()
-                                           ? GetFontListForRange(current_range)
-                                           : GetDefaultFontList();
+        gfx::FontList text_font_list =
+            GetFontList((position >= range.start()) ? current_range->style_info
+                                                    : RangeStyleInfo());
         int elide_result = gfx::ElideRectangleText(
             remaining_string, text_font_list, chunk_bounds.width(),
             chunk_bounds.height(), gfx::WRAP_LONG_WORDS, &substrings);

ui/views/controls/styled_label.h

@@ -118,9 +118,10 @@ class VIEWS_EXPORT StyledLabel : public View {
   const base::string16& GetText() const;
   void SetText(const base::string16& text);
 
-  // Returns the font list that results from the default text context and style
-  // for ranges. This can be used as the basis for a range |custom_font|.
-  gfx::FontList GetDefaultFontList() const;
+  // Returns the FontList that should be used. |style_info| is an optional
+  // argument that takes precedence over the default values.
+  gfx::FontList GetFontList(
+      const RangeStyleInfo& style_info = RangeStyleInfo()) const;
 
   // Marks the given range within |text_| with style defined by |style_info|.
   // |range| must be contained in |text_|.
@@ -206,13 +207,6 @@ class VIEWS_EXPORT StyledLabel : public View {
   // space available on that line.
   int StartX(int excess_space) const;
 
-  // Returns the default line height, based on the default style.
-  int GetDefaultLineHeight() const;
-
-  // Returns the FontList that should be used for |range|.
-  gfx::FontList GetFontListForRange(
-      const StyleRanges::const_iterator& range) const;
-
   // Sets |layout_size_info_| and |layout_views_| for the given |width|.  No-op
   // if current_width <= width <= max_width, where:
   //   current_width = layout_size_info_.total_size.width()
@@ -239,8 +233,7 @@ class VIEWS_EXPORT StyledLabel : public View {
   int text_context_ = style::CONTEXT_LABEL;
   int default_text_style_ = style::STYLE_PRIMARY;
 
-  // Line height. If zero, style::GetLineHeight() is used.
-  int specified_line_height_ = 0;
+  base::Optional<int> line_height_;
 
   // The listener that will be informed of link clicks.
   StyledLabelListener* listener_;

ui/views/controls/styled_label_unittest.cc

@@ -285,7 +285,7 @@ TEST_F(StyledLabelTest, StyledRangeCustomFontUnderlined) {
   StyledLabel::RangeStyleInfo style_info;
   style_info.tooltip = ASCIIToUTF16("tooltip");
   style_info.custom_font =
-      styled()->GetDefaultFontList().DeriveWithStyle(gfx::Font::UNDERLINE);
+      styled()->GetFontList().DeriveWithStyle(gfx::Font::UNDERLINE);
   styled()->AddStyleRange(
       gfx::Range(text.size(), text.size() + underlined_text.size()),
       style_info);
@@ -307,7 +307,7 @@ TEST_F(StyledLabelTest, StyledRangeTextStyleBold) {
   // Pretend disabled text becomes bold for testing.
   bold_provider.SetFont(
       style::CONTEXT_LABEL, style::STYLE_DISABLED,
-      styled()->GetDefaultFontList().DeriveWithWeight(gfx::Font::Weight::BOLD));
+      styled()->GetFontList().DeriveWithWeight(gfx::Font::Weight::BOLD));
 
   StyledLabel::RangeStyleInfo style_info;
   style_info.text_style = style::STYLE_DISABLED;
@@ -373,11 +373,8 @@ TEST_F(StyledLabelTest, Color) {
   styled()->SetBounds(0, 0, 1000, 1000);
   styled()->Layout();
 
-  Widget* widget = new Widget();
-  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
-  widget->Init(std::move(params));
-  View* container = new View();
-  widget->SetContentsView(container);
+  std::unique_ptr<Widget> widget = CreateTestWidget();
+  View* container = widget->SetContentsView(std::make_unique<View>());
 
   // The code below is not prepared to deal with dark mode.
   widget->GetNativeTheme()->set_use_dark_colors(false);
@@ -394,6 +391,7 @@ TEST_F(StyledLabelTest, Color) {
       container->AddChildView(std::make_unique<Link>(ASCIIToUTF16(text_link)));
   const SkColor kDefaultLinkColor = link->GetEnabledColor();
 
+  ASSERT_EQ(3u, styled()->children().size());
   EXPECT_EQ(SK_ColorBLUE, LabelAt(0)->GetEnabledColor());
   EXPECT_EQ(kDefaultLinkColor,
             LabelAt(1, Link::kViewClassName)->GetEnabledColor());

ui/views/style/typography_provider.cc

@@ -143,7 +143,7 @@ SkColor TypographyProvider::GetColor(const View& view,
 }
 
 int TypographyProvider::GetLineHeight(int context, int style) const {
-  return 0;
+  return GetFont(context, style).GetHeight();
 }
 
 // static

ui/views/style/typography_provider.h

@@ -33,7 +33,7 @@ class VIEWS_EXPORT TypographyProvider {
                            int context,
                            int style) const;
 
-  // Gets the line spacing, or 0 if it should be provided by gfx::FontList.
+  // Gets the line spacing.  By default this is the font height.
   virtual int GetLineHeight(int context, int style) const;
 
   // Returns the weight that will result in the ResourceBundle returning an