Type conversion fixes, text rendering edition.

ui/gfx/render_text_harfbuzz.cc

 // Copyright 2014 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.
 
 #include "ui/gfx/render_text_harfbuzz.h"
 
 #include <limits>
 #include <map>
 
 #include "base/i18n/bidi_line_iterator.h"
@@ skipping 58 matching lines @@
 // |paint|'s font.
 void GetGlyphWidthAndExtents(SkPaint* paint,
                              hb_codepoint_t codepoint,
                              hb_position_t* width,
                              hb_glyph_extents_t* extents) {
   DCHECK_LE(codepoint, 0xFFFFU);
   paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding);
 
   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)
     *width = SkiaScalarToHarfBuzzPosition(sk_width);
   if (extents) {
     // Invert y-axis because Skia is y-grows-down but we set up HarfBuzz to be
     // y-grows-up.
     extents->x_bearing = SkiaScalarToHarfBuzzPosition(sk_bounds.fLeft);
     extents->y_bearing = SkiaScalarToHarfBuzzPosition(-sk_bounds.fTop);
     extents->width = SkiaScalarToHarfBuzzPosition(sk_bounds.width());
@@ skipping 176 matching lines @@
   hb_face_t* get() {
     return face_;
   }
 
  private:
   hb_face_t* face_;
 };
 
 // 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;
 
   // TODO(ckocagil): This shouldn't grow indefinitely. Maybe use base::MRUCache?
   static std::map<SkFontID, FaceCache> face_caches;
 
   FaceCache* face_cache = &face_caches[skia_face->uniqueID()];
   if (face_cache->first.get() == NULL)
     face_cache->first.Init(skia_face);
@@ skipping 588 matching lines @@
 
     for (size_t i = 0; i < runs_.size(); ++i) {
       const internal::TextRunHarfBuzz& run = *runs_[visual_to_logical_[i]];
       internal::LineSegment segment;
       segment.x_range = Range(current_x, current_x + run.width);
       segment.char_range = run.range;
       segment.run = i;
       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);
 
       lines[0].size.set_width(lines[0].size.width() + run.width);
       lines[0].size.set_height(std::max(lines[0].size.height(),
                                         metrics.fDescent - metrics.fAscent));
       lines[0].baseline = std::max(lines[0].baseline,
                                    SkScalarRoundToInt(-metrics.fAscent));
     }
 
     set_lines(&lines);
   }
 }
 
 void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) {
   DCHECK(!needs_layout_);
   internal::SkiaTextRenderer renderer(canvas);
   ApplyFadeEffects(&renderer);
   ApplyTextShadows(&renderer);
   ApplyCompositionAndSelectionStyles();
 
   int current_x = 0;
   const Vector2d line_offset = GetLineOffset(0);
   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());
 
     Vector2d origin = line_offset + Vector2d(current_x, lines()[0].baseline);
     scoped_ptr<SkPoint[]> positions(new SkPoint[run.glyph_count]);
     for (size_t j = 0; j < run.glyph_count; ++j) {
       positions[j] = run.positions[j];
       positions[j].offset(SkIntToScalar(origin.x()), SkIntToScalar(origin.y()));
     }
 
     for (BreakList<SkColor>::const_iterator it =
              colors().GetBreak(run.range.start());
          it != colors().breaks().end() && it->first < run.range.end();
          ++it) {
       const Range intersection = colors().GetRange(it).Intersect(run.range);
       const Range colored_glyphs = run.CharRangeToGlyphRange(intersection);
       // The range may be empty if a portion of a multi-character grapheme is
       // selected, yielding two colors for a single glyph. For now, this just
       // paints the glyph with a single style, but it should paint it twice,
       // clipped according to selection bounds. See http://crbug.com/366786
       if (colored_glyphs.is_empty())
         continue;
 
       renderer.SetForegroundColor(it->second);
       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);
     }
 
     current_x += run.width;
   }
 
   renderer.EndDiagonalStrike();
 
   UndoCompositionAndSelectionStyles();
@@ skipping 185 matching lines @@
   skia::RefPtr<SkTypeface> skia_face =
       internal::CreateSkiaTypeface(font_family, run->font_style);
   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;
   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,
   // and the resulting glyph data.
   hb_buffer_t* buffer = hb_buffer_create();
   hb_buffer_add_utf16(buffer, reinterpret_cast<const uint16*>(text.c_str()),
                       text.length(), run->range.start(), run->range.length());
   hb_buffer_set_script(buffer, ICUScriptToHBScript(run->script));
   hb_buffer_set_direction(buffer,
       run->is_rtl ? HB_DIRECTION_RTL : HB_DIRECTION_LTR);
   // TODO(ckocagil): Should we determine the actual language?
   hb_buffer_set_language(buffer, hb_language_get_default());
 
   // Shape the text.
   hb_shape(harfbuzz_font, buffer, NULL, 0);
 
   // Populate the run fields with the resulting glyph data in the buffer.
   unsigned int glyph_count = 0;
   hb_glyph_info_t* infos = hb_buffer_get_glyph_infos(buffer, &glyph_count);
   run->glyph_count = glyph_count;
   hb_glyph_position_t* hb_positions =
       hb_buffer_get_glyph_positions(buffer, NULL);
   run->glyphs.reset(new uint16[run->glyph_count]);
   run->glyph_to_char.resize(run->glyph_count);
   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)
       run->width = std::floor(run->width + 0.5f);
 #endif
   }
 
   hb_buffer_destroy(buffer);
   hb_font_destroy(harfbuzz_font);
   return true;
 }
 
 }  // namespace gfx