RenderTextHarfBuzz: Set font render parameters in font data functions

ui/gfx/render_text_harfbuzz.cc

Index: ui/gfx/render_text_harfbuzz.cc
diff --git a/ui/gfx/render_text_harfbuzz.cc b/ui/gfx/render_text_harfbuzz.cc
index 5a5bd47b517d391ede69932b1569ffee1ad986e8..5ae9ce98f512752f9dc1e8dbe080e6e1c9da2fb0 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -268,7 +268,9 @@ class HarfBuzzFace {
 };
 
 // Creates a HarfBuzz font from the given Skia face and text size.
-hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face, int text_size) {
+hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face,
+                              int text_size,
+                              const FontRenderParams& params) {
   typedef std::pair<HarfBuzzFace, GlyphCache> FaceCache;
 
   // TODO(ckocagil): This shouldn't grow indefinitely. Maybe use base::MRUCache?
@@ -284,6 +286,8 @@ hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face, int text_size) {
   FontData* hb_font_data = new FontData(&face_cache->second);
   hb_font_data->paint_.setTypeface(skia_face);
   hb_font_data->paint_.setTextSize(text_size);
+  // TODO(ckocagil): Do we need to update these params later?
+  internal::ApplyRenderParams(params, &hb_font_data->paint_);
   hb_font_set_funcs(harfbuzz_font, g_font_funcs.Get().get(), hb_font_data,
                     DeleteByType<FontData>);
   hb_font_make_immutable(harfbuzz_font);
@@ -1112,8 +1116,15 @@ bool RenderTextHarfBuzz::ShapeRunWithFont(internal::TextRunHarfBuzz* run,
   if (skia_face == NULL)
     return false;
   run->skia_face = skia_face;
+  FontRenderParamsQuery query(false);
+  query.families.push_back(font_family);
+  query.pixel_size = run->font_size;
+  query.style = run->font_style;
+  FontRenderParams params = GetFontRenderParams(query, NULL);
+  if (background_is_transparent())
+    params.subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_NONE;

[Daniel Erat, 2014/08/15 18:34:25]

can you make ApplyRenderParams() check whether the background is transparent and
only do this there?

[ckocagil, 2014/08/15 19:05:57]

ApplyRenderParams doesn't know whether the background is transparent because it
isn't a RenderText member method. To make that work we'd have to move it into
RenderText and make it public so that it can be called from
<anon>::CreateHarfBuzzFont.

Another option is to add a "void
RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks transparency
and sets the subpixel setting. We'd then call that whenever we get a
FontRenderParams.

[msw, 2014/08/15 19:21:17]

I wonder if the pattern here is correct, given how different it is from the
other impls. Should we be using gfx::Font for things like GetFontRenderParams?

[ckocagil, 2014/08/15 22:28:01]

gfx::Font (as it currently is) is kinda heavyweight and other than a few things
like GetFontRenderParams, it is completely unnecessary to have in RTHB.

Ideally we'd have something like PlatformFontSkia. Then we'd be able to use
gfx::Font like other impls. Perhaps a TODO will suffice for now?

[msw, 2014/08/16 00:30:11]

Acknowledged; if you see a specific reason to add a TODO, go for it.

   hb_font_t* harfbuzz_font = CreateHarfBuzzFont(run->skia_face.get(),
-                                                run->font_size);
+                                                run->font_size, params);
 
   // Create a HarfBuzz buffer and add the string to be shaped. The HarfBuzz
   // buffer holds our text, run information to be used by the shaping engine,
@@ -1147,6 +1158,7 @@ bool RenderTextHarfBuzz::ShapeRunWithFont(internal::TextRunHarfBuzz* run,
     const int y_offset = SkFixedToScalar(hb_positions[i].y_offset);
     run->positions[i].set(run->width + x_offset, -y_offset);
     run->width += SkFixedToScalar(hb_positions[i].x_advance);
+    run->width = std::floor(run->width + 0.5f);

[msw, 2014/08/15 19:21:17]

Whoa, this seems really bad... This rounds the run width each time a glyph's
width is added. Can we avoid this rounding altogether? If not, can we simply
round the overall run width after this glyph loop? Please explain why any
rounding is needed.

[ckocagil, 2014/08/15 22:28:01]

This rounding is needed to place the glyphs at integer values, which is what
Pango does. And it can't be just floor or ceil, it has to be rounded to the
nearest integer (I tried).

[msw, 2014/08/16 00:30:11]

Yuck, please add a comment then. Would we get the same results disabling
subpixel positioning instead? That might be a better [temporary] solution.

While Pango's glyph positioning does seem to look better in the Linux Desktop
screenshots I posted < http://crbug.com/402715 >, I think this will break
CanvasTestMac.*, and I wonder how it affects the look on Mac, Windows, and
Chrome OS (especially with high-dpi). Can you post native/HarfBuzz screenshots
on any of those OSes, similar to the ones I posted? I guess we really want to
achieve parity first and follow up with potential improvements.

[ckocagil, 2014/08/16 14:01:14]

Subpixel positioning seems to be always disabled on Linux. The problem here is,
Skia's rounding logic doesn't match Pango's, so we have to roll our own.

I fixed this to only do the rounding if subpixel positioning is off. I'll add
the screenshots of different rounding methods to http://crbug.com/402715.

   }
 
   hb_buffer_destroy(buffer);