Define step timing function for the cc animation framework.

Define step timing function for the cc animation framework.

This is a chromium-side part. Blink part: https://codereview.chromium.org/812013002

BUG=441006
R=dstockwell@chromium.org
R=ajuma@chromium.org
TEST=ManualTests/animation/compositor-animation-steps.html

Committed: https://crrev.com/515c5da5528c725f8a73584768dc6d1b22e5fbbc
Cr-Commit-Position: refs/heads/master@{#309399}

[loyso (OOO), 2014-12-17 03:45:11]

On 2014/12/17 03:27:30, loyso wrote:

Chromium part: https://codereview.chromium.org/812013002/

[loyso (OOO), 2014-12-17 07:23:45]

On 2014/12/17 at 03:45:11, loyso wrote:
> On 2014/12/17 03:27:30, loyso wrote:
> 
> Chromium part: https://codereview.chromium.org/812013002/

Sorry, that's a link to the blink part.

[ajuma, 2014-12-17 16:32:56]

This looks pretty good.

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:443: // Tests that a steps
timing function works as expected.
Please add a few words so that it explains how this test is different from the
two other tests below (and do the same for the comments for the other tests).

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:444:
TEST(KeyframedAnimationCurveTest, StepsTimingFunction_StepAtStart) {
Style nit: no underscore in the test name. (Same for the other tests.)

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:457: for (float i = 0; i <
num_steps; i += 1.0f) {
0.f

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:463: EXPECT_FLOAT_EQ(ceil(i)
+ 1, curve->GetValue(time2));
1.f

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:488: EXPECT_FLOAT_EQ(0,
curve->GetValue(base::TimeDelta()));
0.f

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:520: EXPECT_FLOAT_EQ(0,
curve->GetValue(base::TimeDelta()));
0.f

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:521: for (float i = 1; i <=
num_steps; i += 1.0f) {
1.f

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:526: EXPECT_FLOAT_EQ(floor(i)
- 1, curve->GetValue(time1));
1.f

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.cc
File cc/animation/timing_function.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:77: steps_start_offset_ == 0.5);
If we're restricting to these values, let's use the StepAtPosition enum instead
of a float as the second argument.

To plumb the enum from Blink to cc, you'll need a version of the enum in Blink
in public/platform (e.g. in WebCompositorAnimationCurve.h) and you'll need to
convert from the Blink enum to the cc enum in cc/blink (e.g. by adding a
conversion function to web_animation_curve_common.{h, cc}).

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:85: floor((steps_ * t) + steps_start_offset_) /
steps_, 0.0, 1.0);
static_cast<double>(steps_) (rather than having an implicit conversion)

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.h
File cc/animation/timing_function.h (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.h:86: enum StepAtPosition { Start, Middle, End };
This doesn't seem to be used anywhere. Were you planning on using it instead of
steps_start_offset? (It seems nicer to use the enum if these are the only
offsets we need to support.) 

If the enum is going to be used, let's make this an enum class since these are
now allowed (see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc).

[loyso (OOO), 2014-12-18 01:53:06]

A couple of questions. Thanks in advance!

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:457: for (float i = 0; i <
num_steps; i += 1.0f) {
On 2014/12/17 at 16:32:56, ajuma wrote:
> 0.f
Do we have a rationale behind it (any references to style guides)? The type of a
literal and the source type are meaningless (this is not a narrowing
conversion). It bloats the code and introduces extra noise otherwise. Thanks in
advance for any comments :)

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.cc
File cc/animation/timing_function.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:77: steps_start_offset_ == 0.5);
On 2014/12/17 at 16:32:56, ajuma wrote:
> If we're restricting to these values, let's use the StepAtPosition enum
instead of a float as the second argument.
> 
> To plumb the enum from Blink to cc, you'll need a version of the enum in Blink
in public/platform (e.g. in WebCompositorAnimationCurve.h) and you'll need to
convert from the Blink enum to the cc enum in cc/blink (e.g. by adding a
conversion function to web_animation_curve_common.{h, cc}).

My initial implementation was absolutely like that. However, after a
conversation with dstockwell@ we decided to go with a float instead of adding
two more enums.

In general, we want to get rid of redundancy (a timing function must be
implemented twice on both sides, for example). Enums is a good demonstration for
the cc_blink pain we have.

Would be nice to have a separate discussion on this topic. Just want to note:
this CL is a short-term change before a massive refactoring.

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:85: floor((steps_ * t) + steps_start_offset_) /
steps_, 0.0, 1.0);
On 2014/12/17 at 16:32:56, ajuma wrote:
> static_cast<double>(steps_) (rather than having an implicit conversion)

There is a bunch of implicit conversions. steps_ is an integer and return type
is float.
Should I make all of them explicit? Is it mentioned in our style guides?

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.h
File cc/animation/timing_function.h (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.h:86: enum StepAtPosition { Start, Middle, End };
On 2014/12/17 at 16:32:56, ajuma wrote:
> This doesn't seem to be used anywhere. Were you planning on using it instead
of steps_start_offset? (It seems nicer to use the enum if these are the only
offsets we need to support.) 
> 
> If the enum is going to be used, let's make this an enum class since these are
now allowed (see
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc).

I forgot to erase this enum. I had the initial implementation (not presented
here) using enums. See my comment below.

[ajuma, 2014-12-18 02:15:41]

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:457: for (float i = 0; i <
num_steps; i += 1.0f) {
On 2014/12/18 01:53:05, loyso wrote:
> On 2014/12/17 at 16:32:56, ajuma wrote:
> > 0.f
> Do we have a rationale behind it (any references to style guides)? The type of
a
> literal and the source type are meaningless (this is not a narrowing
> conversion). It bloats the code and introduces extra noise otherwise. Thanks
in
> advance for any comments :)

Not sure if it's documented anywhere, but this is the usual practice at least in
cc. It's true that it doesn't change the value of something like 0, but it would
make a difference if the value wasn't exactly representable as a float. Using
float literals everywhere seems more consistent.

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.cc
File cc/animation/timing_function.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:77: steps_start_offset_ == 0.5);
On 2014/12/18 01:53:05, loyso wrote:
> On 2014/12/17 at 16:32:56, ajuma wrote:
> > If we're restricting to these values, let's use the StepAtPosition enum
> instead of a float as the second argument.
> > 
> > To plumb the enum from Blink to cc, you'll need a version of the enum in
Blink
> in public/platform (e.g. in WebCompositorAnimationCurve.h) and you'll need to
> convert from the Blink enum to the cc enum in cc/blink (e.g. by adding a
> conversion function to web_animation_curve_common.{h, cc}).
> 
> My initial implementation was absolutely like that. However, after a
> conversation with dstockwell@ we decided to go with a float instead of adding
> two more enums.

Ok.

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:85: floor((steps_ * t) + steps_start_offset_) /
steps_, 0.0, 1.0);
On 2014/12/18 01:53:05, loyso wrote:
> On 2014/12/17 at 16:32:56, ajuma wrote:
> > static_cast<double>(steps_) (rather than having an implicit conversion)
> 
> There is a bunch of implicit conversions. steps_ is an integer and return type
> is float.
> Should I make all of them explicit? Is it mentioned in our style guides?

