Use RenderText directly in omnibox_result_view.cc.

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

Index: chrome/browser/ui/views/omnibox/omnibox_result_view.cc
===================================================================
--- chrome/browser/ui/views/omnibox/omnibox_result_view.cc	(revision 146345)
+++ chrome/browser/ui/views/omnibox/omnibox_result_view.cc	(working copy)
@@ -14,6 +14,7 @@
 #include <algorithm>  // NOLINT
 
 #include "base/i18n/bidi_line_iterator.h"
+#include "base/memory/scoped_vector.h"
 #include "chrome/browser/ui/omnibox/omnibox_popup_model.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_result_view_model.h"
@@ -25,6 +26,7 @@
 #include "ui/base/text/text_elider.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/render_text.h"
 
 namespace {
 
@@ -47,6 +49,7 @@
   const gfx::Font* font;
   SkColor color;
   gfx::Size pixel_size;
+  gfx::RenderText* render_text;  // Weak.
 };
 
 // Precalculated data used to draw a complete visual run within the match.
@@ -339,6 +342,7 @@
   if (!bidi_line.Open(text, base::i18n::IsRTL(), is_url))
     return x;
   const int num_runs = bidi_line.CountRuns();
+  ScopedVector<gfx::RenderText> render_texts;
   Runs runs;
   for (int run = 0; run < num_runs; ++run) {
     int run_start_int = 0, run_length_int = 0;
@@ -389,12 +393,21 @@
         current_data->color = GetColor(state, DIMMED_TEXT);
       else
         current_data->color = GetColor(state, force_dim ? DIMMED_TEXT : TEXT);
-      int width = 0;
-      int height = 0;
-      gfx::Canvas::SizeStringInt(current_data->text, *current_data->font,
-                                 &width, &height, gfx::Canvas::NO_ELLIPSIS);
-      current_data->pixel_size = gfx::Size(width, height);
-      current_run->pixel_width += width;
+
+      render_texts.push_back(gfx::RenderText::CreateInstance());
+      current_data->render_text = render_texts.back();
+      current_data->render_text->SetFontList(
+          gfx::FontList(*current_data->font));
+      current_data->render_text->SetText(current_data->text);
+
+      gfx::StyleRange style_range;
+      style_range.foreground = current_data->color;
+      style_range.font_style = current_data->font->GetStyle();
+      current_data->render_text->set_default_style(style_range);
+      current_data->render_text->ApplyDefaultStyle();
+
+      current_data->pixel_size = current_data->render_text->GetStringSize();
+      current_run->pixel_width += current_data->pixel_size.width();
     }
     DCHECK(!current_run->classifications.empty());
   }
@@ -438,39 +451,21 @@
   std::sort(runs.begin(), runs.end(), &SortRunsVisually);
 
   // Draw the runs.
+  const int text_height = GetTextHeight();

[msw, 2012/07/23 18:56:08]

nit: remove unused local variable.

[Alexei Svitkine (slow), 2012/07/23 19:38:12]

