[omnibox] Fix for last character of an answer could be incorrectly bold.

The first line of answers contains text and bolding that's calculated
server-side. That calculation is nearly identical to what we do Chrome-
side for non-answers. This change makes it so that we use the Chrome
calculation to avoid the problem were an old-but-still-matching answer
could be copied to the what-you-typed match and have the wrong bolding.

Because the first line of answers can have some additional text besides
the query, that additional text is added in a separate step.

Because we're no longer using the server-provided bolding, which used
HTML <b> tags, we no longer need the parsing of those tags that we were
previously using.

Finally, since there was code moving around in omnibox_text_view.cc
anyway, I made some simplifications based on the fact that this class
only ever has one render text.

Bug: 721820
Change-Id: I816c55cf9ff680dd922a18ef4cb9301543e1de25
Reviewed-on: https://chromium-review.googlesource.com/1070506Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567248}

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

@@ -135,12 +135,15 @@ void OmniboxResultView::Invalidate() {
       omnibox::kKeywordSearchIcon, GetLayoutConstant(LOCATION_BAR_ICON_SIZE),
       GetColor(OmniboxPart::RESULTS_ICON)));
 
+  // The content text is set to the match text and calculated classifications.
+  // Answers use their own styling for additional content text and the
+  // description text, whereas non-answer suggestions use the match text and
+  // calculated classifications for the description text.
+  suggestion_view_->content()->SetText(match_.contents, match_.contents_class);
   if (match_.answer) {
-    suggestion_view_->content()->SetText(match_.answer->first_line());
+    suggestion_view_->content()->AppendExtraText(match_.answer->first_line());
     suggestion_view_->description()->SetText(match_.answer->second_line());
   } else {
-    suggestion_view_->content()->SetText(match_.contents,
-                                         match_.contents_class);
     suggestion_view_->description()->SetText(match_.description,
                                              match_.description_class);
   }

chrome/browser/ui/views/omnibox/omnibox_text_view.cc

@@ -169,23 +169,32 @@ int OmniboxTextView::GetHeightForWidth(int width) const {
                                      : kVerticalPadding);
 }
 
