Type conversion fixes, ui/gfx/ edition.

Type conversion fixes, ui/gfx/ edition.

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

BUG=81439
TEST=none

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

[Peter Kasting, 2014-10-17 01:26:04]

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

[Peter Kasting, 2014-10-17 01:26:05]

There's one place I wasn't sure the existing code was correct, which I marked
with a "!!!" comment.  (There might be other incorrect places, but I'm not
knowledgeable enough for any to stick out.)

[Peter Kasting, 2014-10-17 23:13:40]

New version up which fixes test failure in patch set 1 by continuing to use
double precision in one place instead of converting down to float.

What this tells me is that a lot of this code is _extremely_ sensitive to
floating-point precision, and some of the other changes I've made may not be
correct even if they do pass all the tests.

Please think carefully about the precision effects of this change.

[danakj, 2014-10-18 18:40:06]

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return static_cast<int>(
Can you ToFlooredInt here instead of cast+std::floor?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:121: uint8_t centroid[3];
huh, these should be _ suffix'd..

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:541: U8CPU r = SkColorGetR(c);
uint8?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:573: U8CPU r = SkColorGetR(c);
uint8?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:81: return base::saturated_cast<unsigned char>(
nice change here

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:202: SkAlpha alpha = SkColorGetA(color);
why not uint8?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/display.cc#newcod...
ui/gfx/display.cc:156: // !!! Should this use ToRoundedPoint() instead?
It looks like the size gets floored:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/display/displa...

So I'd expect this to match that to avoid a logical gap between monitors.

+oshima we should have unit tests for this?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/geometry/insets.h
File ui/gfx/geometry/insets.h (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/geometry/insets.h...
ui/gfx/geometry/insets.h:37: return InsetsF(static_cast<float>(top()),
static_cast<float>(left()),
i really wanna remove these int->float operators, I think they're evil. after i
argued to keep them before :/

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/interpolated_tran...
File ui/gfx/interpolated_transform.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/interpolated_tran...
ui/gfx/interpolated_transform.cc:277: result.Translate3d(ValueBetween(t,
static_cast<float>(start_pos_.x()),
isn't start_pos_ floats already?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/interpolate...

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/interpolated_tran...
ui/gfx/interpolated_transform.cc:337:
to_pivot.Translate(SkScalarToMScalar(SkIntToScalar(-pivot.x())),
yo dawg, i heard you like static casts :/

we should have an SkIntToMScalar added to skia. can you file a bug and/or send
them a patch to add such a thing? and TODO here to point to the bug to use that?

also related: https://code.google.com/p/skia/issues/detail?id=3040

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/skia_util.cc#newc...
ui/gfx/skia_util.cc:177: SkAlpha alpha = SkGetPackedA32(pixel_in);
uint8?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform.cc#newc...
ui/gfx/transform.cc:519: SkMScalar p[4] =
{SkScalarToMScalar(SkIntToScalar(point->x())),
ditto with TODO?

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc
File ui/gfx/transform_util.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:152: matrix.set3x3(SK_MScalar1 - two * (y * y + z * z),
i think you should downcast to float after the computation is done for each of
these components and keep the x/y/z/w as doubles. this is definitely a change of
behaviour otherwise

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:422: std::sqrt(SkDoubleToMScalar(std::max(1.0 + row00 -
row11 - row22, 0.0)));
do the double to mscalar after taking the sqrt

[Peter Kasting, 2014-10-20 23:38:56]

Thanks for the thorough review; PTAL.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return static_cast<int>(
On 2014/10/18 18:40:06, danakj wrote:
> Can you ToFlooredInt here instead of cast+std::floor?

I can do even better and use ToRoundedInt() to eliminate the 0.5+ as well.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:121: uint8_t centroid[3];
On 2014/10/18 18:40:06, danakj wrote:
> huh, these should be _ suffix'd..

Done.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:541: U8CPU r = SkColorGetR(c);
On 2014/10/18 18:40:06, danakj wrote:
> uint8?

Done.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_analysis.cc...
ui/gfx/color_analysis.cc:573: U8CPU r = SkColorGetR(c);
On 2014/10/18 18:40:06, danakj wrote:
> uint8?

Done.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:202: SkAlpha alpha = SkColorGetA(color);
On 2014/10/18 18:40:06, danakj wrote:
> why not uint8?

SkAlpha is typedefed to uint8, so they should be the same; using an SkAlpha to
hold an alpha value felt semantically clearer to me.  I'll change it if there's
some strong reason to.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/display.cc
File ui/gfx/display.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/display.cc#newcod...
ui/gfx/display.cc:156: // !!! Should this use ToRoundedPoint() instead?
On 2014/10/18 18:40:06, danakj wrote:
> It looks like the size gets floored:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/display/displa...
> 
> So I'd expect this to match that to avoid a logical gap between monitors.

Acknowledged.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/geometry/insets.h
File ui/gfx/geometry/insets.h (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/geometry/insets.h...
ui/gfx/geometry/insets.h:37: return InsetsF(static_cast<float>(top()),
static_cast<float>(left()),
On 2014/10/18 18:40:06, danakj wrote:
> i really wanna remove these int->float operators, I think they're evil. after
i
> argued to keep them before :/

I don't have an opinion, other than that I'd rather not do it in this patch
unless none have any callers.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/interpolated_tran...
File ui/gfx/interpolated_transform.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/interpolated_tran...
ui/gfx/interpolated_transform.cc:277: result.Translate3d(ValueBetween(t,
static_cast<float>(start_pos_.x()),
On 2014/10/18 18:40:06, danakj wrote:
> isn't start_pos_ floats already?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/interpolate...

Yes -- I think this got changed since the time I originally made this fix
locally.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/interpolated_tran...
ui/gfx/interpolated_transform.cc:337:
to_pivot.Translate(SkScalarToMScalar(SkIntToScalar(-pivot.x())),
On 2014/10/18 18:40:06, danakj wrote:
> yo dawg, i heard you like static casts :/
> 
> we should have an SkIntToMScalar added to skia. can you file a bug and/or send
> them a patch to add such a thing? and TODO here to point to the bug to use
that?

Filed https://code.google.com/p/skia/issues/detail?id=3045 .  Added TODO.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/skia_util.cc#newc...
ui/gfx/skia_util.cc:177: SkAlpha alpha = SkGetPackedA32(pixel_in);
On 2014/10/18 18:40:06, danakj wrote:
> uint8?

Same reply as before; they're equivalent but SkAlpha seems clearer.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform.cc#newc...
ui/gfx/transform.cc:519: SkMScalar p[4] =
{SkScalarToMScalar(SkIntToScalar(point->x())),
On 2014/10/18 18:40:06, danakj wrote:
> ditto with TODO?

Done.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc
File ui/gfx/transform_util.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:152: matrix.set3x3(SK_MScalar1 - two * (y * y + z * z),
On 2014/10/18 18:40:06, danakj wrote:
> i think you should downcast to float after the computation is done for each of
> these components and keep the x/y/z/w as doubles. this is definitely a change
of
> behaviour otherwise

Done.  It might be nice to have a test that would fail on the earlier version of
my change (but I don't know that I can write one myself).

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:422: std::sqrt(SkDoubleToMScalar(std::max(1.0 + row00 -
row11 - row22, 0.0)));
On 2014/10/18 18:40:06, danakj wrote:
> do the double to mscalar after taking the sqrt

Done.

[Peter Kasting, 2014-10-21 00:53:56]

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
Note: This causes test failures in TweenTest.LinearIntValueBetweenNegative
because the rounding behavior is changed in my patch from "round toward positive
infinity" to "round away from 0".  "Round away from 0" matches e.g. std::round()
and seems more intuitive to me, so I'm inclined to change the test expectations,
but let me know what you think should be done.

[Peter Kasting, 2014-10-22 22:27:43]

Ping danakj

[danakj, 2014-10-23 15:20:41]

danakj@chromium.org changed reviewers:
+ ajuma@chromium.org

[danakj, 2014-10-23 15:20:42]

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/21 00:53:56, Peter Kasting wrote:
> Note: This causes test failures in TweenTest.LinearIntValueBetweenNegative
> because the rounding behavior is changed in my patch from "round toward
positive
> infinity" to "round away from 0".  "Round away from 0" matches e.g.
std::round()
> and seems more intuitive to me, so I'm inclined to change the test
expectations,
> but let me know what you think should be done.

We should have ajuma decide the right thing here, it may be speced.

[danakj, 2014-10-23 15:35:44]

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:202: SkAlpha alpha = SkColorGetA(color);
On 2014/10/20 23:38:56, Peter Kasting wrote:
> On 2014/10/18 18:40:06, danakj wrote:
> > why not uint8?
> 
> SkAlpha is typedefed to uint8, so they should be the same; using an SkAlpha to
> hold an alpha value felt semantically clearer to me.  I'll change it if
there's
> some strong reason to.

uint8 is a familiar type and we all know what it is. SkAlpha is familiar to less
folks so I like using it less if it's the same thing.

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/geometry/insets.h
File ui/gfx/geometry/insets.h (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/geometry/insets.h...
ui/gfx/geometry/insets.h:37: return InsetsF(static_cast<float>(top()),
static_cast<float>(left()),
On 2014/10/20 23:38:56, Peter Kasting wrote:
> On 2014/10/18 18:40:06, danakj wrote:
> > i really wanna remove these int->float operators, I think they're evil.
after
> i
> > argued to keep them before :/
> 
> I don't have an opinion, other than that I'd rather not do it in this patch
> unless none have any callers.

Sorry ya I didn't mean in this patch, just griping aloud about the existence of
these :)

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc
File ui/gfx/transform_util.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/transform_util.cc...
ui/gfx/transform_util.cc:152: matrix.set3x3(SK_MScalar1 - two * (y * y + z * z),
On 2014/10/20 23:38:56, Peter Kasting wrote:
> On 2014/10/18 18:40:06, danakj wrote:
> > i think you should downcast to float after the computation is done for each
of
> > these components and keep the x/y/z/w as doubles. this is definitely a
change
> of
> > behaviour otherwise
> 
> Done.  It might be nice to have a test that would fail on the earlier version
of
> my change (but I don't know that I can write one myself).

I agree, tho I'm not sure either :) +awoloszyn FYI

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/geometry/rect.h#n...
ui/gfx/geometry/rect.h:225: int x = ToFlooredInt(rect.x() * x_scale);
i had written these purposefully as static_casts instead of ToFloored/CeiledInt
because those functions do clamping stuff to make sure the result fits in an
int, whereas this method is going from int inputs to int outputs, so I wanted to
rely on the caller making sure things fit instead.

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/interpolated_tran...
File ui/gfx/interpolated_transform.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/interpolated_tran...
ui/gfx/interpolated_transform.cc:334: // TODO(danakj): When
https://code.google.com/p/skia/issues/detail?id=3045 is
Cool! Looks like SkIntToMScalar exists now?

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/transform.cc#newc...
ui/gfx/transform.cc:519: // TODO(danakj): When
https://code.google.com/p/skia/issues/detail?id=3045 is
Can use SkIntToMScalar now?

[ajuma, 2014-10-23 15:44:39]

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/23 15:20:42, danakj wrote:
> On 2014/10/21 00:53:56, Peter Kasting wrote:
> > Note: This causes test failures in TweenTest.LinearIntValueBetweenNegative
> > because the rounding behavior is changed in my patch from "round toward
> positive
> > infinity" to "round away from 0".  "Round away from 0" matches e.g.
> std::round()
> > and seems more intuitive to me, so I'm inclined to change the test
> expectations,
> > but let me know what you think should be done.
> 
> We should have ajuma decide the right thing here, it may be speced.

Rounding towards positive infinity is indeed part of the CSS transitions spec.
See http://www.w3.org/TR/css3-transitions/#animatable-types

[Peter Kasting, 2014-10-23 17:29:45]

I don't want to be obstinate here, but I'd like to discuss some of these more...

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:202: SkAlpha alpha = SkColorGetA(color);
On 2014/10/23 15:35:44, danakj wrote:
> On 2014/10/20 23:38:56, Peter Kasting wrote:
> > On 2014/10/18 18:40:06, danakj wrote:
> > > why not uint8?
> > 
> > SkAlpha is typedefed to uint8, so they should be the same; using an SkAlpha
to
> > hold an alpha value felt semantically clearer to me.  I'll change it if
> there's
> > some strong reason to.
> 
> uint8 is a familiar type and we all know what it is. SkAlpha is familiar to
less
> folks so I like using it less if it's the same thing.

Sorry, I forgot the main rationale here when I replied before, or I would have
mentioned it then.  This value is passed to HSLToSkColor() below, which takes an
SkAlpha, not a uint8.  I think we should ensure those types match.

(Also, if someone is writing Skia code, and dealing with color alpha values, and
yet is confused by SkAlpha, I feel like something is wrong...)

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/23 15:44:39, ajuma wrote:
> On 2014/10/23 15:20:42, danakj wrote:
> > On 2014/10/21 00:53:56, Peter Kasting wrote:
> > > Note: This causes test failures in TweenTest.LinearIntValueBetweenNegative
> > > because the rounding behavior is changed in my patch from "round toward
> > positive
> > > infinity" to "round away from 0".  "Round away from 0" matches e.g.
> > std::round()
> > > and seems more intuitive to me, so I'm inclined to change the test
> > expectations,
> > > but let me know what you think should be done.
> > 
> > We should have ajuma decide the right thing here, it may be speced.
> 
> Rounding towards positive infinity is indeed part of the CSS transitions spec.
> See http://www.w3.org/TR/css3-transitions/#animatable-types

This isn't used for CSS transitions, though.  This is only used in Chrome UI
animations.  How does the CSS spec relate?

(Also, if we have a ToRoundedInt() in ui/gfx/ at all, and it rounds away from
zero, aren't we already risking other people rounding in this way?  Are you guys
proposing changing how that method is implemented?)

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/geometry/rect.h#n...
ui/gfx/geometry/rect.h:225: int x = ToFlooredInt(rect.x() * x_scale);
On 2014/10/23 15:35:44, danakj wrote:
> i had written these purposefully as static_casts instead of
ToFloored/CeiledInt
> because those functions do clamping stuff to make sure the result fits in an
> int, whereas this method is going from int inputs to int outputs, so I wanted
to
> rely on the caller making sure things fit instead.

Is that a performance concern?  I originally added casts in here but it made the
code harder to read due to introducing line-wrapping; using these helpers is
very clear.  If you're worried only about clarity, we could simply add a comment
in these functions noting that the clamping portion of the helpers is
unnecessary.  If this is a performance problem, then we can probably cast and
note that it's impossible for the code to need clamping due to the source values
being integral.

[ajuma, 2014-10-23 17:37:11]

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/23 17:29:45, Peter Kasting wrote:
> On 2014/10/23 15:44:39, ajuma wrote:
> > On 2014/10/23 15:20:42, danakj wrote:
> > > On 2014/10/21 00:53:56, Peter Kasting wrote:
> > > > Note: This causes test failures in
TweenTest.LinearIntValueBetweenNegative
> > > > because the rounding behavior is changed in my patch from "round toward
> > > positive
> > > > infinity" to "round away from 0".  "Round away from 0" matches e.g.
> > > std::round()
> > > > and seems more intuitive to me, so I'm inclined to change the test
> > > expectations,
> > > > but let me know what you think should be done.
> > > 
> > > We should have ajuma decide the right thing here, it may be speced.
> > 
> > Rounding towards positive infinity is indeed part of the CSS transitions
spec.
> > See http://www.w3.org/TR/css3-transitions/#animatable-types
> 
> This isn't used for CSS transitions, though.  This is only used in Chrome UI
> animations.  How does the CSS spec relate?

It's used by cc::FilterOperation when animating CSS filters.

[Peter Kasting, 2014-10-23 17:39:52]

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/23 17:37:11, ajuma wrote:
> On 2014/10/23 17:29:45, Peter Kasting wrote:
> > On 2014/10/23 15:44:39, ajuma wrote:
> > > On 2014/10/23 15:20:42, danakj wrote:
> > > > On 2014/10/21 00:53:56, Peter Kasting wrote:
> > > > > Note: This causes test failures in
> TweenTest.LinearIntValueBetweenNegative
> > > > > because the rounding behavior is changed in my patch from "round
toward
> > > > positive
> > > > > infinity" to "round away from 0".  "Round away from 0" matches e.g.
> > > > std::round()
> > > > > and seems more intuitive to me, so I'm inclined to change the test
> > > > expectations,
> > > > > but let me know what you think should be done.
> > > > 
> > > > We should have ajuma decide the right thing here, it may be speced.
> > > 
> > > Rounding towards positive infinity is indeed part of the CSS transitions
> spec.
> > > See http://www.w3.org/TR/css3-transitions/#animatable-types
> > 
> > This isn't used for CSS transitions, though.  This is only used in Chrome UI
> > animations.  How does the CSS spec relate?
> 
> It's used by cc::FilterOperation when animating CSS filters.

I see...

I'll change this; in the future it would be awesome if we could add comments
documenting subtle things like how this piece of ui/ is actually used for web
content, and that there's a spec that controls its behavior.  (I'll add such a
comment here.)

[ajuma, 2014-10-23 17:42:15]

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/23 17:39:52, Peter Kasting wrote:
> On 2014/10/23 17:37:11, ajuma wrote:
> > On 2014/10/23 17:29:45, Peter Kasting wrote:
> > > On 2014/10/23 15:44:39, ajuma wrote:
> > > > On 2014/10/23 15:20:42, danakj wrote:
> > > > > On 2014/10/21 00:53:56, Peter Kasting wrote:
> > > > > > Note: This causes test failures in
> > TweenTest.LinearIntValueBetweenNegative
> > > > > > because the rounding behavior is changed in my patch from "round
> toward
> > > > > positive
> > > > > > infinity" to "round away from 0".  "Round away from 0" matches e.g.
> > > > > std::round()
> > > > > > and seems more intuitive to me, so I'm inclined to change the test
> > > > > expectations,
> > > > > > but let me know what you think should be done.
> > > > > 
> > > > > We should have ajuma decide the right thing here, it may be speced.
> > > > 
> > > > Rounding towards positive infinity is indeed part of the CSS transitions
> > spec.
> > > > See http://www.w3.org/TR/css3-transitions/#animatable-types
> > > 
> > > This isn't used for CSS transitions, though.  This is only used in Chrome
UI
> > > animations.  How does the CSS spec relate?
> > 
> > It's used by cc::FilterOperation when animating CSS filters.
> 
> I see...
> 
> I'll change this; in the future it would be awesome if we could add comments
> documenting subtle things like how this piece of ui/ is actually used for web
> content, and that there's a spec that controls its behavior.  (I'll add such a
> comment here.)

FWIW, the .h file has such a comment :)  But it's true that having a comment
beside the implementation would help to avoid future confusion.

[danakj, 2014-10-23 17:43:23]

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/649203003/diff/20001/ui/gfx/color_utils.cc#ne...
ui/gfx/color_utils.cc:202: SkAlpha alpha = SkColorGetA(color);
On 2014/10/23 17:29:45, Peter Kasting wrote:
> On 2014/10/23 15:35:44, danakj wrote:
> > On 2014/10/20 23:38:56, Peter Kasting wrote:
> > > On 2014/10/18 18:40:06, danakj wrote:
> > > > why not uint8?
> > > 
> > > SkAlpha is typedefed to uint8, so they should be the same; using an
SkAlpha
> to
> > > hold an alpha value felt semantically clearer to me.  I'll change it if
> > there's
> > > some strong reason to.
> > 
> > uint8 is a familiar type and we all know what it is. SkAlpha is familiar to
> less
> > folks so I like using it less if it's the same thing.
> 
> Sorry, I forgot the main rationale here when I replied before, or I would have
> mentioned it then.  This value is passed to HSLToSkColor() below, which takes
an
> SkAlpha, not a uint8.  I think we should ensure those types match.

I think we rely on SkColorGet[ARGB]() to return a byte in many places in the
code base so it's not something we could just change. 


HSLToSkColor eventually ends up in SkColorSetARGBInline() which takes a uint8_t
for the alpha value, so it's just converting the SkAlpha back. I think
HSLToSkColor should really use a uint8_t also, that'd be simpler to
read/understand.

But I don't feel strongly really it's my preference here so go with what you
like, I don't mean to block the rest of the changes over this.

> (Also, if someone is writing Skia code, and dealing with color alpha values,
and
> yet is confused by SkAlpha, I feel like something is wrong...)

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.cc
File ui/gfx/animation/tween.cc (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/animation/tween.c...
ui/gfx/animation/tween.cc:153: return ToRoundedInt(DoubleValueBetween(value,
start, target));
On 2014/10/23 17:29:45, Peter Kasting wrote:
> On 2014/10/23 15:44:39, ajuma wrote:
> > On 2014/10/23 15:20:42, danakj wrote:
> > > On 2014/10/21 00:53:56, Peter Kasting wrote:
> > > > Note: This causes test failures in
TweenTest.LinearIntValueBetweenNegative
> > > > because the rounding behavior is changed in my patch from "round toward
> > > positive
> > > > infinity" to "round away from 0".  "Round away from 0" matches e.g.
> > > std::round()
> > > > and seems more intuitive to me, so I'm inclined to change the test
> > > expectations,
> > > > but let me know what you think should be done.
> > > 
> > > We should have ajuma decide the right thing here, it may be speced.
> > 
> > Rounding towards positive infinity is indeed part of the CSS transitions
spec.
> > See http://www.w3.org/TR/css3-transitions/#animatable-types
> 
> This isn't used for CSS transitions, though.  This is only used in Chrome UI
> animations.  How does the CSS spec relate?
> 
> (Also, if we have a ToRoundedInt() in ui/gfx/ at all, and it rounds away from
> zero, aren't we already risking other people rounding in this way?  Are you
guys
> proposing changing how that method is implemented?)

I'm not proposing that. ToRoundedInt should work like round(). But in this case
round() is not the right behaviour due to spec.

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/649203003/diff/60001/ui/gfx/geometry/rect.h#n...
ui/gfx/geometry/rect.h:225: int x = ToFlooredInt(rect.x() * x_scale);
On 2014/10/23 17:29:45, Peter Kasting wrote:
> On 2014/10/23 15:35:44, danakj wrote:
> > i had written these purposefully as static_casts instead of
> ToFloored/CeiledInt
> > because those functions do clamping stuff to make sure the result fits in an
> > int, whereas this method is going from int inputs to int outputs, so I
wanted
> to
> > rely on the caller making sure things fit instead.
> 
> Is that a performance concern?  I originally added casts in here but it made
the
> code harder to read due to introducing line-wrapping; using these helpers is
> very clear.  If you're worried only about clarity, we could simply add a
comment
> in these functions noting that the clamping portion of the helpers is
> unnecessary.  If this is a performance problem, then we can probably cast and
> note that it's impossible for the code to need clamping due to the source
values
> being integral.

I think we should cast unless we can show there is no negative performance
change by using the helpers. The testing methodology I used in
https://code.google.com/p/chromium/issues/detail?id=407444 would be appropriate
here too if you want to do that.

[Peter Kasting, 2014-11-03 20:28:56]

New snap up:

* Changed Tween::LinearIntValueBetween() back to round-toward-positive-infinity
and added a comment about why.
* ScaleToEnclos{ing,ed}Rect() now cast + DCHECK that the cast is safe.
* Used SkIntToMScalar() where possible

[danakj, 2014-11-03 20:35:26]

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:22: #include "ui/gfx/geometry/safe_integer_conversions.h"
unused?

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:231:
DCHECK(base::IsValueInRangeForNumericType<int>(scaled_x));
nit: it might be easier to read if you just moved all these DCHECKs to the top
of the method and just duplicate the math in them?

i.e.
DCHECK(base::IsValue...Type<int>(rect.x() * x_scale);
DCHECK(base::IsValue...Type<int>(rect.y() * y_scale);
DCHECK(base::IsValue...Type<int>(rect.right() * x_scale);
DCHECK(base::IsValue...Type<int>(rect.bottom() * y_scale);
...actual code...

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/interpolated_tra...
File ui/gfx/interpolated_transform.cc (right):

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/interpolated_tra...
ui/gfx/interpolated_transform.cc:334: // TODO(danakj): When
https://code.google.com/p/skia/issues/detail?id=3045 is
The TODO can go?

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/transform.cc#new...
ui/gfx/transform.cc:519: // TODO(danakj): When
https://code.google.com/p/skia/issues/detail?id=3045 is
The TODO can go?

[Peter Kasting, 2014-11-03 22:19:55]

PTAL.

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:22: #include "ui/gfx/geometry/safe_integer_conversions.h"
On 2014/11/03 20:35:26, danakj wrote:
> unused?

Oops, thanks.

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:231:
DCHECK(base::IsValueInRangeForNumericType<int>(scaled_x));
On 2014/11/03 20:35:26, danakj wrote:
> nit: it might be easier to read if you just moved all these DCHECKs to the top
> of the method and just duplicate the math in them?
> 
> i.e.
> DCHECK(base::IsValue...Type<int>(rect.x() * x_scale);
> DCHECK(base::IsValue...Type<int>(rect.y() * y_scale);
> DCHECK(base::IsValue...Type<int>(rect.right() * x_scale);
> DCHECK(base::IsValue...Type<int>(rect.bottom() * y_scale);
> ...actual code...

Done.

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/interpolated_tra...
File ui/gfx/interpolated_transform.cc (right):

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/interpolated_tra...
ui/gfx/interpolated_transform.cc:334: // TODO(danakj): When
https://code.google.com/p/skia/issues/detail?id=3045 is
On 2014/11/03 20:35:26, danakj wrote:
> The TODO can go?

Done.

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/transform.cc
File ui/gfx/transform.cc (right):

https://codereview.chromium.org/649203003/diff/100001/ui/gfx/transform.cc#new...
ui/gfx/transform.cc:519: // TODO(danakj): When
https://code.google.com/p/skia/issues/detail?id=3045 is
On 2014/11/03 20:35:26, danakj wrote:
> The TODO can go?

Done.

[danakj, 2014-11-04 19:57:01]

Thanks! LGTM!

[Peter Kasting, 2014-11-04 20:23:43]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-11-04 20:24:49]

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

[commit-bot: I haz the power, 2014-11-04 22:30:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-04 22:31:35]

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