Fix the size of omnibox suggestion answers.

chrome/browser/ui/views/omnibox/omnibox_result_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.
 
 // For WinDDK ATL compatibility, these ATL headers must come first.
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <atlbase.h>  // NOLINT
 #include <atlwin.h>  // NOLINT
@@ skipping 106 matching lines @@
               {NativeTheme::kColorId_ResultsTableNormalText,
                NativeTheme::kColorId_ResultsTableHoveredText,
                NativeTheme::kColorId_ResultsTableSelectedText},
               gfx::NORMAL_BASELINE};
     case SuggestionAnswer::ANSWER_TEXT_LARGE:
       return {ui::ResourceBundle::LargeFont,
               {NativeTheme::kColorId_ResultsTableNormalText,
                NativeTheme::kColorId_ResultsTableHoveredText,
                NativeTheme::kColorId_ResultsTableSelectedText},
               gfx::NORMAL_BASELINE};
-    case SuggestionAnswer::SUGGESTION_SECONDARY_TEXT_SMALL:  // Fall through.
+    case SuggestionAnswer::SUGGESTION_SECONDARY_TEXT_SMALL:
+      return {ui::ResourceBundle::LargeFont,

[Peter Kasting, 2017/02/01 02:00:58]

LargeFont?  Shouldn't this be BaseFont?

[Justin Donnelly, 2017/02/01 16:58:45]

Unfortunately, no, LargeFont is "correct". The way the actual text sizes are
produced is pretty hacky. One the one hand, it doesn't actually matter what this
value is because it's only consulted for the first type that appears on each
line. On the other, this text type only ever appears (with the current answer
types, anyway) following text types with a LargeFont (and that font is thus used
for this type - it's the gfx::INFERIOR that actually makes this type appear
small). So it makes more sense to align this with the font that actually will be
applied. Especially since this is already the way we handle similar small types
(TOP_ALIGNED, DESCRIPTION_NEGATIVE, DESCRIPTION_POSITIVE).

We really need to be able to apply font sizes to ranges in RenderText. Any
suggestions on who to talk to about that? I'm wondering if it's on anyone's
roadmap, if it's a 2-day or a 2-month job, etc.

[Peter Kasting, 2017/02/01 19:37:21]

I wonder if any of this deserves a comment in the code somewhere.
Ask msw, he'd know better than I would.

[Justin Donnelly, 2017/02/01 20:44:15]

sgtm, done.

+              {NativeTheme::kColorId_ResultsTableNormalDimmedText,
+               NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+               NativeTheme::kColorId_ResultsTableSelectedDimmedText},
+              gfx::INFERIOR};
     case SuggestionAnswer::SUGGESTION_SECONDARY_TEXT_MEDIUM:
       return {ui::ResourceBundle::BaseFont,
               {NativeTheme::kColorId_ResultsTableNormalDimmedText,
                NativeTheme::kColorId_ResultsTableHoveredDimmedText,
                NativeTheme::kColorId_ResultsTableSelectedDimmedText},
               gfx::NORMAL_BASELINE};
     case SuggestionAnswer::SUGGESTION:  // Fall through.
     default:
       return {ui::ResourceBundle::BaseFont,
               {NativeTheme::kColorId_ResultsTableNormalText,
@@ skipping 133 matching lines @@
   // selected. The non-answer text is already accessible as a consequence of
   // updating the text in the omnibox but this alert and GetAccessibleNodeData
   // below make the answer contents accessible.
   if (match_.answer)
     NotifyAccessibilityEvent(ui::AX_EVENT_ALERT, true);
 }
 
 gfx::Size OmniboxResultView::GetPreferredSize() const {
   if (!match_.answer)
     return gfx::Size(0, GetContentLineHeight());
-  // An answer implies a match and a description in a large font.
   if (match_.answer->second_line().num_text_lines() == 1)
     return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight());
   if (!description_rendertext_) {
     description_rendertext_ =
-        CreateAnswerLine(match_.answer->second_line(), font_list_);
+        CreateAnswerLine(match_.answer->second_line(), GetAnswerLineFont());
   }
   description_rendertext_->SetDisplayRect(
       gfx::Rect(text_bounds_.width(), 0));
   description_rendertext_->GetStringSize();
   return gfx::Size(
       0, GetContentLineHeight() +
              GetAnswerLineHeight() * description_rendertext_->GetNumLines());
 }
 
 void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
