CC: Add curve (whole animation) timing function to compositor animations.

CC: Add curve (whole animation) timing function to compositor animations.

For adding web animations with animation timing on compositor.

BUG=417909
Committed: https://crrev.com/ab5e169ebadefcbe3455cbcf5731379e8ef97670
Cr-Commit-Position: refs/heads/master@{#297147}

[ikilpatrick, 2014-09-24 01:39:18]

ikilpatrick@chromium.org changed reviewers:
+ dstockwell@chromium.org, samli@chromium.org

[samli, 2014-09-24 07:55:33]

Comments below.

You should also create a bug for this feature and reference it in the issue.

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

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:32: // Assumes that
(*keyframes).front()->Time() < t < (*keyframes).back()-Time().
Use a DCHECK to assert this assumption

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:33: template <typename KeyframeType>
Is there a reason for renaming from Keyframes to KeyframeType?

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:34: double GetProgress(const
ScopedPtrVector<KeyframeType>* keyframes,
Use references instead

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:34: double GetProgress(const
ScopedPtrVector<KeyframeType>* keyframes,
Can you separate the curve timing function logic to the keyframe timing
function? It's a little confusing now that some of the logic has been shifted
around. eg i being created, passed in and out.

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:38: double t = time;
Unnecessary variable

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:67: std::min(std::max(progress, 0.),
1.));
No need for 0., just use 0

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

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:570:
TEST(KeyframedAnimationCurveTest, CurveAndKeyframeTiming) {
The keyframe doesn't have a specified timing function? Could you add a test
which has both a curve and keyframe timing function?

[ikilpatrick, 2014-09-25 04:33:26]

PTAL at split up function. Not sure on naming etc.

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

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:32: // Assumes that
(*keyframes).front()->Time() < t < (*keyframes).back()-Time().
On 2014/09/24 07:55:32, samli wrote:
> Use a DCHECK to assert this assumption

Acknowledged.

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:33: template <typename KeyframeType>
On 2014/09/24 07:55:32, samli wrote:
> Is there a reason for renaming from Keyframes to KeyframeType?

A lot of this came from deleted GetCurveValue below which had KeyframeType
instead.

Which version is more idiomatic?

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:34: double GetProgress(const
ScopedPtrVector<KeyframeType>* keyframes,
On 2014/09/24 07:55:32, samli wrote:
> Use references instead

Done.

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:34: double GetProgress(const
ScopedPtrVector<KeyframeType>* keyframes,
On 2014/09/24 07:55:32, samli wrote:
> Can you separate the curve timing function logic to the keyframe timing
> function? It's a little confusing now that some of the logic has been shifted
> around. eg i being created, passed in and out.

Done. PTAL.

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:38: double t = time;
On 2014/09/24 07:55:32, samli wrote:
> Unnecessary variable

Done.

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:67: std::min(std::max(progress, 0.),
1.));
On 2014/09/24 07:55:32, samli wrote:
> No need for 0., just use 0

Need to use 0. as template won't match.
"deduced conflicting types for parameter '_Tp' ('double' vs. 'int')"

Need to compare like with like.

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

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:570:
TEST(KeyframedAnimationCurveTest, CurveAndKeyframeTiming) {
On 2014/09/24 07:55:32, samli wrote:
> The keyframe doesn't have a specified timing function? Could you add a test
> which has both a curve and keyframe timing function?

Keyframe: 473-577.
Curve: 581-583.

?

[samli, 2014-09-25 07:03:48]

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

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:33: template <typename KeyframeType>
On 2014/09/25 04:33:25, ikilpatrick wrote:
> On 2014/09/24 07:55:32, samli wrote:
> > Is there a reason for renaming from Keyframes to KeyframeType?
> 
> A lot of this came from deleted GetCurveValue below which had KeyframeType
> instead.
> 
> Which version is more idiomatic?

Keyframe IMO

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:34: double GetProgress(const
ScopedPtrVector<KeyframeType>* keyframes,
On 2014/09/25 04:33:25, ikilpatrick wrote:
> On 2014/09/24 07:55:32, samli wrote:
> > Can you separate the curve timing function logic to the keyframe timing
> > function? It's a little confusing now that some of the logic has been
shifted
> > around. eg i being created, passed in and out.
> 
> Done. PTAL.

Looks good

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve.cc:67: std::min(std::max(progress, 0.),
1.));
On 2014/09/25 04:33:25, ikilpatrick wrote:
> On 2014/09/24 07:55:32, samli wrote:
> > No need for 0., just use 0
> 
> Need to use 0. as template won't match.
> "deduced conflicting types for parameter '_Tp' ('double' vs. 'int')"
> 
> Need to compare like with like.

Ah. I think the correct style for double is 0.0

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

https://codereview.chromium.org/598853003/diff/1/cc/animation/keyframed_anima...
cc/animation/keyframed_animation_curve_unittest.cc:570:
TEST(KeyframedAnimationCurveTest, CurveAndKeyframeTiming) {
On 2014/09/25 04:33:26, ikilpatrick wrote:
> On 2014/09/24 07:55:32, samli wrote:
> > The keyframe doesn't have a specified timing function? Could you add a test
> > which has both a curve and keyframe timing function?
> 
> Keyframe: 473-577.
> Curve: 581-583.
> 
> ?

Yep, missed it sorry.

https://codereview.chromium.org/598853003/diff/20001/cc/animation/keyframed_a...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/598853003/diff/20001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:238: double progress =
GetKeyframeProgress(keyframes_, t, i);
Maybe TransformedAnimationTime and TransformedKeyframeTime? They should be at
least consistent with each other since they are both applying timing functions
to transform the timing input

[ikilpatrick, 2014-09-25 09:01:38]

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

[ikilpatrick, 2014-09-25 09:01:45]

https://codereview.chromium.org/598853003/diff/20001/cc/animation/keyframed_a...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/598853003/diff/20001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:238: double progress =
GetKeyframeProgress(keyframes_, t, i);
On 2014/09/25 07:03:48, samli wrote:
> Maybe TransformedAnimationTime and TransformedKeyframeTime? They should be at
> least consistent with each other since they are both applying timing functions
> to transform the timing input

Renamed to: TransformedAnimationTime & TransformedKeyframeProgress.

Keyframe should be progress (or something) as is fractional, rather than a time.

[ajuma, 2014-09-25 14:44:38]

https://codereview.chromium.org/598853003/diff/40001/cc/animation/keyframed_a...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/598853003/diff/40001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:32: template <typename Keyframe>
I see the naming of this has been discussed already, but it seems confusing to
use 'Keyframe' here when that's already the name of a class. I'd prefer to use
something different, like KeyframeType as you had before.

https://codereview.chromium.org/598853003/diff/40001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:49: double time) {
DCHECK_GE(keyframes.size(), 2ul);

https://codereview.chromium.org/598853003/diff/40001/cc/blink/web_filter_anim...
File cc/blink/web_filter_animation_curve_impl.h (right):

https://codereview.chromium.org/598853003/diff/40001/cc/blink/web_filter_anim...
cc/blink/web_filter_animation_curve_impl.h:39: #if
WEB_ANIMATION_SUPPORTS_ANIMATION_TIMING
Since you're just adding new methods rather than changing existing ones, is the
#if really needed here? (Same question for the #ifs in the other files.)

https://codereview.chromium.org/598853003/diff/40001/cc/blink/web_filter_anim...
cc/blink/web_filter_animation_curve_impl.h:40: virtual void
setTimingFunction(TimingFunctionType = TimingFunctionTypeEase);
No default argument; the one on the Blink side is fine and should be sufficient
(e.g. see WebFilterAnimationCurve::add, which has a default value for
TimingFunctionType, and WebFilterAnimationCurveImpl::add above, which does not).

Same comment for the other files.

[ikilpatrick, 2014-09-25 22:54:59]

https://codereview.chromium.org/598853003/diff/40001/cc/animation/keyframed_a...
File cc/animation/keyframed_animation_curve.cc (right):

https://codereview.chromium.org/598853003/diff/40001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:32: template <typename Keyframe>
On 2014/09/25 14:44:38, ajuma wrote:
> I see the naming of this has been discussed already, but it seems confusing to
> use 'Keyframe' here when that's already the name of a class. I'd prefer to use
> something different, like KeyframeType as you had before.

Done.

https://codereview.chromium.org/598853003/diff/40001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:49: double time) {
On 2014/09/25 14:44:38, ajuma wrote:
> DCHECK_GE(keyframes.size(), 2ul);

Done.

https://codereview.chromium.org/598853003/diff/40001/cc/blink/web_filter_anim...
File cc/blink/web_filter_animation_curve_impl.h (right):

https://codereview.chromium.org/598853003/diff/40001/cc/blink/web_filter_anim...
cc/blink/web_filter_animation_curve_impl.h:39: #if
WEB_ANIMATION_SUPPORTS_ANIMATION_TIMING
On 2014/09/25 14:44:38, ajuma wrote:
> Since you're just adding new methods rather than changing existing ones, is
the
> #if really needed here? (Same question for the #ifs in the other files.)

Done.

https://codereview.chromium.org/598853003/diff/40001/cc/blink/web_filter_anim...
cc/blink/web_filter_animation_curve_impl.h:40: virtual void
setTimingFunction(TimingFunctionType = TimingFunctionTypeEase);
On 2014/09/25 14:44:38, ajuma wrote:
> No default argument; the one on the Blink side is fine and should be
sufficient
> (e.g. see WebFilterAnimationCurve::add, which has a default value for
> TimingFunctionType, and WebFilterAnimationCurveImpl::add above, which does
not).
> 
> Same comment for the other files.

Done.

[ajuma, 2014-09-26 14:10:22]

Thanks, lgtm.

[ikilpatrick, 2014-09-29 07:02:51]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2014-09-29 07:03:38]

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

[commit-bot: I haz the power, 2014-09-29 07:07:49]

Committed patchset #5 (id:80001) as 411c2a1509e95708b33391b163337eaa95a66b43

[commit-bot: I haz the power, 2014-09-29 07:09:49]

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

[loyso (OOO), 2015-01-05 03:33:54]

On 2014/09/29 07:09:49, I haz the power (commit-bot) wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/ab5e169ebadefcbe3455cbcf5731379e8ef97670
> Cr-Commit-Position: refs/heads/master@{#297147}

The blink part of this CL hasn't landed.

See my port of this CL here: https://codereview.chromium.org/814853003/