Fix the size of omnibox suggestion answers.

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
index a91113692703a538094cd399bf96c4a9f21c8028..1ad5c2122aab879ac8c3fed55e8b85133ad5a2dc 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -124,7 +124,12 @@ TextStyle GetTextStyle(int type) {
                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,
+              {NativeTheme::kColorId_ResultsTableNormalDimmedText,
+               NativeTheme::kColorId_ResultsTableHoveredDimmedText,
+               NativeTheme::kColorId_ResultsTableSelectedDimmedText},
+              gfx::INFERIOR};
     case SuggestionAnswer::SUGGESTION_SECONDARY_TEXT_MEDIUM:
       return {ui::ResourceBundle::BaseFont,
               {NativeTheme::kColorId_ResultsTableNormalDimmedText,
@@ -278,12 +283,11 @@ void OmniboxResultView::OnSelected() {
 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));
@@ -353,7 +357,8 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
 
   if (description_max_width != 0) {
     if (match.answer) {
-      y += GetContentLineHeight();
+      const int kTextVerticalAdjustment = 1;

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

Are you trying to offset for the "1" added in GetAnswerLineHeight()?  If so find
a way to share the constants.

But this seems really weird.  We're saying the answer line is n + 2 DIP high,
and e.g. GetPreferredSize() sizes things based on the content + answer line
height, but then here we're not actually positioning things that way.  So you
have a mismatch between the sizing and the layout logic.

See also comment below.

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

No, I'm trying to deal with what I think is an effect of the padding for the
content line being between the content and description lines rather than at the
bottom of the entire suggestion. But I'll admit I'm not 100% sure I understand
what's going on here. I've removed this and I'll return to it at a later time.

[Peter Kasting, 2017/02/01 20:50:00]

It sounds like what really really need is for both layout and sizing to put
padding around neither content nor answer lines, but instead around the [content
+ answer] lines?  That is, the padding in GetContentLineHeight() may be in the
wrong place.

+      y += GetContentLineHeight() - kTextVerticalAdjustment;
       if (!answer_image_.isNull()) {
         int answer_icon_size = GetAnswerLineHeight();
         canvas->DrawImageInt(
@@ -647,7 +652,7 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
         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);
@@ -684,12 +689,22 @@ void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   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();
+  const int kTextVerticalPad = 1;

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

Whaaat.  Why do you want this?  At the very least, this needs a detailed
comment.

But my instincts are all "don't do this".  How does it work for languages with
very different font metrics (e.g. Hindi)?  Do you really want 1 DIP, or is that
too much in high DPI?

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

Here I'm simply doing what GetContentLineHeight() does below to add padding.
Currently, the only padding at the bottom of an answer suggestion is the descent
of the text. It needs its own padding just like the content line.

+  return GetAnswerLineFont().GetHeight() + 2 * kTextVerticalPad;
 }
 
 int OmniboxResultView::GetContentLineHeight() const {
@@ -703,7 +718,7 @@ int OmniboxResultView::GetContentLineHeight() const {
 
 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);