Yes, please make them explicit. The style guide talks about casts/conversions
here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting

[loyso (OOO), 2014-12-18 03:17:30]

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:457: for (float i = 0; i <
num_steps; i += 1.0f) {
On 2014/12/18 at 02:15:41, ajuma wrote:
> On 2014/12/18 01:53:05, loyso wrote:
> > On 2014/12/17 at 16:32:56, ajuma wrote:
> > > 0.f
> > Do we have a rationale behind it (any references to style guides)? The type
of a
> > literal and the source type are meaningless (this is not a narrowing
> > conversion). It bloats the code and introduces extra noise otherwise. Thanks
in
> > advance for any comments :)
> 
> Not sure if it's documented anywhere, but this is the usual practice at least
in cc. It's true that it doesn't change the value of something like 0, but it
would make a difference if the value wasn't exactly representable as a float.
Using float literals everywhere seems more consistent.

Ok, I'll make it consistent to cc/ folder. Overall: the usage of integer
literals with no narrowing conversion would reduce our code base significantly.

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.cc
File cc/animation/timing_function.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:85: floor((steps_ * t) + steps_start_offset_) /
steps_, 0.0, 1.0);
On 2014/12/18 at 02:15:41, ajuma wrote:
> On 2014/12/18 01:53:05, loyso wrote:
> > On 2014/12/17 at 16:32:56, ajuma wrote:
> > > static_cast<double>(steps_) (rather than having an implicit conversion)
> > 
> > There is a bunch of implicit conversions. steps_ is an integer and return
type
> > is float.
> > Should I make all of them explicit? Is it mentioned in our style guides?
> 
> Yes, please make them explicit. The style guide talks about casts/conversions
here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting

That link says nothing on implicit vs. explicit conversions. On the contrary,
that expression is a subject for usual arithmetic conversions (via double in
this case) and it reads well without C++-nasty-style explicit casts. Any other
arguments? :)

[ajuma, 2014-12-18 14:32:54]

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function.cc
File cc/animation/timing_function.cc (right):

https://codereview.chromium.org/809523004/diff/1/cc/animation/timing_function...
cc/animation/timing_function.cc:85: floor((steps_ * t) + steps_start_offset_) /
steps_, 0.0, 1.0);
On 2014/12/18 03:17:30, loyso wrote:
> On 2014/12/18 at 02:15:41, ajuma wrote:
> > On 2014/12/18 01:53:05, loyso wrote:
> > > On 2014/12/17 at 16:32:56, ajuma wrote:
> > > > static_cast<double>(steps_) (rather than having an implicit conversion)
> > > 
> > > There is a bunch of implicit conversions. steps_ is an integer and return
> type
> > > is float.
> > > Should I make all of them explicit? Is it mentioned in our style guides?
> > 
> > Yes, please make them explicit. The style guide talks about
casts/conversions
> here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting
> 
> That link says nothing on implicit vs. explicit conversions. On the contrary,
> that expression is a subject for usual arithmetic conversions (via double in
> this case) and it reads well without C++-nasty-style explicit casts. Any other
> arguments? :)

It's a (relatively) common practice in cc. It makes clear that a conversion is
happening, without having to lookup the types of the variables involved.

[ajuma, 2014-12-18 16:12:53]

Latest patch set lgtm.

[dstockwell, 2014-12-19 01:41:21]

lgtm

[loyso (OOO), 2014-12-19 01:57:49]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2014-12-19 02:00:45]

The CQ bit was unchecked by loyso@chromium.org

[loyso (OOO), 2014-12-22 04:44:02]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2014-12-22 04:44:19]

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

[commit-bot: I haz the power, 2014-12-22 06:28:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-22 06:29:53]

Patchset 3 (id:??) landed as
https://crrev.com/515c5da5528c725f8a73584768dc6d1b22e5fbbc
Cr-Commit-Position: refs/heads/master@{#309399}