AiS: Have Omnibox definitions use new multi-line elision in RenderText

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 8dc6d54a0d73bbed8a32e40473bac8be5a4c17ee..33d07f1d35c208ffe91eeb6688109fce375222d0 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 {
 
+// Don't trust the number of lines from AiS. Cap it.
+constexpr int kMAX_DISPLAY_LINES = 3;

[Peter Kasting, 2016/06/01 16:15:29]

Nit: Declare this in the lone scope where it's used.

Name it kMaxDisplayLines.  (All caps style, which we don't use, would omit the
leading k.)

[Kevin Bailey, 2016/06/01 17:33:59]

Done.

+
 // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
 // NativeTheme colors.
 struct TranslationTable {
@@ -278,7 +281,20 @@ 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.
-  return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight());
+  const auto& text_fields = match_.answer->second_line().text_fields();
+  if (!text_fields.empty() && text_fields[0].has_num_lines()) {

[Peter Kasting, 2016/06/01 16:15:29]

Nit: You use [0] here and .front() below.  Pick one.  (I tend to use .front(),
but I think I'm an outlier.)

[Kevin Bailey, 2016/06/01 17:34:00]

Done.

+    if (!description_rendertext_)

[Peter Kasting, 2016/06/01 16:15:29]

Nit: {}

[Kevin Bailey, 2016/06/01 17:34:00]

Done.

+      description_rendertext_ =
+          CreateAnswerLine(match_.answer->second_line(), font_list_);
+    description_rendertext_->SetDisplayRect(
+        gfx::Rect(text_bounds().width(), 0));

[Peter Kasting, 2016/06/01 16:15:29]

Nit: Prefer text_bounds_

[Kevin Bailey, 2016/06/01 17:34:00]

This one surprises me, particularly since 'text_bounds()' is const, but sure.

+    description_rendertext_->GetStringSize();
+    return gfx::Size(
+        0, GetContentLineHeight() +
+               GetAnswerLineHeight() * description_rendertext_->GetNumLines());
+  } else {
+    return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight());

[Peter Kasting, 2016/06/01 16:15:29]

Nit: Reverse the conditional above and do this as an early-return; or else do
something like:

  int num_lines = 1;
  if (...) {
    ...
    num_lines = description_rendertext_->GetNumLines();
  }
  return gfx::Size(0,
                   GetContentLineHeight() + GetAnswerLineHeight() * num_lines);

[Kevin Bailey, 2016/06/01 17:34:00]

Done.

+  }
 }
 
 void OmniboxResultView::GetAccessibleState(ui::AXViewState* state) {
@@ -430,12 +446,17 @@ int OmniboxResultView::DrawRenderText(
     prefix_render_text->Draw(canvas);
   }
 
-  // Set the display rect to trigger eliding.
-  const int height = (render_text_type == DESCRIPTION && match.answer) ?
-      GetAnswerLineHeight() : GetContentLineHeight();
+  // Set the display rect to trigger elision.
+  int final_width = right_x - x;

[Peter Kasting, 2016/06/01 16:15:29]

Nit: const?

[Kevin Bailey, 2016/06/01 17:34:00]

Done.

+  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();
+  }
   render_text->SetDisplayRect(
       gfx::Rect(mirroring_context_->mirrored_left_coord(x, right_x), y,
-                right_x - x, height));
+                final_width, height));
   render_text->Draw(canvas);
   return right_x;
 }
@@ -681,10 +702,8 @@ void OmniboxResultView::OnPaint(gfx::Canvas* canvas) {
       if (match_.answer) {
         contents_rendertext_ =
             CreateAnswerLine(match_.answer->first_line(), font_list_);
-        description_rendertext_ = CreateAnswerLine(
-            match_.answer->second_line(),
-            ui::ResourceBundle::GetSharedInstance().GetFontList(
-                ui::ResourceBundle::LargeFont));
+        description_rendertext_ =
+            CreateAnswerLine(match_.answer->second_line(), font_list_);
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);
@@ -737,13 +756,22 @@ int OmniboxResultView::GetContentLineHeight() const {
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerLine(
     const SuggestionAnswer::ImageLine& line,
-    gfx::FontList font_list) {
+    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())

[Kevin Bailey, 2016/06/01 14:56:36]

Am I the only bothered by the same variable being used for 4 different things?

[Peter Kasting, 2016/06/01 16:15:29]

Well, it's really 3 variables due to scoping.

If you're really bothered, you could call this one field, the next first_field,
the next additional_text, and the next status_text?

[Kevin Bailey, 2016/06/01 17:33:59]

I took care of one, but the others didn't seem like big enough improvements.
Maybe the next change to this function will tip the balance.

[Justin Donnelly, 2016/06/01 17:38:17]

To me a variable named after its type is just saying it's "a text field" which
all 4 things clearly are. And it has the advantage of being relatively short -
line breaks in statements tend to bother me in an unreasonable way. :) But I
have no objection to giving them different names if it reads clearer to you.

     AppendAnswerText(destination.get(), text_field.text(), text_field.type());
+  if (!line.text_fields().empty()) {
+    const SuggestionAnswer::TextField& text_field = line.text_fields().front();
+    if (text_field.has_num_lines() && text_field.num_lines() > 1 &&
+        destination->MultilineSupported()) {
+      destination->SetMultiline(true);
+      destination->SetMaxLines(
+          std::min(kMAX_DISPLAY_LINES, text_field.num_lines()));
+    }
+  }
   const base::char16 space(' ');
   const auto* text_field = line.additional_text();
   if (text_field) {
@@ -760,7 +788,7 @@ std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerLine(
 
 void OmniboxResultView::AppendAnswerText(gfx::RenderText* destination,
                                          const base::string16& text,
-                                         int text_type) {
+                                         int text_type) const {
   // TODO(dschuyler): make this better.  Right now this only supports unnested
   // bold tags.  In the future we'll need to flag unexpected tags while adding
   // support for b, i, u, sub, and sup.  We'll also need to support HTML
@@ -789,7 +817,7 @@ void OmniboxResultView::AppendAnswerText(gfx::RenderText* destination,
 void OmniboxResultView::AppendAnswerTextHelper(gfx::RenderText* destination,
                                                const base::string16& text,
                                                int text_type,
-                                               bool is_bold) {
+                                               bool is_bold) const {
   if (text.empty())
     return;
   int offset = destination->text().length();