Type conversion fixes, text rendering edition.

Type conversion fixes, text rendering edition.

This is mostly to fix MSVC warnings about possible value truncation.

BUG=81439
TEST=none

Committed: https://crrev.com/bc91db51bd9df5f29ebfe7940691016716caf5a0
Cr-Commit-Position: refs/heads/master@{#300550}

[Peter Kasting, 2014-10-17 01:10:36]

pkasting@chromium.org changed reviewers:
+ msw@chromium.org

[Peter Kasting, 2014-10-17 01:10:37]

There's a lot of stuff in here that seems sketchy/scary to me; I've highlighted
some of the things that seem most questionable with "!!!" comments, but really
the whole change deserves careful review.

It feels like some functions support floating-point layout, while others don't,
and the interface between the two is basically crowbared together :/

[msw, 2014-10-17 22:13:22]

msw@chromium.org changed reviewers:
+ ckocagil@chromium.org, derat@chromium.org

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

Yeah, float support in RenderText is lacking, we'll ultimately want widespread
float support for text, decoration, and display_rect dimensions. +Dan and Cem,
who have been working in this area.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas.cc#newcode99
ui/gfx/canvas.cc:99: *width = static_cast<int>(std::ceil(fractional_width));
nit q: DCHECK when the int type can't support the float value?
(afaict, static_cast's overflow behavior here is impl-defined)

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas_skia.cc#newcod...
ui/gfx/canvas_skia.cc:177: Rect rect(static_cast<int>(*width), INT_MAX);
Hmm, it seems silly to cast float->int->float for ElideRectangle text. Perhaps
just construct the Rect after ElideRectangle, casting float->int there (until
[Update]RenderText supports RectF)? (ditto nit q: DCHECK there?)

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas_skia.cc#newcod...
ui/gfx/canvas_skia.cc:207: Rect rect(static_cast<int>(*width),
static_cast<int>(*height));
ditto nit q: DCHECK here?

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:727: return GetStringSize();
Cool, I hadn't seen the gfx::Size operator SizeF() before.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:863: // !!! Should this round or ceil?
Dan or Cem may have opinions here; we ultimately want float support everywhere.
For now, I'd naively suggest to ceil, expanding the content width to include
partially filled pixels, unless that breaks something, or conflicts with recent
eliding width treatment: https://codereview.chromium.org/631363002

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:79: uint16_t glyph =
static_cast<uint16_t>(codepoint);
ditto nit q: DCHECK against uint32_t -> uint16_t overflow?

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:940: // !!! Should this be SkScalarRoundToInt, or
SkScalarCeilToInt?
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.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1180: run->glyphs[i] =
static_cast<uint16>(infos[i].codepoint);
ditto nit q: DCHECK against uint32_t -> uint16_t overflow?

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1183:
SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].x_offset));
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?

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:832: SkScalarFloorToInt(start_pos.x()),
text_offset.y(),
Like my other DrawDecorations suggestion, perhaps this should round?

https://codereview.chromium.org/660953002/diff/1/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/text_elider.cc#newcod...
ui/gfx/text_elider.cc:215: // !!! Should this be ToEnclosingRect?
This seems related to Dan's recent fix
<https://codereview.chromium.org/631363002> and Cem's recent test
<codereview.chromium.org/652573002>.

