Omnibox UI: Fix Answers in Suggest display in Views

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 5c4cdc271b31a7556b7930a780b4f383ffed1aee..6d29e0a88eb076eed56a869c07f408f5eeb59282 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
@@ -380,6 +380,8 @@ void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
       const int kAnswerIconToTextPadding = 2;
       x += answer_icon_size + kAnswerIconToTextPadding;
     }
+    DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
+                   description_max_width);

[Peter Kasting, 2017/06/09 02:33:48]

Depending on how we address the above, we may be able to refactor this so this
DrawRenderText() call happens after these conditional blocks that draw the
CONTENTS and update x/y.

[tommycli, 2017/06/09 16:09:21]

Yes I definitely thought about this. Ultimately I thought it would be clearer if
all the operations for the answer case were together and all the operations for
the non-answer horizontal, and all the operations for the non-answer vertical
were together.

We can change in a followup if you hove strong opinions here.

[Peter Kasting, 2017/06/09 19:30:53]

I think we should change it.  Here's my mock attempt to do so.  To me this is
clearer and simpler.  You'd want to address the TODOs I added here.

  // Draw contents.
  // For vertical layouts with no description, vertically center.
  // TODO(pkasting): I think we should remove this behavior
  const bool vertical =
      base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout);
  const bool show_description = description_max_width > 0;
  if (vertical && !show_description)
    y += (GetTextHeight() + kVerticalPadding) / 2;
  const int contents_end = DrawRenderText(match, contents, CONTENTS, canvas, x,
                                          y, contents_max_width);

  // Move to position to draw answer/description.
  if (vertical || match.answer) {
    y += GetTextHeight() + kVerticalPadding;
  } else if (show_description) {
    x = DrawRenderText(match, separator_rendertext_.get(), SEPARATOR, canvas,
                       contents_end, y, separator_width_);
  }

  // Draw answer image, if any.
  // TODO(pkasting): Not clear whether we need first half of conditional
  if (match.answer && !answer_image_.isNull()) {
    int answer_icon_size = GetAnswerHeight();
    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).
    const int kAnswerIconToTextPadding = 2;
    x += answer_icon_size + kAnswerIconToTextPadding;
  }

  // Draw description.
  // TODO(pkasting): Conditional should maybe be hoisted to DrawRenderText()
  if (show_description) {
    DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
                   description_max_width);
  }

     return;
   }