-std::unique_ptr<gfx::RenderText> OmniboxTextView::CreateRenderText(
-    const base::string16& text) const {
-  auto render_text = gfx::RenderText::CreateHarfBuzzInstance();
-  render_text->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
-  render_text->SetCursorEnabled(false);
-  render_text->SetElideBehavior(gfx::ELIDE_TAIL);
-  render_text->SetFontList(
-      views::style::GetFont(CONTEXT_OMNIBOX_PRIMARY, kTextStyle));
-  render_text->SetText(text);
-  return render_text;
+void OmniboxTextView::OnPaint(gfx::Canvas* canvas) {
+  View::OnPaint(canvas);
+
+  if (!render_text_) {
+    return;
+  }
+  render_text_->SetDisplayRect(GetContentsBounds());
+  render_text_->Draw(canvas);
+}
+
+void OmniboxTextView::Dim() {
+  render_text_->SetColor(
+      result_view_->GetColor(OmniboxPart::RESULTS_TEXT_DIMMED));
 }
 
-std::unique_ptr<gfx::RenderText> OmniboxTextView::CreateClassifiedRenderText(
-    const base::string16& text,
-    const ACMatchClassifications& classifications) const {
-  std::unique_ptr<gfx::RenderText> render_text(CreateRenderText(text));
-  const size_t text_length = render_text->text().length();
+void OmniboxTextView::SetText(const base::string16& text) {
+  render_text_.reset();
+  render_text_ = CreateRenderText(text);
+  UpdateLineHeight();
+}
+
+void OmniboxTextView::SetText(const base::string16& text,
+                              const ACMatchClassifications& classifications) {
+  render_text_ = CreateRenderText(text);
+
+  const size_t text_length = render_text_->text().length();
   for (size_t i = 0; i < classifications.size(); ++i) {
     const size_t text_start = classifications[i].offset;
     if (text_start >= text_length)
@@ -199,158 +208,111 @@ std::unique_ptr<gfx::RenderText> OmniboxTextView::CreateClassifiedRenderText(
 
     // Calculate style-related data.
     if (classifications[i].style & ACMatchClassification::MATCH)
-      render_text->ApplyWeight(gfx::Font::Weight::BOLD, current_range);
+      render_text_->ApplyWeight(gfx::Font::Weight::BOLD, current_range);
 
     OmniboxPart part = OmniboxPart::RESULTS_TEXT_DEFAULT;
     if (classifications[i].style & ACMatchClassification::URL) {
       part = OmniboxPart::RESULTS_TEXT_URL;
-      render_text->SetDirectionalityMode(gfx::DIRECTIONALITY_AS_URL);
+      render_text_->SetDirectionalityMode(gfx::DIRECTIONALITY_AS_URL);
     } else if (classifications[i].style & ACMatchClassification::DIM) {
       part = OmniboxPart::RESULTS_TEXT_DIMMED;
     } else if (classifications[i].style & ACMatchClassification::INVISIBLE) {
       part = OmniboxPart::RESULTS_TEXT_INVISIBLE;
     }
-    render_text->ApplyColor(result_view_->GetColor(part), current_range);
+    render_text_->ApplyColor(result_view_->GetColor(part), current_range);
   }
-  return render_text;
-}
 
-void OmniboxTextView::OnPaint(gfx::Canvas* canvas) {
-  View::OnPaint(canvas);
-
-  if (!render_text_) {
-    return;
-  }
-  render_text_->SetDisplayRect(GetContentsBounds());
-  render_text_->Draw(canvas);
-}
-
-void OmniboxTextView::Dim() {
-  render_text_->SetColor(
-      result_view_->GetColor(OmniboxPart::RESULTS_TEXT_DIMMED));
-}
-
-void OmniboxTextView::SetText(const base::string16& text,
-                              const ACMatchClassifications& classifications) {
-  render_text_.reset();
-  render_text_ = CreateClassifiedRenderText(text, classifications);
-  UpdateLineHeight();
-}
-
-void OmniboxTextView::SetText(const base::string16& text) {
-  render_text_.reset();
-  render_text_ = CreateRenderText(text);
   UpdateLineHeight();
 }
 
 void OmniboxTextView::SetText(const SuggestionAnswer::ImageLine& line) {
   wrap_text_lines_ = line.num_text_lines() > 1;
   render_text_.reset();
-  render_text_ = CreateText(line);
-  UpdateLineHeight();
-}
-
-std::unique_ptr<gfx::RenderText> OmniboxTextView::CreateText(
-    const SuggestionAnswer::ImageLine& line) const {
-  std::unique_ptr<gfx::RenderText> destination =
-      CreateRenderText(base::string16());
+  render_text_ = CreateRenderText(base::string16());
   if (!OmniboxFieldTrial::IsNewAnswerLayoutEnabled()) {
     // This assumes that the first text type in the line can be used to specify
     // the font for all the text fields in the line.  For now this works but
     // eventually it may be necessary to get RenderText to support multiple font
     // sizes or use multiple RenderTexts.
-    destination->SetFontList(GetFontForType(line.text_fields()[0].type()));
+    render_text_->SetFontList(GetFontForType(line.text_fields()[0].type()));
   }
 
   for (const SuggestionAnswer::TextField& text_field : line.text_fields())
-    AppendText(destination.get(), text_field.text(), text_field.type());
+    AppendText(text_field.text(), text_field.type());
   if (!line.text_fields().empty()) {
     constexpr int kMaxDisplayLines = 3;
     const SuggestionAnswer::TextField& first_field = line.text_fields().front();
     if (first_field.has_num_lines() && first_field.num_lines() > 1 &&
-        destination->MultilineSupported()) {
-      destination->SetMultiline(true);
-      destination->SetMaxLines(
+        render_text_->MultilineSupported()) {
+      render_text_->SetMultiline(true);
+      render_text_->SetMaxLines(
           std::min(kMaxDisplayLines, first_field.num_lines()));
     }
   }
+
+  // Add the "additional" and "status" text from |line|, if any.
+  AppendExtraText(line);
+
+  UpdateLineHeight();
+}
+
+void OmniboxTextView::AppendExtraText(const SuggestionAnswer::ImageLine& line) {
   const base::char16 space(' ');
   const auto* text_field = line.additional_text();
   if (text_field) {
-    AppendText(destination.get(), space + text_field->text(),
-               text_field->type());
+    AppendText(space + text_field->text(), text_field->type());
   }
   text_field = line.status_text();
   if (text_field) {
-    AppendText(destination.get(), space + text_field->text(),
-               text_field->type());
+    AppendText(space + text_field->text(), text_field->type());
   }
-  return destination;
 }
 
-void OmniboxTextView::AppendText(gfx::RenderText* destination,
-                                 const base::string16& text,
-                                 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) {
-      AppendTextHelper(destination, text.substr(begin), text_type, false);
-      break;
-    }
-    AppendTextHelper(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;
-    AppendTextHelper(destination, text.substr(begin, end - begin), text_type,
-                     true);
-    begin = end + end_tag.length();
-  }
+int OmniboxTextView::GetLineHeight() const {
+  return font_height_;
+}
+
+std::unique_ptr<gfx::RenderText> OmniboxTextView::CreateRenderText(
+    const base::string16& text) const {
+  auto render_text = gfx::RenderText::CreateHarfBuzzInstance();
+  render_text->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
+  render_text->SetCursorEnabled(false);
+  render_text->SetElideBehavior(gfx::ELIDE_TAIL);
+  render_text->SetFontList(
+      views::style::GetFont(CONTEXT_OMNIBOX_PRIMARY, kTextStyle));
+  render_text->SetText(text);
+  return render_text;
 }
 
-void OmniboxTextView::AppendTextHelper(gfx::RenderText* destination,
-                                       const base::string16& text,
-                                       int text_type,
-                                       bool is_bold) const {
+void OmniboxTextView::AppendText(const base::string16& text,
+                                 int text_type) const {
   if (text.empty())
     return;
-  int offset = destination->text().length();
+  int offset = render_text_->text().length();
   gfx::Range range(offset, offset + text.length());
-  destination->AppendText(text);
+  render_text_->AppendText(text);
   if (OmniboxFieldTrial::IsNewAnswerLayoutEnabled()) {
-    destination->ApplyColor(
+    render_text_->ApplyWeight(gfx::Font::Weight::NORMAL, range);
+    render_text_->ApplyColor(
         result_view_->GetColor(OmniboxPart::RESULTS_TEXT_DIMMED), range);
   } else {
     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->ApplyWeight(
-        is_bold ? gfx::Font::Weight::BOLD : gfx::Font::Weight::NORMAL, range);
-    destination->ApplyColor(result_view_->GetColor(text_style.part), range);
+    // one RenderText.  Maybe with render_text_->SetFontList(...).
+    render_text_->ApplyWeight(gfx::Font::Weight::NORMAL, range);
+    render_text_->ApplyColor(result_view_->GetColor(text_style.part), range);
 
     // Baselines are always aligned under the touch UI. Font sizes change
     // instead.
     if (!ui::MaterialDesignController::IsTouchOptimizedUiEnabled()) {
-      destination->ApplyBaselineStyle(text_style.baseline, range);
+      render_text_->ApplyBaselineStyle(text_style.baseline, range);
     } else if (text_style.touchable_size_delta != 0) {
-      destination->ApplyFontSizeOverride(
+      render_text_->ApplyFontSizeOverride(
           GetFontForType(text_type).GetFontSize(), range);
     }
   }
 }
 
-int OmniboxTextView::GetLineHeight() const {
-  return font_height_;
-}
-
 void OmniboxTextView::UpdateLineHeight() {
   const int height_normal = render_text_->font_list().GetHeight();
   const int height_bold =

chrome/browser/ui/views/omnibox/omnibox_text_view.h

@@ -36,17 +36,20 @@ class OmniboxTextView : public views::View {
   int GetHeightForWidth(int width) const override;
   void OnPaint(gfx::Canvas* canvas) override;
 
-  // Dim the text (i.e. make it gray). This is used for secondary text (so that
-  // the non-dimmed text stands out more).
+  // Dims the text (i.e. makes it gray). This is used for secondary text (so
+  // that the non-dimmed text stands out more).
   void Dim();
 
-  // Creates a RenderText with default rendering for the given |text|. The
+  // Sets the render text with default rendering for the given |text|. The
   // |classifications| are used to style the text. An ImageLine incorporates
   // both the text and the styling.
+  void SetText(const base::string16& text);
   void SetText(const base::string16& text,
                const ACMatchClassifications& classifications);
   void SetText(const SuggestionAnswer::ImageLine& line);
-  void SetText(const base::string16& text);
+
+  // Adds the "additional" and "status" text from |line|, if any.
+  void AppendExtraText(const SuggestionAnswer::ImageLine& line);
 
   // Get the height of one line of text.  This is handy if the view might have
   // multiple lines.
@@ -56,30 +59,13 @@ class OmniboxTextView : public views::View {
   std::unique_ptr<gfx::RenderText> CreateRenderText(
       const base::string16& text) const;
 
-  // Similar to CreateRenderText, but also apply styling (classifications).
-  std::unique_ptr<gfx::RenderText> CreateClassifiedRenderText(
-      const base::string16& text,
-      const ACMatchClassifications& classifications) const;
-
-  // Creates a RenderText with text and styling from the image line.
-  std::unique_ptr<gfx::RenderText> CreateText(
-      const SuggestionAnswer::ImageLine& line) const;
-
-  // Adds |text| to |destination|.  |text_type| is an index into the
+  // Adds |text| to the render text.  |text_type| is an index into the
   // kTextStyles constant defined in the .cc file and is used to style the text,
   // including setting the font size, color, and baseline style.  See the
   // TextStyle struct in the .cc file for more.
-  void AppendText(gfx::RenderText* destination,
-                  const base::string16& text,
-                  int text_type) const;
-
-  // AppendText will break up the |text| into bold and non-bold pieces
-  // and pass each to this helper with the correct |is_bold| value.
-  void AppendTextHelper(gfx::RenderText* destination,
-                        const base::string16& text,
-                        int text_type,
-                        bool is_bold) const;
+  void AppendText(const base::string16& text, int text_type) const;
 
+  // Updates the cached maximum line height.
   void UpdateLineHeight();
 
   // To get color values.