@@ skipping 343 matching lines @@
                          icon_bounds_.y());
     int x = GetMirroredXForRect(text_bounds_);
     mirroring_context_->Initialize(x, text_bounds_.width());
     InitContentsRenderTextIfNecessary();
 
     if (!description_rendertext_) {
       if (match_.answer) {
         contents_rendertext_ =
             CreateAnswerLine(match_.answer->first_line(), font_list_);
         description_rendertext_ =
-            CreateAnswerLine(match_.answer->second_line(), font_list_);
+            CreateAnswerLine(match_.answer->second_line(), GetAnswerLineFont());
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);
       }
     }
     PaintMatch(match_, contents_rendertext_.get(),
                description_rendertext_.get(), canvas, x);
   }
 
   AutocompleteMatch* keyword_match = match_.associated_keyword.get();
@@ skipping 16 matching lines @@
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }
 
+const gfx::FontList& OmniboxResultView::GetAnswerLineFont() const {
+  // This assumes that the first text type in the second answer line can be used
+  // to specify the font for all the text fields in the line. For now this works
+  // but eventually it will be necessary to get RenderText to support multiple
+  // font sizes or use multiple RenderTexts.
+  int text_type =
+      match_.answer && !match_.answer->second_line().text_fields().empty()
+          ? match_.answer->second_line().text_fields()[0].type()
+          : SuggestionAnswer::SUGGESTION;
+  return ui::ResourceBundle::GetSharedInstance().GetFontList(
+      GetTextStyle(text_type).font);
+}
+
 int OmniboxResultView::GetAnswerLineHeight() const {
-  // ANSWER_TEXT_LARGE is the largest font used and so defines the boundary that
-  // all the other answer styles fit within.
-  return ui::ResourceBundle::GetSharedInstance()
-      .GetFontList(GetTextStyle(SuggestionAnswer::ANSWER_TEXT_LARGE).font)
-      .GetHeight();
+  return GetAnswerLineFont().GetHeight();

[Peter Kasting, 2017/02/01 02:00:58]

Should we just inline this into callers now?  I didn't look closely and don't
have a strong opinion.

[Justin Donnelly, 2017/02/01 16:58:45]

I started doing this but then began playing around with a small bit of height
and positioning adjustments for answers, in which case it's useful to keep this
method.

Please take another quick look.

 }
 
 int OmniboxResultView::GetContentLineHeight() const {
   using Md = ui::MaterialDesignController;
   const int kIconVerticalPad = Md::GetMode() == Md::MATERIAL_HYBRID ? 8 : 4;
   const int kTextVerticalPad = 3;
   return std::max(
       LocationBarView::kIconWidth + 2 * kIconVerticalPad,
       GetTextHeight() + 2 * kTextVerticalPad);
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerLine(
     const SuggestionAnswer::ImageLine& line,
-    gfx::FontList font_list) const {
+    const gfx::FontList& font_list) const {
   std::unique_ptr<gfx::RenderText> destination =
       CreateRenderText(base::string16());
   destination->SetFontList(font_list);
 
   for (const SuggestionAnswer::TextField& text_field : line.text_fields())
     AppendAnswerText(destination.get(), text_field.text(), text_field.type());
   if (!line.text_fields().empty()) {
     constexpr int kMaxDisplayLines = 3;
     const SuggestionAnswer::TextField& first_field = line.text_fields().front();
     if (first_field.has_num_lines() && first_field.num_lines() > 1 &&
@@ skipping 56 matching lines @@
   destination->AppendText(text);
   const TextStyle& text_style = GetTextStyle(text_type);
   // TODO(dschuyler): follow up on the problem of different font sizes within
   // one RenderText.  Maybe with destination->SetFontList(...).
   destination->ApplyWeight(
       is_bold ? gfx::Font::Weight::BOLD : gfx::Font::Weight::NORMAL, range);
   destination->ApplyColor(
       GetNativeTheme()->GetSystemColor(text_style.colors[GetState()]), range);
   destination->ApplyBaselineStyle(text_style.baseline, range);
 }