I guess your current ToEnclosedRect use matches the pre-existing implicit
float->int cast behavior (floor), right? On the other hand ToEnclosingRect seems
better for now, so that eliding with the exact float witdh of text doesn't cause
any actual elision? (seems like that case was handled by Dan's recent fix)

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

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:863: // !!! Should this round or ceil?
On 2014/10/17 22:13:23, msw wrote:
> Dan or Cem may have opinions here; we ultimately want float support
everywhere.
> For now, I'd naively suggest to ceil, expanding the content width to include
> partially filled pixels, unless that breaks something, or conflicts with
recent
> eliding width treatment: https://codereview.chromium.org/631363002 

i have no opinion here.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:940: // !!! Should this be SkScalarRoundToInt, or
SkScalarCeilToInt?
On 2014/10/17 22:13:23, msw wrote:
> 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.

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

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

PTAL.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas.cc#newcode99
ui/gfx/canvas.cc:99: *width = static_cast<int>(std::ceil(fractional_width));
On 2014/10/17 22:13:23, msw wrote:
> nit q: DCHECK when the int type can't support the float value?

Changed to ToCeiledInt() for these, which saturates.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/canvas_skia.cc#newcod...
ui/gfx/canvas_skia.cc:177: Rect rect(static_cast<int>(*width), INT_MAX);
On 2014/10/17 22:13:23, msw wrote:
> Hmm, it seems silly to cast float->int->float for ElideRectangle text. Perhaps
> just construct the Rect after ElideRectangle, casting float->int there (until
> [Update]RenderText supports RectF)?

Done.

> (ditto nit q: DCHECK there?)

Used ClampToInt() here and below, which saturates.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text.cc#newcod...
ui/gfx/render_text.cc:863: // !!! Should this round or ceil?
On 2014/10/17 22:13:23, msw wrote:
> Dan or Cem may have opinions here; we ultimately want float support
everywhere.
> For now, I'd naively suggest to ceil, expanding the content width to include
> partially filled pixels, unless that breaks something, or conflicts with
recent
> eliding width treatment: https://codereview.chromium.org/631363002 

It seems like ceiling here is a different behavior than the truncating the CL
you link does.  I can change this, but the inconsistency worries me.  I'm not
sure how to test whether this is really the right thing.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:79: uint16_t glyph =
static_cast<uint16_t>(codepoint);
On 2014/10/17 22:13:23, msw wrote:
> ditto nit q: DCHECK against uint32_t -> uint16_t overflow?

This was checked above (line 74).

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:940: // !!! Should this be SkScalarRoundToInt, or
SkScalarCeilToInt?
On 2014/10/17 22:13:23, msw wrote:
> 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.

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

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1180: run->glyphs[i] =
static_cast<uint16>(infos[i].codepoint);
On 2014/10/17 22:13:23, msw wrote:
> ditto nit q: DCHECK against uint32_t -> uint16_t overflow?

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

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1183:
SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].x_offset));
On 2014/10/17 22:13:23, msw wrote:
> 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?

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.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:832: SkScalarFloorToInt(start_pos.x()),
text_offset.y(),
On 2014/10/17 22:13:23, msw wrote:
> Like my other DrawDecorations suggestion, perhaps this should round?

Done.

https://codereview.chromium.org/660953002/diff/1/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/660953002/diff/1/ui/gfx/text_elider.cc#newcod...
ui/gfx/text_elider.cc:215: // !!! Should this be ToEnclosingRect?
On 2014/10/17 22:13:23, msw wrote:
> This seems related to Dan's recent fix
> <https://codereview.chromium.org/631363002> and Cem's recent test
> <codereview.chromium.org/652573002>.
> 
> I guess your current ToEnclosedRect use matches the pre-existing implicit
> float->int cast behavior (floor), right? On the other hand ToEnclosingRect
seems
> better for now, so that eliding with the exact float witdh of text doesn't
cause
> any actual elision? (seems like that case was handled by Dan's recent fix)

OK, I'll use ToEnclosingRect().

[msw, 2014-10-21 02:05:27]

lgtm

[Peter Kasting, 2014-10-21 02:14:18]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:17:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660953002/40001

[commit-bot: I haz the power, 2014-10-21 03:55:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 03:55:08]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Kasting, 2014-10-21 19:15:37]

As discussed over IM, I reverted the SetDisplayOffset() behavior change due to
it breaking a test.

[Peter Kasting, 2014-10-21 19:15:48]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-21 19:17:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660953002/60001

[commit-bot: I haz the power, 2014-10-21 20:35:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 20:36:58]

Patchset 4 (id:??) landed as
https://crrev.com/bc91db51bd9df5f29ebfe7940691016716caf5a0
Cr-Commit-Position: refs/heads/master@{#300550}