Make Keyframe use TimeTicks/TimeDelta to represent time instead of double.

Make Keyframe use TimeTicks/TimeDelta to represent time instead of double.

BUG=178171
Committed: https://crrev.com/591d77f964eae64a49bd83910bae574e40f20c1a
Cr-Commit-Position: refs/heads/master@{#304612}

[patro, 2014-11-13 12:50:09]

Patchset #2 (id:20001) has been deleted

[patro, 2014-11-13 12:50:20]

Patchset #1 (id:1) has been deleted

[patro, 2014-11-13 14:25:56]

behara.ms@samsung.com changed reviewers:
+ ajuma@chromium.org, siva.gunturi@samsung.com

[patro, 2014-11-13 14:25:57]

Kindly Review

[ajuma, 2014-11-13 15:45:52]

Thanks, this is off to a good start.

I think we should also change TransformAnimationCurve::GetValue,
ColorAnimationCurve::GetValue, and FloatAnimationCurve::GetValue, and
ScrollOffsetAnimationCurve::GetValue to take TimeDeltas rather than doubles as
part of this CL, to avoid having a mix of doubles and TimeDeltas which ends up
requiring lots of back-and-forth conversion between double and TimeDelta.

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

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:41: (keyframes.back()->Time() -
keyframes.front()->Time()).InSecondsF();
Can we use TimeDelta for these values rather than using InSecondsF to convert to
double? This would probably require also changing |time| to be TimeDelta, since
the next line subtracts |start_time| from |time|.

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:56: if (time < keyframes[i +
1]->Time().InSecondsF())
How about using TimeDelta for |time|, so we don't need to call InSecondsF here?

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:70: (keyframes[i + 1]->Time() -
keyframes[i]->Time()).InSecondsF();
Use TimeDelta for |time|. Then subtraction can be done without calling
InSecondsF. And add a Divide function to TimeUtil that takes two TimeDeltas and
returns their quotient (as a double).

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:228: SkColor
KeyframedColorAnimationCurve::GetValue(double t) const {
Let's use a TimeDelta for |t|.

[patro, 2014-11-14 10:33:13]

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

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:41: (keyframes.back()->Time() -
keyframes.front()->Time()).InSecondsF();
On 2014/11/13 15:45:52, ajuma wrote:
> Can we use TimeDelta for these values rather than using InSecondsF to convert
to
> double? This would probably require also changing |time| to be TimeDelta,
since
> the next line subtracts |start_time| from |time|.

Done.

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:56: if (time < keyframes[i +
1]->Time().InSecondsF())
On 2014/11/13 15:45:52, ajuma wrote:
> How about using TimeDelta for |time|, so we don't need to call InSecondsF
here?

Done.

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:70: (keyframes[i + 1]->Time() -
keyframes[i]->Time()).InSecondsF();
On 2014/11/13 15:45:52, ajuma wrote:
> Use TimeDelta for |time|. Then subtraction can be done without calling
> InSecondsF. And add a Divide function to TimeUtil that takes two TimeDeltas
and
> returns their quotient (as a double).

Done.

https://codereview.chromium.org/719453007/diff/60001/cc/animation/keyframed_a...
cc/animation/keyframed_animation_curve.cc:228: SkColor
KeyframedColorAnimationCurve::GetValue(double t) const {
On 2014/11/13 15:45:51, ajuma wrote:
> Let's use a TimeDelta for |t|.

Done.

[patro, 2014-11-14 11:10:14]

Some unit tests are failing due to lack of precision.

The least possible time unit representable using TimeDelta is
1 MicroSecond = 0.000001 seconds

Example:
In ScrollOffsetAnimationCurveTest.GetValue unit test

curve duration = 745350 MicroSeconds
duration_in_seconds = 0.074535
duration_in_seconds / 2.0 = 0.0372675
time = base::TimeDelta::FromSecondsD(duration_in_seconds/2.0)
time.InSecondsF() = 0.037267

Either we can modify the unit_test to be approximately equal 
or
We have to increase the precision of TimeDelta

Any other ideas to resolve this issue?

[ajuma, 2014-11-14 15:12:02]

On 2014/11/14 11:10:14, patro wrote:
> Some unit tests are failing due to lack of precision.
> 
> The least possible time unit representable using TimeDelta is
> 1 MicroSecond = 0.000001 seconds
> 
> Example:
> In ScrollOffsetAnimationCurveTest.GetValue unit test
> 
> curve duration = 745350 MicroSeconds
> duration_in_seconds = 0.074535
> duration_in_seconds / 2.0 = 0.0372675
> time = base::TimeDelta::FromSecondsD(duration_in_seconds/2.0)
> time.InSecondsF() = 0.037267
> 
> Either we can modify the unit_test to be approximately equal 
> or
> We have to increase the precision of TimeDelta
> 
> Any other ideas to resolve this issue?

Which tests are affected? If it's only tests that involve the duration of a
scroll offset animation, I'm fine with checking for approximate equality (since
the duration of a scroll offset animation is computed based on distance, and can
wind up very "non-round" as a result).

[ajuma, 2014-11-14 15:12:33]

https://codereview.chromium.org/719453007/diff/90001/cc/animation/scroll_offs...
File cc/animation/scroll_offset_animation_curve_unittest.cc (right):

https://codereview.chromium.org/719453007/diff/90001/cc/animation/scroll_offs...
cc/animation/scroll_offset_animation_curve_unittest.cc:102: double
duration_in_seconds = curve->Duration().InSecondsF();
We should just use duration as a TimeDelta rather than converting to a double
here (and then use TimeUtil to scale as needed below).

https://codereview.chromium.org/719453007/diff/90001/cc/blink/web_scroll_offs...
File cc/blink/web_scroll_offset_animation_curve_impl.cc (right):

https://codereview.chromium.org/719453007/diff/90001/cc/blink/web_scroll_offs...
cc/blink/web_scroll_offset_animation_curve_impl.cc:38:
curve_->GetValue(base::TimeDelta::FromSeconds(time));
This should be FromSecondsD.
(this is likely what's causing all the fast/scroll-behavior layout test
failures)

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:170: base::TimeDelta time = monotonic_time -
base::TimeTicks();
The existing code treats time as a value in milliseconds (rather than in
seconds), so this change will have the effect of slowing down the animation by a
factor of 1000 (I think). Do any tests fail (in
top_controls_manager_unittest.cc) because of this? If not, we should add test
coverage (and make sure that the behavior stays the same before and after this
change).

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:202: base::TimeDelta start_time =
gfx::FrameTime::Now() - base::TimeTicks();
Same comment as above.

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:243: base::TimeDelta time_ms = time -
base::TimeTicks();
Here too.

[patro, 2014-11-17 13:26:48]

Kindly Review

Thanks,
Shyam

https://codereview.chromium.org/719453007/diff/90001/cc/animation/scroll_offs...
File cc/animation/scroll_offset_animation_curve_unittest.cc (right):

https://codereview.chromium.org/719453007/diff/90001/cc/animation/scroll_offs...
cc/animation/scroll_offset_animation_curve_unittest.cc:102: double
duration_in_seconds = curve->Duration().InSecondsF();
On 2014/11/14 15:12:33, ajuma wrote:
> We should just use duration as a TimeDelta rather than converting to a double
> here (and then use TimeUtil to scale as needed below).

Done.

https://codereview.chromium.org/719453007/diff/90001/cc/blink/web_scroll_offs...
File cc/blink/web_scroll_offset_animation_curve_impl.cc (right):

https://codereview.chromium.org/719453007/diff/90001/cc/blink/web_scroll_offs...
cc/blink/web_scroll_offset_animation_curve_impl.cc:38:
curve_->GetValue(base::TimeDelta::FromSeconds(time));
On 2014/11/14 15:12:33, ajuma wrote:
> This should be FromSecondsD.
> (this is likely what's causing all the fast/scroll-behavior layout test
> failures)

Done.

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:170: base::TimeDelta time = monotonic_time -
base::TimeTicks();
On 2014/11/14 15:12:33, ajuma wrote:
> The existing code treats time as a value in milliseconds (rather than in
> seconds), so this change will have the effect of slowing down the animation by
a
> factor of 1000 (I think). Do any tests fail (in
> top_controls_manager_unittest.cc) because of this? If not, we should add test
> coverage (and make sure that the behavior stays the same before and after this
> change).

No tests are not failing in top_controls_manager_unittest.cc
This is beacuse existing code treats it as milliseconds. 
So we are creating keyframe in milliseconds and getValue is also called in
milliseconds.

Now in this change we are creating in TimeDelta and getValue is also in
TimeDelta.

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:202: base::TimeDelta start_time =
gfx::FrameTime::Now() - base::TimeTicks();
On 2014/11/14 15:12:33, ajuma wrote:
> Same comment as above.

Done.

https://codereview.chromium.org/719453007/diff/90001/cc/input/top_controls_ma...
cc/input/top_controls_manager.cc:243: base::TimeDelta time_ms = time -
base::TimeTicks();
On 2014/11/14 15:12:33, ajuma wrote:
> Here too.

Done.

[ajuma, 2014-11-17 14:47:20]

Please look into the cc_unittest failures on the bots. The scroll animation
failures look like cases where we need approximate equality (as we discussed
before), but LayerTreeHostAnimationTestAddAnimationWithTimingFunction looks like
a actual failure.

https://codereview.chromium.org/719453007/diff/110001/cc/input/top_controls_m...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/719453007/diff/110001/cc/input/top_controls_m...
cc/input/top_controls_manager.cc:243: base::TimeDelta time_ms = time -
base::TimeTicks();
time_ms isn't a good name anymore. Maybe animation_time?

[patro, 2014-11-18 10:28:32]

Kindly Review

Thanks,
Shyam

https://codereview.chromium.org/719453007/diff/110001/cc/input/top_controls_m...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/719453007/diff/110001/cc/input/top_controls_m...
cc/input/top_controls_manager.cc:243: base::TimeDelta time_ms = time -
base::TimeTicks();
On 2014/11/17 14:47:20, ajuma wrote:
> time_ms isn't a good name anymore. Maybe animation_time?

Done.

[ajuma, 2014-11-18 14:58:14]

Thanks, lgtm with one nit.

https://codereview.chromium.org/719453007/diff/170001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/719453007/diff/170001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:515: 
Please remove this blank line.

[patro, 2014-11-18 15:04:53]

https://codereview.chromium.org/719453007/diff/170001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/719453007/diff/170001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_animation.cc:515: 
On 2014/11/18 14:58:14, ajuma wrote:
> Please remove this blank line.

Done.

[patro, 2014-11-18 15:05:23]

The CQ bit was checked by behara.ms@samsung.com

[commit-bot: I haz the power, 2014-11-18 15:06:04]

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

[commit-bot: I haz the power, 2014-11-18 16:01:48]

Committed patchset #9 (id:190001)

[commit-bot: I haz the power, 2014-11-18 16:03:15]

Patchset 9 (id:??) landed as
https://crrev.com/591d77f964eae64a49bd83910bae574e40f20c1a
Cr-Commit-Position: refs/heads/master@{#304612}