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,
+              {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 49 matching lines @@
       match.answer != nullptr,
       !AutocompleteMatch::IsSearchType(match.type),
       &contents_max_width,
       &description_max_width);
 
   int after_contents_x = DrawRenderText(match, contents, CONTENTS, canvas,
                                         x, y, contents_max_width);
 
   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(
             answer_image_,
             0, 0, answer_image_.width(), answer_image_.height(),
             GetMirroredXInView(x), y, answer_icon_size, answer_icon_size, true);
         // TODO(dschuyler): Perhaps this should be based on the font size
         // instead of hardcoded to 2 dp (e.g. by adding a space in an
         // appropriate font to the beginning of the description, then reducing
         // the additional padding here to zero).
@@ skipping 273 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();
+  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 {
   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);
 }