Type conversion fixes, text rendering edition.

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 442199a6d7f69b837645513e8f4b722b37f1df93..416dfc4ca5842087e9b2e4dfea4717558720ab47 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -76,7 +76,7 @@ void GetGlyphWidthAndExtents(SkPaint* paint,
 
   SkScalar sk_width;
   SkRect sk_bounds;
-  uint16_t glyph = codepoint;
+  uint16_t glyph = static_cast<uint16_t>(codepoint);

[msw, 2014/10/17 22:13:23]

ditto nit q: DCHECK against uint32_t -> uint16_t overflow?

[Peter Kasting, 2014/10/21 01:20:45]

This was checked above (line 74).

 
   paint->getTextWidths(&glyph, sizeof(glyph), &sk_width, &sk_bounds);
   if (width)
@@ -273,7 +273,7 @@ class HarfBuzzFace {
 
 // Creates a HarfBuzz font from the given Skia face and text size.
 hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face,
-                              int text_size,
+                              SkScalar text_size,
                               const FontRenderParams& params,
                               bool background_is_transparent) {
   typedef std::pair<HarfBuzzFace, GlyphCache> FaceCache;
@@ -882,7 +882,7 @@ void RenderTextHarfBuzz::EnsureLayout() {
       lines[0].segments.push_back(segment);
 
       paint.setTypeface(run.skia_face.get());
-      paint.setTextSize(run.font_size);
+      paint.setTextSize(SkIntToScalar(run.font_size));
       SkPaint::FontMetrics metrics;
       paint.getFontMetrics(&metrics);
 
@@ -909,7 +909,7 @@ void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) {
   for (size_t i = 0; i < runs_.size(); ++i) {
     const internal::TextRunHarfBuzz& run = *runs_[visual_to_logical_[i]];
     renderer.SetTypeface(run.skia_face.get());
-    renderer.SetTextSize(run.font_size);
+    renderer.SetTextSize(SkIntToScalar(run.font_size));
     renderer.SetFontRenderParams(run.render_params,
                                  background_is_transparent());
 
@@ -937,9 +937,11 @@ void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) {
       renderer.DrawPosText(&positions[colored_glyphs.start()],
                            &run.glyphs[colored_glyphs.start()],
                            colored_glyphs.length());
-      int width = (colored_glyphs.end() == run.glyph_count ? run.width :
-              run.positions[colored_glyphs.end()].x()) -
-          run.positions[colored_glyphs.start()].x();
+      // !!! Should this be SkScalarRoundToInt, or SkScalarCeilToInt?

[msw, 2014/10/17 22:13:23]

Again, Dan or Cem might have more informed advice, and we'll eventually want to
support floating point dimensions for text decorations. For now, I'd suggest
rounding, so decorations only extend into mostly-filled pixels.

[Daniel Erat, 2014/10/17 23:20:05]

i'm not sure about this either, but rounding seems okay to me.

[Peter Kasting, 2014/10/21 01:20:45]

After in-person discussion, I fixed bugs in this code and changed the behavior
to round.

+      int width = SkScalarFloorToInt(
+          (colored_glyphs.end() == run.glyph_count ?
+              run.width : run.positions[colored_glyphs.end()].x()) -
+          run.positions[colored_glyphs.start()].x());
       renderer.DrawDecorations(origin.x(), origin.y(), width, run.underline,
                                run.strike, run.diagonal_strike);
     }
@@ -1145,8 +1147,9 @@ bool RenderTextHarfBuzz::ShapeRunWithFont(internal::TextRunHarfBuzz* run,
   query.pixel_size = run->font_size;
   query.style = run->font_style;
   run->render_params = GetFontRenderParams(query, NULL);
-  hb_font_t* harfbuzz_font = CreateHarfBuzzFont(run->skia_face.get(),
-      run->font_size, run->render_params, background_is_transparent());
+  hb_font_t* harfbuzz_font = CreateHarfBuzzFont(
+      run->skia_face.get(), SkIntToScalar(run->font_size), run->render_params,
+      background_is_transparent());
 
   // 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,
@@ -1174,12 +1177,15 @@ bool RenderTextHarfBuzz::ShapeRunWithFont(internal::TextRunHarfBuzz* run,
   run->positions.reset(new SkPoint[run->glyph_count]);
   run->width = 0.0f;
   for (size_t i = 0; i < run->glyph_count; ++i) {
-    run->glyphs[i] = infos[i].codepoint;
+    run->glyphs[i] = static_cast<uint16>(infos[i].codepoint);

[msw, 2014/10/17 22:13:23]

ditto nit q: DCHECK against uint32_t -> uint16_t overflow?

[Peter Kasting, 2014/10/21 01:20:45]

Added a DCHECK.  Used numeric_limits::max here for clarity (and above for
consistency).

     run->glyph_to_char[i] = infos[i].cluster;
-    const int x_offset = SkFixedToScalar(hb_positions[i].x_offset);
-    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);
+    const int x_offset =
+        SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].x_offset));

[msw, 2014/10/17 22:13:23]

Hmm, this looks like it undoes my <https://codereview.chromium.org/440213004>
and may regress <http://crbug.com/391843>; perhaps it ought to be using floor?

[Peter Kasting, 2014/10/21 01:20:46]

Yeah, this is a remnant of how the code looked before your change.

I think |x_offset| and |y_offset| should actually be SkScalars instead of ints. 
This avoids losing precision anywhere in here.  I tested to ensure that (a) the
previous CL's change here is what caused my patch set 1 try failures and (b) the
change I'm proposing passed those same tests.

+    const int y_offset =
+        SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].y_offset));
+    run->positions[i].iset(run->width + x_offset, -y_offset);
+    run->width +=
+        SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].x_advance));
 #if defined(OS_LINUX)
     // Match Pango's glyph rounding logic on Linux.
     if (!run->render_params.subpixel_positioning)