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

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 30 matching lines @@
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/gfx/vector_icons_public.h"
 #include "ui/native_theme/native_theme.h"
 
 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 {
   ui::NativeTheme::ColorId id;
   OmniboxResultView::ResultViewState state;
   OmniboxResultView::ColorKind kind;
 } static const kTranslationTable[] = {
   { NativeTheme::kColorId_ResultsTableNormalBackground,
     OmniboxResultView::NORMAL, OmniboxResultView::BACKGROUND },
   { NativeTheme::kColorId_ResultsTableHoveredBackground,
@@ skipping 210 matching lines @@
   // updating the text in the omnibox but this alert and GetAccessibleState
   // 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.
-  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) {
   state->name = match_.answer
                     ? l10n_util::GetStringFUTF16(
                           IDS_OMNIBOX_ACCESSIBLE_ANSWER, match_.contents,
                           match_.answer->second_line().AccessibleText())
                     : match_.contents;
 }
 
@@ skipping 131 matching lines @@
         gfx::DIRECTIONALITY_FORCE_RTL : gfx::DIRECTIONALITY_FORCE_LTR);
     prefix_render_text->SetHorizontalAlignment(
           is_match_contents_rtl ? gfx::ALIGN_RIGHT : gfx::ALIGN_LEFT);
     prefix_render_text->SetDisplayRect(
         gfx::Rect(mirroring_context_->mirrored_left_coord(
                       prefix_x, prefix_x + prefix_width),
                   y, prefix_width, GetContentLineHeight()));
     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;
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateRenderText(
     const base::string16& text) const {
   std::unique_ptr<gfx::RenderText> render_text(
       gfx::RenderText::CreateInstance());
   render_text->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
   render_text->SetCursorEnabled(false);
@@ skipping 225 matching lines @@
     canvas->DrawImageInt(GetIcon(), GetMirroredXForRect(icon_bounds_),
                          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(),
-            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);
       }
     }
     PaintMatch(match_, contents_rendertext_.get(),
                description_rendertext_.get(), canvas, x);
   }
 
   AutocompleteMatch* keyword_match = match_.associated_keyword.get();
@@ skipping 32 matching lines @@
 }
 
 int OmniboxResultView::GetContentLineHeight() const {
   return std::max(
       default_icon_size_ + GetLayoutInsets(OMNIBOX_DROPDOWN_ICON).height(),
       GetTextHeight() + GetLayoutInsets(OMNIBOX_DROPDOWN_TEXT).height());
 }
 
 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) {
     AppendAnswerText(destination.get(), space + text_field->text(),
                      text_field->type());
   }
   text_field = line.status_text();
   if (text_field) {
     AppendAnswerText(destination.get(), space + text_field->text(),
                      text_field->type());
   }
   return destination;
 }
 
 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
   // entities (&lt; for '<', etc.).
   const base::string16 begin_tag = base::ASCIIToUTF16("<b>");
   const base::string16 end_tag = base::ASCIIToUTF16("</b>");
   size_t begin = 0;
   while (true) {
     size_t end = text.find(begin_tag, begin);
     if (end == base::string16::npos) {
       AppendAnswerTextHelper(destination, text.substr(begin), text_type, false);
       break;
     }
     AppendAnswerTextHelper(destination, text.substr(begin, end - begin),
                            text_type, false);
     begin = end + begin_tag.length();
     end = text.find(end_tag, begin);
     if (end == base::string16::npos)
       break;
     AppendAnswerTextHelper(destination, text.substr(begin, end - begin),
                            text_type, true);
     begin = end + end_tag.length();
   }
 }
 
 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();
   gfx::Range range(offset, offset + text.length());
   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->ApplyStyle(gfx::BOLD, is_bold, range);
   destination->ApplyColor(
       GetNativeTheme()->GetSystemColor(text_style.colors[GetState()]), range);
   destination->ApplyBaselineStyle(text_style.baseline, range);
 }
 
 int OmniboxResultView::StartMargin() const {
   return ui::MaterialDesignController::IsModeMaterial() ?
       model_->start_margin() : 0;
 }
 
 int OmniboxResultView::EndMargin() const {
   return ui::MaterialDesignController::IsModeMaterial() ?
       model_->end_margin() : 0;
 }