Refine the layout of omnibox answer suggestions.

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 bc71cdf4d05f2003db63b53907d6dd64e74b89d9..a841b1ef4ddcbccc7e449f5d7b44ca780e39651e 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -48,6 +48,9 @@ using ui::NativeTheme;
 
 namespace {
 
+// The padding that should be placed between lines in a top and bottom layout.

[Peter Kasting, 2017/04/24 22:16:05]

What is "a top and bottom layout"?  Do you mean "When a single result has more
than one line of text"?

Also, I wonder if instead of pulling this in multiple places below, it would
make sense to use RenderText::SetMinLineHeight() on the answer rendertext.  Of
course, that will result in spacing out every line of text, not just placing
space above "the whole answer"... maybe that's not what you want.

[Justin Donnelly, 2017/04/25 16:52:30]

"Top and bottom" is meant to distinguish a layout where the content and
description are laid out vertically rather than side by side or horizontally.
We're going to be experimenting with having other suggestions than answers use
this layout so I wanted to introduce more general terminology. Now that I say it
out loud "vertical layout" sounds much better. Switched to that and changed the
wording to hopefully make this clearer.
This is the spacing between the content and description rendertexts. It's
confusing because these have been referred to as "lines" in this class but now
the description can be multiline. Suggestions on making this clearer are
welcome.

+static const int kVerticalPadding = 3;
+
 // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
 // NativeTheme colors.
 struct TranslationTable {
@@ -294,20 +297,10 @@ void OmniboxResultView::OnSelected() {
 }
 
 gfx::Size OmniboxResultView::GetPreferredSize() const {
-  if (!match_.answer)
-    return gfx::Size(0, GetContentLineHeight());
-  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(), GetAnswerLineFont());
-  }
-  description_rendertext_->SetDisplayRect(
-      gfx::Rect(text_bounds_.width(), 0));
-  description_rendertext_->GetStringSize();
-  return gfx::Size(
-      0, GetContentLineHeight() +
-             GetAnswerLineHeight() * description_rendertext_->GetNumLines());
+  int height = GetTextHeight() + (2 * GetVerticalMargin());
+  if (match_.answer)
+    height += GetAnswerLineHeight() + kVerticalPadding;
+  return gfx::Size(0, height);
 }
 
 void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
@@ -341,7 +334,7 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
                                    gfx::RenderText* description,
                                    gfx::Canvas* canvas,
                                    int x) const {
-  int y = text_bounds_.y();
+  int y = text_bounds_.y() + GetVerticalMargin();
 
   if (!separator_rendertext_) {
     const base::string16& separator =
@@ -370,7 +363,7 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
 
   if (description_max_width != 0) {
     if (match.answer) {
-      y += GetContentLineHeight();
+      y += GetTextHeight() + kVerticalPadding;
       if (!answer_image_.isNull()) {
         int answer_icon_size = GetAnswerLineHeight();

[Peter Kasting, 2017/04/24 22:16:05]

This will vertically center the icon against (however many answer lines you
have) instead of against the center of the top line of the answer.  Is that
really what you want?

[Justin Donnelly, 2017/04/25 16:52:30]

Yes, that's what I want. With multiple lines of text, the line height will be
small.

         canvas->DrawImageInt(
@@ -460,21 +453,17 @@ int OmniboxResultView::DrawRenderText(
     prefix_render_text->SetDisplayRect(
         gfx::Rect(mirroring_context_->mirrored_left_coord(
                       prefix_x, prefix_x + prefix_width),
-                  y, prefix_width, GetContentLineHeight()));
+                  y, prefix_width, GetTextHeight()));
     prefix_render_text->Draw(canvas);
   }
 
   // Set the display rect to trigger elision.
-  const int final_width = right_x - x;
-  int height = GetContentLineHeight();
-  if (render_text_type == DESCRIPTION && match.answer) {
-    render_text->SetDisplayRect(gfx::Rect(gfx::Size(final_width, 0)));
-    render_text->GetStringSize();
-    height = GetAnswerLineHeight() * render_text->GetNumLines();
-  }
+  int height = (render_text_type == DESCRIPTION && match.answer)
+                   ? GetAnswerLineHeight()
+                   : GetTextHeight();
   render_text->SetDisplayRect(
       gfx::Rect(mirroring_context_->mirrored_left_coord(x, right_x), y,
-                final_width, height));
+                right_x - x, height));
   render_text->Draw(canvas);
   return right_x;
 }
@@ -625,8 +614,9 @@ void OmniboxResultView::Layout() {
   const int end_x = width() - start_x;
 
   const gfx::ImageSkia icon = GetIcon();
-  icon_bounds_.SetRect(start_x, (GetContentLineHeight() - icon.height()) / 2,
-                       icon.width(), icon.height());
+  const int icon_y =
+      GetVerticalMargin() + (GetTextHeight() - icon.height()) / 2;
+  icon_bounds_.SetRect(start_x, icon_y, icon.width(), icon.height());
 
   const int text_x = start_x + LocationBarView::kIconWidth + horizontal_padding;
   int text_width = end_x - text_x;
@@ -665,7 +655,7 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
     if (!description_rendertext_) {
       if (match_.answer) {
         description_rendertext_ =
-            CreateAnswerLine(match_.answer->second_line(), GetAnswerLineFont());
+            CreateAnswerLine(match_.answer->second_line(), GetAnswerFont());
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);
@@ -698,7 +688,7 @@ void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   SchedulePaint();
 }
 
-const gfx::FontList& OmniboxResultView::GetAnswerLineFont() const {
+const gfx::FontList& OmniboxResultView::GetAnswerFont() 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
@@ -712,16 +702,31 @@ const gfx::FontList& OmniboxResultView::GetAnswerLineFont() const {
 }
 
 int OmniboxResultView::GetAnswerLineHeight() const {

[Peter Kasting, 2017/04/24 22:16:05]

Nit: Seems like GetAnswerHeight() would be better both from "parallel structure"
and "it can be more than one line" perspectives.

[Justin Donnelly, 2017/04/25 16:52:30]

Good point, done. Also changed CreateAnswerLine() to CreateAnswerText() to try
to eliminate the old "line" terminology.

-  return GetAnswerLineFont().GetHeight();
+  // If the answer specifies a maximum of 1 line we can simply return the answer
+  // font height.
+  if (match_.answer->second_line().num_text_lines() == 1)
+    return GetAnswerFont().GetHeight();
+
+  // Multi-line answers require layout in order to determine the number of lines
+  // the RenderText will use.
+  if (!description_rendertext_) {
+    description_rendertext_ =
+        CreateAnswerLine(match_.answer->second_line(), GetAnswerFont());
+  }
+  description_rendertext_->SetDisplayRect(gfx::Rect(text_bounds_.width(), 0));
+  description_rendertext_->GetStringSize();
+  return GetAnswerFont().GetHeight() * description_rendertext_->GetNumLines();
 }
 
-int OmniboxResultView::GetContentLineHeight() const {
+int OmniboxResultView::GetVerticalMargin() const {
+  // Regardless of the text size, we ensure a minimum size for the content line

[Justin Donnelly, 2017/04/21 16:57:08]

This is my understanding of what this logic is trying to accomplish. Please let
me know if I'm missing some subtlety.

+  // here. This minimum is larger for hybrid mouse/touch devices to ensure an
+  // adequately sized touch target.
   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);
+  const int min_height = LocationBarView::kIconWidth + 2 * kIconVerticalPad;
+
+  return std::max(kVerticalPadding, (min_height - GetTextHeight()) / 2);
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerLine(