CC should use TimeTicks and TimeDelta to represent time

CC:Animations should use TimeTicks and TimeDelta to represent time

BUG=178171

[Sikugu_, 2014-04-08 15:13:32]

PTAL..

[ajuma, 2014-04-08 15:44:16]

Thanks for working on this!

Please make the corresponding changes to test code, and make sure that tests
build and pass (in particular, build and run cc_unittests).

It might be easier to break this up into smaller chunks (e.g. remove doubles
from LayerAnimationController's API in one CL, work on Animation in a different
CL, and so on.)

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

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.cc#ne...
cc/animation/animation.cc:105: double monotonic_time = (time -
base::TimeTicks()).InSecondsF();
Instead of converting to double here, how about using a TimeTicks for
pause_time_ and a TimeDelta for total_paused_time?

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.cc#ne...
cc/animation/animation.cc:150: double monotonic_time = (time -
base::TimeTicks()).InSecondsF();
Can we avoid the conversion here by making sure that all the variables used by
the comparison are either TimeTicks or TimeDelta?

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

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.h#new...
cc/animation/animation.h:72: double start_time() const {
We should make this return a TimeTicks instead of a double.

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.h#new...
cc/animation/animation.h:79: return !!(start_time_ -
base::TimeTicks()).InSecondsF();
return start_time_ != base::TimeTicks()

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.h#new...
cc/animation/animation.h:83: return (time_offset_ -
base::TimeTicks()).InSecondsF();
It seems like time_offset_ should be a TimeDelta, and this function should
return a TimeDelta too.

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.h#new...
cc/animation/animation.h:85: void set_time_offset(base::TimeTicks
monotonic_time) {
TimeDelta?

https://codereview.chromium.org/226543005/diff/1/cc/animation/animation.h#new...
cc/animation/animation.h:170: base::TimeTicks time_offset_;
TimeDelta seems more appropriate for an offset.

https://codereview.chromium.org/226543005/diff/1/cc/animation/layer_animation...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/226543005/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.cc:48: (time_offset +
active_animations_[i]->start_time()) *
We should change time_offset to a TimeDelta, and then we won't have a double
that needs converting here.

https://codereview.chromium.org/226543005/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.cc:131: DCHECK(time);
Instead of converting to double, just DCHECK(monotonic_time !=
base::TimeTicks())

https://codereview.chromium.org/226543005/diff/1/cc/input/page_scale_animation.h
File cc/input/page_scale_animation.h (right):

https://codereview.chromium.org/226543005/diff/1/cc/input/page_scale_animatio...
cc/input/page_scale_animation.h:68: double end_time() const { return start_time_
+ duration_; }
Should these be changed to TimeTicks/TimeDelta too?

[piman, 2014-04-08 17:23:46]

lgtm

[enne (OOO), 2014-04-11 14:41:09]

https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h
File cc/animation/animation.h (right):

https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h...
cc/animation/animation.h:73: double ticks_inseconds(base::TimeTicks base_time)
const {
I don't think this merits a member function.  Just call
base::TimeDelta(base_time).InSecondsF() where you need it.  In most cases,
you're subtracting one TimeTicks from another resulting in a TimeDelta, so you
don't even need the TimeDelta object.

https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h...
cc/animation/animation.h:76: double delta_inseconds(base::TimeDelta delta) const
{
This doesn't need to be a member function either.  Just call InSecondsF on the
delta directly.

https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h...
cc/animation/animation.h:184: base::TimeTicks total_paused_time_;
total_paused_time_ seems like it should be a delta and not ticks.

https://codereview.chromium.org/226543005/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/226543005/diff/20001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1121: layer_animation_controller_->PauseAnimation(
Can you push this up another layer to the WebLayerImpl, so that cc is at least
all dealing in base::Time* and the conversion happens in the compositor
bindings?

[Sikugu_, 2014-04-11 15:58:11]

On 2014/04/11 14:41:09, enne wrote:
> https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h
> File cc/animation/animation.h (right):
> 
>
https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h...
> cc/animation/animation.h:73: double ticks_inseconds(base::TimeTicks base_time)
> const {
> I don't think this merits a member function.  Just call
> base::TimeDelta(base_time).InSecondsF() where you need it.  In most cases,
> you're subtracting one TimeTicks from another resulting in a TimeDelta, so you
> don't even need the TimeDelta object.
> 
>
https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h...
> cc/animation/animation.h:76: double delta_inseconds(base::TimeDelta delta)
const
> {
> This doesn't need to be a member function either.  Just call InSecondsF on the
> delta directly.
> 
>
https://codereview.chromium.org/226543005/diff/20001/cc/animation/animation.h...
> cc/animation/animation.h:184: base::TimeTicks total_paused_time_;
> total_paused_time_ seems like it should be a delta and not ticks.
> 
> https://codereview.chromium.org/226543005/diff/20001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/226543005/diff/20001/cc/layers/layer.cc#newco...
> cc/layers/layer.cc:1121: layer_animation_controller_->PauseAnimation(
> Can you push this up another layer to the WebLayerImpl, so that cc is at least
> all dealing in base::Time* and the conversion happens in the compositor
> bindings?

Hi enne, thanks for the comments,
i actually started with one more patch where i will take care of all your
comments.
https://codereview.chromium.org/231133002/

Thanks.