Fix the size of omnibox suggestion answers.

Fix the size of omnibox suggestion answers.

Through a confluence of various changes, OmniboxResultView got into a state where the height given to answers is always the largest size but the actual font used is the regular font size.

This fix ensures that both the size of the font and the space it's given are driven by the data in the answer. For the currently launched answers, this is always the largest size so they're being shown too small. But this fix will also support upcoming answer types that don't use the largest font size.

BUG=685714
Review-Url: https://codereview.chromium.org/2654163005
Cr-Commit-Position: refs/heads/master@{#447702}
Committed: https://chromium.googlesource.com/chromium/src/+/5fdae6fa2582b407ad6281c152e5cb6f7b95a391

[Justin Donnelly, 2017-01-26 23:05:50]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 23:06:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Donnelly, 2017-01-26 23:15:32]

Description was changed from

==========
Fix the size of suggestion answers.

BUG=
==========

to

==========
Fix the size of omnibox suggestion answers.

Through a confluence of various changes, OmniboxResultView got into a state
where the height given to answers is always the largest size but the actual font
used is the regular font size.

This fix ensures that both the size of the font and the space it's given are
driven by the data in the answer. For the currently launched answers, this is
always the largest size so they're being shown too small. But this fix will also
support upcoming answer types that don't use the largest font size.

BUG=685714
==========

[Justin Donnelly, 2017-01-26 23:15:32]

jdonnelly@chromium.org changed reviewers:
+ pkasting@chromium.org

[commit-bot: I haz the power, 2017-01-27 01:39:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 01:39:07]

Dry run: This issue passed the CQ dry run.

[Justin Donnelly, 2017-01-31 15:22:00]

Friendly ping.

[Peter Kasting, 2017-01-31 21:39:45]

On 2017/01/31 15:22:00, Justin Donnelly wrote:
> Friendly ping.

I think there was no preceding mail, so I'm glad it's friendly, because it's the
first time I'm seeing it :)

[Justin Donnelly, 2017-01-31 21:42:21]

Yeah, sorry about that. User error. :)

On Tue, Jan 31, 2017 at 1:39 PM, <pkasting@chromium.org> wrote:

> On 2017/01/31 15:22:00, Justin Donnelly wrote:
> > Friendly ping.
>
> I think there was no preceding mail, so I'm glad it's friendly, because
> it's the
> first time I'm seeing it :)
>
> https://codereview.chromium.org/2654163005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

LGTM

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:128: return
{ui::ResourceBundle::LargeFont,
LargeFont?  Shouldn't this be BaseFont?

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:705: return
GetAnswerLineFont().GetHeight();
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:49:57]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 16:50:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Donnelly, 2017-02-01 16:53:45]

Patchset #2 (id:20001) has been deleted

[Justin Donnelly, 2017-02-01 16:57:08]

Patchset #2 (id:40001) has been deleted

[Justin Donnelly, 2017-02-01 16:58:28]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 16:58:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:128: return
{ui::ResourceBundle::LargeFont,
On 2017/02/01 02:00:58, Peter Kasting wrote:
> LargeFont?  Shouldn't this be BaseFont?

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.

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:705: return
GetAnswerLineFont().GetHeight();
On 2017/02/01 02:00:58, Peter Kasting wrote:
> Should we just inline this into callers now?  I didn't look closely and don't
> have a strong opinion.

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.

[commit-bot: I haz the power, 2017-02-01 17:40:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 17:40:20]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:128: return
{ui::ResourceBundle::LargeFont,
On 2017/02/01 16:58:45, Justin Donnelly wrote:
> On 2017/02/01 02:00:58, Peter Kasting wrote:
> > LargeFont?  Shouldn't this be BaseFont?
> 
> 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).

I wonder if any of this deserves a comment in the code somewhere.

> 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.

Ask msw, he'd know better than I would.

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:360: const int
kTextVerticalAdjustment = 1;
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.

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:706: const int
kTextVerticalPad = 1;
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]

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:128: return
{ui::ResourceBundle::LargeFont,
On 2017/02/01 19:37:21, Peter Kasting wrote:
> I wonder if any of this deserves a comment in the code somewhere.

sgtm, done.

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:360: const int
kTextVerticalAdjustment = 1;
On 2017/02/01 19:37:21, Peter Kasting wrote:
> 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.

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.

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:706: const int
kTextVerticalPad = 1;
On 2017/02/01 19:37:21, Peter Kasting wrote:
> 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?

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.

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

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:360: const int
kTextVerticalAdjustment = 1;
On 2017/02/01 20:44:15, Justin Donnelly wrote:
> On 2017/02/01 19:37:21, Peter Kasting wrote:
> > 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.
> 
> 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.

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.

[Justin Donnelly, 2017-02-01 22:40:57]

On 2017/02/01 20:50:00, Peter Kasting wrote:
>
https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
> File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):
> 
>
https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/omnibox/omnibox_result_view.cc:360: const int
> kTextVerticalAdjustment = 1;
> On 2017/02/01 20:44:15, Justin Donnelly wrote:
> > On 2017/02/01 19:37:21, Peter Kasting wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

But that would leave no padding between the content and answer lines. How the
padding works is deserving of a rethink but I'm planning to merge this CL to M57
to fix the more serious sizing issue. A small tweak to add padding to the answer
line in a way that's consistent with how it's added to the content line seemed
reasonable and is an incremental improvement to how the answer suggestions look
so I was hoping to include it. But if you have serious reservations about it, I
can remove these lines from the CL as well.

