Define accelerated steps time function.

Define accelerated steps time function.

This patch is the chromium side. It defines a StepsTimingFunction
class in the compositor. For that, it also creates a Steps class
in ui/gfx/geometry. The patch also implements a new add method
in the animation_curve_impl classes.

BUG=229405
R=ajuma@chromium.org

[a.renevier, 2014-01-24 23:20:37]

forgot to add parenthesis (previous patch didn't build)

[a.renevier, 2014-01-24 23:20:38]

forgot to add parenthesis (previous patch didn't build)

[a.renevier, 2014-01-24 23:20:39]

forgot to add parenthesis (previous patch didn't build)

[a.renevier, 2014-01-24 23:20:40]

forgot to add parenthesis (previous patch didn't build)

[ajuma, 2014-02-21 19:05:08]

Thanks for working on this! This is looking pretty good, just a few nits.

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
File cc/animation/timing_function.h (right):

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
cc/animation/timing_function.h:59: static scoped_ptr<StepsTimingFunction>
Create(int steps, bool stepsAtStart);
nit: steps_at_start

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
cc/animation/timing_function.h:67: StepsTimingFunction(int steps, bool
stepsAtStart);
nit: steps_at_start

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
cc/animation/timing_function.h:70: bool stepsAtStart_;
nit: steps_at_start_

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
File cc/animation/timing_function_unittest.cc (right):

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
cc/animation/timing_function_unittest.cc:4: 
Note that this file has been deleted since you posted this (the tests were moved
to ui/gfx/cubic_bezier_unittest.cc), so you'll need to recreate this file with
just your new tests.

https://codereview.chromium.org/140253013/diff/290001/cc/animation/timing_fun...
cc/animation/timing_function_unittest.cc:146: EXPECT_NEAR(function->GetValue(0),
0, epsilon);
We might be able to just use EXPECT_FLOAT_EQ here, since, unlike with cubic
bezier math above, the numbers should be pretty well behaved.

https://codereview.chromium.org/140253013/diff/290001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_filter_animation_curve_impl.h
(right):

https://codereview.chromium.org/140253013/diff/290001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_filter_animation_curve_impl.h:34: int
steps, bool stepsAtStart);
nit: steps_at_start

https://codereview.chromium.org/140253013/diff/290001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_float_animation_curve_impl.h
(right):

https://codereview.chromium.org/140253013/diff/290001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_float_animation_curve_impl.h:32: int
steps, bool stepsAtStart);
steps_at_start

https://codereview.chromium.org/140253013/diff/290001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_transform_animation_curve_impl.h
(right):

https://codereview.chromium.org/140253013/diff/290001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_transform_animation_curve_impl.h:35: int
steps, bool stepsAtStart);
steps_at_start

[a.renevier, 2014-02-24 18:47:11]

Thanks for the review.

Also, since my first iteration of the patch, the logic for bezier curve has been
moved to ui/gfx/geometry
https://chromium.googlesource.com/chromium/src.git/+/2b58524da5241f175121a4b7...
Should I do the same, and put the logic for the steps function in
ui/gfx/geometry too?

If not, should I recreate a file cc/animation/timing_function_unittest.cc to
test for the steps function?

[ajuma, 2014-02-24 18:56:08]

On 2014/02/24 18:47:11, a.renevier wrote:
> Thanks for the review.
> 
> Also, since my first iteration of the patch, the logic for bezier curve has
been
> moved to ui/gfx/geometry
>
https://chromium.googlesource.com/chromium/src.git/+/2b58524da5241f175121a4b7...
> Should I do the same, and put the logic for the steps function in
> ui/gfx/geometry too?

Yes, for the sake of consistency, I think doing that makes sense.

[ajuma, 2014-02-24 21:11:16]

Just a few more comments.

https://codereview.chromium.org/140253013/diff/420001/cc/animation/keyframed_...
File cc/animation/keyframed_animation_curve_unittest.cc (right):

https://codereview.chromium.org/140253013/diff/420001/cc/animation/keyframed_...
cc/animation/keyframed_animation_curve_unittest.cc:443: EXPECT_FLOAT_EQ(0.25f,
curve->GetValue(0.25));
0.25f

https://codereview.chromium.org/140253013/diff/420001/cc/animation/keyframed_...
cc/animation/keyframed_animation_curve_unittest.cc:445: EXPECT_FLOAT_EQ(0.75f,
curve->GetValue(0.75));
0.75f

https://codereview.chromium.org/140253013/diff/420001/cc/animation/keyframed_...
cc/animation/keyframed_animation_curve_unittest.cc:446: EXPECT_FLOAT_EQ(1.f,
curve->GetValue(1.f));
It'd be good to also test values that don't land exactly on a step, e.g. 0.74f.

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.cc
File ui/gfx/geometry/steps.cc (right):

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.c...
ui/gfx/geometry/steps.cc:1: // Copyright 2014 Samsung. All rights reserved.
"The Chromium Authors"

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.c...
ui/gfx/geometry/steps.cc:21: double Steps::Solve(double x) const {
x should be clamped to [0, 1] (so that, e.g., Solve(-4.0) is equivalent to
Solve(0.0)).

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.c...
ui/gfx/geometry/steps.cc:26: *min = static_cast<float>(steps_at_start_) /
static_cast<float>(steps_);
Use doubles instead of floats here (since *min is a double).

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.h
File ui/gfx/geometry/steps.h (right):

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.h...
ui/gfx/geometry/steps.h:1: // Copyright 2014 Samsung. All rights reserved.
I believe this should be "The Chromium Authors" (see
http://www.chromium.org/developers/coding-style#TOC-File-headers). Note also
that you'll need to sign the CLA (see
http://dev.chromium.org/developers/contributing-code/external-contributor-che...)
and add yourself to the AUTHORS file.

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps.h...
ui/gfx/geometry/steps.h:21: // Sets |min| and |max| to the bezier's minimum and
maximium y values in the
s/bezier/step function/

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps_u...
File ui/gfx/geometry/steps_unittest.cc (right):

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps_u...
ui/gfx/geometry/steps_unittest.cc:16: EXPECT_DOUBLE_EQ(function.Solve(0), 1.0);
Shouldn't this be 0.0?

https://codereview.chromium.org/140253013/diff/420001/ui/gfx/geometry/steps_u...
ui/gfx/geometry/steps_unittest.cc:30: EXPECT_DOUBLE_EQ(function.Solve(0.75),
0.81764);
Shouldn't this be 0.75, not 0.81764?

[a.renevier, 2014-09-15 23:49:18]

updated patch: fixes reviewer comments + rebase against ToT

[a.renevier, 2014-09-15 23:49:52]

a.renevier@samsung.com changed reviewers:
+ danakj@chromium.org

[danakj, 2014-09-16 00:16:22]

https://codereview.chromium.org/140253013/diff/460001/ui/gfx/geometry/steps.h
File ui/gfx/geometry/steps.h (right):

https://codereview.chromium.org/140253013/diff/460001/ui/gfx/geometry/steps.h...
ui/gfx/geometry/steps.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
This seems like it is an animation thing not a geometry thing. Is there a better
home for this?

[a.renevier, 2014-09-16 00:22:34]

On 2014/09/16 00:16:22, danakj wrote:
> https://codereview.chromium.org/140253013/diff/460001/ui/gfx/geometry/steps.h
> File ui/gfx/geometry/steps.h (right):
> 
>
https://codereview.chromium.org/140253013/diff/460001/ui/gfx/geometry/steps.h...
> ui/gfx/geometry/steps.h:1: // Copyright 2014 The Chromium Authors. All rights
> reserved.
> This seems like it is an animation thing not a geometry thing. Is there a
better
> home for this?

I did put it here because that's where CubicBezier is.
If Steps moves somewhere else, should CubicBezier also move?

[danakj, 2014-09-16 00:24:15]

On 2014/09/16 00:22:34, a.renevier wrote:
> On 2014/09/16 00:16:22, danakj wrote:
> >
https://codereview.chromium.org/140253013/diff/460001/ui/gfx/geometry/steps.h
> > File ui/gfx/geometry/steps.h (right):
> > 
> >
>
https://codereview.chromium.org/140253013/diff/460001/ui/gfx/geometry/steps.h...
> > ui/gfx/geometry/steps.h:1: // Copyright 2014 The Chromium Authors. All
rights
> > reserved.
> > This seems like it is an animation thing not a geometry thing. Is there a
> better
> > home for this?
> 
> I did put it here because that's where CubicBezier is.
> If Steps moves somewhere else, should CubicBezier also move?

Ya, probably both should move to ui/gfx/animation/

[a.renevier, 2014-09-16 01:00:50]

updated patch: put Steps and CubicBezier in ui/gfx/animation

[ajuma, 2014-09-16 14:50:15]

https://codereview.chromium.org/140253013/diff/480001/cc/animation/timing_fun...
File cc/animation/timing_function.h (right):

https://codereview.chromium.org/140253013/diff/480001/cc/animation/timing_fun...
cc/animation/timing_function.h:67: // with non bezier curves make sense?
ScrollOffsetAnimationCurves with non-bezier curves make sense (though we don't
have any current use cases).

Defining Velocity to be 0 for a StepsTimingFunction (as you've done) seems
reasonable.

[a.renevier, 2014-09-16 17:33:30]

updated patch: remove question about velocity

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

On 2014/09/16 17:33:30, a.renevier wrote:
> updated patch: remove question about velocity

I'm implementing the cc steps timing function here:
https://codereview.chromium.org/809523004