Done.

   for (Runs::iterator i(runs.begin()); i != runs.end(); ++i) {
     const bool reverse_visible_order = (i->is_rtl != base::i18n::IsRTL());
-    int flags = gfx::Canvas::NO_ELLIPSIS;  // We've already elided.
-    if (reverse_visible_order) {
+    if (reverse_visible_order)
       std::reverse(i->classifications.begin(), i->classifications.end());
-      if (i->is_rtl)
-        flags |= gfx::Canvas::FORCE_RTL_DIRECTIONALITY;

[Peter Kasting, 2012/07/12 17:51:48]

It worries me that you have no equivalent of this statement.  This seems like a
big red flag for RTL.

[Alexei Svitkine (slow), 2012/07/12 17:58:02]

RenderText will use RTL directionality in the presence of strong RTL characters
in the string. Is there a case where this will not give the results we want?

[Peter Kasting, 2012/07/12 18:05:14]

We may get bad results for URLs with Hebrew characters in them where we render
those sections in reversed order (note how when calculating is_rtl we exclude
the is_url cases).  I don't know how coherent the results of the GetVisualRun()
call on line 357 will be with RenderText's algorithm otherwise so I can't
speculate much.

[msw, 2012/07/23 15:48:12]

I'll write a patch to force base LTR/RTL with a flag (at least for
RenderTextWin), which should be done with Uniscribe settings instead of wrapping
the text with control characters like ui/gfx/canvas_skia.cc.

-    }
     for (Classifications::const_iterator j(i->classifications.begin());
          j != i->classifications.end(); ++j) {
       const int left =
           mirroring_context_->mirrored_left_coord(x, x + j->pixel_size.width());
-      // By passing the same y-coordinate for each run, we vertically align the
-      // tops of successive runs.  This isn't actually what we want; we want to
-      // align the baselines, but Canvas doesn't currently expose text
-      // measurement APIs sufficient to make that happen.  The problem here is
-      // font substitution: if no fonts are substituted, then all runs have the
-      // same font (in bold or normal styles), and thus the same height and same
-      // baseline.  If fonts are substituted within a run, the characters are
-      // baseline-aligned within the run, but using the same top coordinate as
-      // for other runs is only correct if the overall ascent for this run is
-      // the same as for other runs -- that is, if the tallest ascent of all
-      // fonts in the run is equal to the ascent of the normal font.  If this
-      // condition doesn't hold, the baseline for this run will be drawn too
-      // high or too low, depending on whether the run's tallest ascent is
-      // shorter or higher than the normal font's ascent, respectively.
-      //
-      // TODO(asvitkine): Fix this by replacing the SizeStringInt() calls
-      // elsewhere in this file with calls that can calculate actual baselines
-      // even in the face of font fallback. Tracked as: http://crbug.com/128027
-      canvas->DrawStringInt(j->text, *j->font, j->color, left, y,
-                            j->pixel_size.width(), j->pixel_size.height(),
-                            flags);
+      // Align the text runs to a common baseline.
+      const int top =
+          y + normal_font_.GetBaseline() - j->render_text->GetBaseline();
+      gfx::Rect rect(left, top, j->pixel_size.width(), j->pixel_size.height());
+      j->render_text->SetDisplayRect(rect);
+      j->render_text->Draw(canvas);
       x += j->pixel_size.width();
     }
   }
@@ -499,6 +494,7 @@
         // an ellipsis, since there isn't enough room to draw it after this
         // classification.
         j->text += kEllipsis;
+        j->render_text->SetText(j->text);
 
         // We also add this classification's width (sans ellipsis) back to the
         // available width since we want to consider the available space we'll
@@ -533,16 +529,16 @@
         // If we could only fit an ellipsis, then only make it bold if there was
         // an immediate prior classification in this run that was also bold, or
         // it will look orphaned.
-        if ((elided_text.length() == 1) &&
+        if ((j->font != &normal_font_) &&
+            (elided_text.length() == 1) &&

[msw, 2012/07/23 18:56:08]

optional nit: fit on line above.

[Alexei Svitkine (slow), 2012/07/23 19:38:12]

Done.

             (on_first_classification ||
-             (prior_classification->font == &normal_font_)))
+            (prior_classification->font == &normal_font_))) {

[msw, 2012/07/23 18:56:08]

nit: re-indent by one space.

[Alexei Svitkine (slow), 2012/07/23 19:38:12]

Done.

           j->font = &normal_font_;
+          j->render_text->SetFontList(gfx::FontList(*j->font));
+        }
 
-        int width = 0;
-        int height = 0;
-        gfx::Canvas::SizeStringInt(elided_text, *j->font, &width, &height,
-                                   gfx::Canvas::NO_ELLIPSIS);
-        j->pixel_size = gfx::Size(width, height);
+        j->render_text->SetText(elided_text);
+        j->pixel_size = j->render_text->GetStringSize();
 
         // Erase any other classifications that come after the elided one.
         i->classifications.erase(j.base(), i->classifications.end());