[Peter Kasting, 2017-02-01 22:46:55]

On 2017/02/01 22:40:57, Justin Donnelly wrote:
> On 2017/02/01 20:50:00, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2654163005/diff/60001/chrome/browser/ui/views...
> > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:360: const int
> > kTextVerticalAdjustment = 1;
> > On 2017/02/01 20:44:15, Justin Donnelly wrote:
> > > On 2017/02/01 19:37:21, Peter Kasting wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> But that would leave no padding between the content and answer lines.

Sorry, I meant to imply we should have padding there, but computed distinctly
from the height of the content/answer lines themselves.

> How the
> padding works is deserving of a rethink but I'm planning to merge this CL to
M57
> to fix the more serious sizing issue. A small tweak to add padding to the
answer
> line in a way that's consistent with how it's added to the content line seemed
> reasonable and is an incremental improvement to how the answer suggestions
look
> so I was hoping to include it. But if you have serious reservations about it,
I
> can remove these lines from the CL as well.

If you're planning to merge back, I'd say trim back to just the sizing issue,
and file a separate bug about the padding, so we can fix it coherently.  I agree
with you that the current code is wrong and something should be done, and I
don't want to block your M57 fix on that :)

[Justin Donnelly, 2017-02-01 23:01:49]

The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 23:02:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Donnelly, 2017-02-01 23:03:12]

On 2017/02/01 22:46:55, Peter Kasting wrote:
> If you're planning to merge back, I'd say trim back to just the sizing issue,
> and file a separate bug about the padding, so we can fix it coherently.

Done.

[Peter Kasting, 2017-02-01 23:09:54]

On 2017/02/01 23:03:12, Justin Donnelly wrote:
> On 2017/02/01 22:46:55, Peter Kasting wrote:
> > If you're planning to merge back, I'd say trim back to just the sizing
issue,
> > and file a separate bug about the padding, so we can fix it coherently.
> 
> Done.

LGTM

[commit-bot: I haz the power, 2017-02-01 23:52:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 23:52:02]

Dry run: This issue passed the CQ dry run.

[Justin Donnelly, 2017-02-02 01:14:51]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2017-02-02 01:15:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 01:22:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 01:22:28]

Failed to apply patch for
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:647
  error: chrome/browser/ui/views/omnibox/omnibox_result_view.cc: patch does not
apply

Patch:       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..3bcde2435dc5c02a8381ee01a13170cc74c221a7
100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -86,6 +86,19 @@ struct TextStyle {
   gfx::BaselineStyle baseline;
 };
 
+// Returns the styles that should be applied to the specified answer text type.
+//
+// Note that the font value is only consulted for the first text type that
+// appears on an answer line, because RenderText does not yet support multiple
+// font sizes. Subsequent text types on the same line will share the text size
+// of the first type, while the color and baseline styles specified here will
+// always apply. The gfx::INFERIOR baseline style is used as a workaround to
+// produce smaller text on the same line. The way this is used in the current
+// set of answers is that the small types (TOP_ALIGNED, DESCRIPTION_NEGATIVE,
+// DESCRIPTION_POSITIVE and SUGGESTION_SECONDARY_TEXT_SMALL) only ever appear
+// following LargeFont text, so for consistency they specify LargeFont for the
+// first value even though this is not actually used (since they're not the
+// first value).
 TextStyle GetTextStyle(int type) {
   switch (type) {
     case SuggestionAnswer::TOP_ALIGNED:
@@ -124,7 +137,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 +296,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));
@@ -647,7 +664,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 +701,21 @@ 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();
+  return GetAnswerLineFont().GetHeight();
 }
 
 int OmniboxResultView::GetContentLineHeight() const {
@@ -703,7 +729,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);

[Justin Donnelly, 2017-02-02 02:24:12]

The CQ bit was checked by jdonnelly@chromium.org

[Justin Donnelly, 2017-02-02 02:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2654163005/#ps120001
(title: "Merge.")

[commit-bot: I haz the power, 2017-02-02 02:24:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 05:49:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486002252380140,
"parent_rev": "0a597131aec73063576b26e6caabde99b09ec890", "commit_rev":
"5fdae6fa2582b407ad6281c152e5cb6f7b95a391"}

[commit-bot: I haz the power, 2017-02-02 05:50:36]

Description was changed from

==========
Fix the size of omnibox suggestion answers.

Through a confluence of various changes, OmniboxResultView got into a state
where the height given to answers is always the largest size but the actual font
used is the regular font size.

This fix ensures that both the size of the font and the space it's given are
driven by the data in the answer. For the currently launched answers, this is
always the largest size so they're being shown too small. But this fix will also
support upcoming answer types that don't use the largest font size.

BUG=685714
==========

to

==========
Fix the size of omnibox suggestion answers.

Through a confluence of various changes, OmniboxResultView got into a state
where the height given to answers is always the largest size but the actual font
used is the regular font size.

This fix ensures that both the size of the font and the space it's given are
driven by the data in the answer. For the currently launched answers, this is
always the largest size so they're being shown too small. But this fix will also
support upcoming answer types that don't use the largest font size.

BUG=685714

Review-Url: https://codereview.chromium.org/2654163005
Cr-Commit-Position: refs/heads/master@{#447702}
Committed:
https://chromium.googlesource.com/chromium/src/+/5fdae6fa2582b407ad6281c152e5...
==========

[commit-bot: I haz the power, 2017-02-02 05:50:37]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5fdae6fa2582b407ad6281c152e5...