CC::Animations should use TimeTicks & TimeDelta to represent time

CC::LayerAnimationController should use TimeTicks and
TimeDelta to represent time.

As of now complete CC/animation code uses double type
for time durations. This patch replaces  double with
TimeTicks where time stamps is used and TimeDeltas
where offset is used.

BUG=178171
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270718

[Sikugu_, 2014-04-09 16:17:26]

PTAL..
I have split the patch into 2 patches.
This takes care of LayerAnimationController api changes.

Review comments published https://codereview.chromium.org/226543005/ are taken
care in this patch. CL 226543005 takes care of CC:Animations related changes.

[danakj, 2014-04-09 16:33:03]

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.cc:292: 
whitespace?

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.cc:297: 
whitespace?

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.h:137: double
GetTimeTicks(base::TimeTicks time) {
make this a file-static method (or anon namespace) in the .cc file, and maybe
name something like TimeTicksToDoubleTime or something, to say you're not
returning a TimeTicks

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:26: const double
kInitialTickTime = 1.0;
why not change this to a TimeTicks?

we should avoid doing double->timeticks all over the unit tests and just work
with timeticks instead?

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:27: base::TimeTicks
GetTimeTicks(double time) {
blank line above method definition.

[danakj, 2014-04-09 16:34:14]

https://codereview.chromium.org/231133002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/231133002/diff/1/cc/layers/layer.cc#newcode1123
cc/layers/layer.cc:1123: base::TimeTicks::FromInternalValue(time_offset *
Is converting back and forth from double to time ticks going to cause
jitter/error in the time value? Can we change the PauseAnimation method to
TimeTicks here or is that later?

[ajuma, 2014-04-09 17:37:39]

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.h:43: void PauseAnimation(int
animation_id, base::TimeTicks time_offset);
An offset seems more like a TimeDelta (a period of time) rather than a TimeTicks
(a point in time). That said, to avoid the double-to-TimeTicks-back-to-double
issue that Dana pointed out, I think you're better off leaving this as a double
in this CL and changing it to a TimeDelta in the CL where cc::Animation switches
from doubles to TimeTicks/TimeDeltas.

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:26: const double
kInitialTickTime = 1.0;
On 2014/04/09 16:33:03, danakj wrote:
> why not change this to a TimeTicks?
> 
> we should avoid doing double->timeticks all over the unit tests and just work
> with timeticks instead?

+1, please change this to TimeTicks. Note that we disallow static initializers
for non-POD types (at least in non-test code; not sure about the rule in test
code), so you'll need a function that returns a TimeTicks (e.g. base::TimeTicks
InitialTickTime()) rather than just a static TimeTicks.

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:146:
controller->Animate(GetTimeTicks(kInitialTickTime + 0.5));
Offsets (like 0.5 above) should be TimeDeltas.

[Sikugu_, 2014-04-10 14:04:57]

3 scrolloffset tests of layeranimation got failed when running the cc_unittests,
this is because of the jitter created by to (In the unit tests) and fro (while
passing to animations)conversion of double to Timedelta.

3 tests failed:
    LayerAnimationControllerTest.ScrollOffsetTransition
    LayerAnimationControllerTest.ScrollOffsetTransitionNoImplProvider
    LayerAnimationControllerTest.ScrollOffsetTransitionOnImplOnly
Tests took 74 seconds.

I think we have to add the animation code changes as well in this patch for the
tests to work.

How shall we go about this?

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.cc:292: 
On 2014/04/09 16:33:03, danakj wrote:
> whitespace?

Done.

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.cc:297: 
On 2014/04/09 16:33:03, danakj wrote:
> whitespace?

Done.

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.h:43: void PauseAnimation(int
animation_id, base::TimeTicks time_offset);
On 2014/04/09 17:37:39, ajuma wrote:
> An offset seems more like a TimeDelta (a period of time) rather than a
TimeTicks
> (a point in time). That said, to avoid the double-to-TimeTicks-back-to-double
> issue that Dana pointed out, I think you're better off leaving this as a
double
> in this CL and changing it to a TimeDelta in the CL where cc::Animation
switches
> from doubles to TimeTicks/TimeDeltas.

Done.

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller.h:137: double
GetTimeTicks(base::TimeTicks time) {
On 2014/04/09 16:33:03, danakj wrote:
> make this a file-static method (or anon namespace) in the .cc file, and maybe
> name something like TimeTicksToDoubleTime or something, to say you're not
> returning a TimeTicks

Done.

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

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:26: const double
kInitialTickTime = 1.0;
On 2014/04/09 16:33:03, danakj wrote:
> why not change this to a TimeTicks?
> 
> we should avoid doing double->timeticks all over the unit tests and just work
> with timeticks instead?

Done.

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:26: const double
kInitialTickTime = 1.0;
On 2014/04/09 16:33:03, danakj wrote:
> why not change this to a TimeTicks?
> 
> we should avoid doing double->timeticks all over the unit tests and just work
> with timeticks instead?

Few calls of animation functions still use double.
Will take care of this as part of the Animation Changes.

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:27: base::TimeTicks
GetTimeTicks(double time) {
On 2014/04/09 16:33:03, danakj wrote:
> blank line above method definition.

Done.

https://codereview.chromium.org/231133002/diff/1/cc/animation/layer_animation...
cc/animation/layer_animation_controller_unittest.cc:146:
controller->Animate(GetTimeTicks(kInitialTickTime + 0.5));
On 2014/04/09 17:37:39, ajuma wrote:
> Offsets (like 0.5 above) should be TimeDeltas.

Done.

https://codereview.chromium.org/231133002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/231133002/diff/1/cc/layers/layer.cc#newcode1123
cc/layers/layer.cc:1123: base::TimeTicks::FromInternalValue(time_offset *
On 2014/04/09 16:34:15, danakj wrote:
> Is converting back and forth from double to time ticks going to cause
> jitter/error in the time value? Can we change the PauseAnimation method to
> TimeTicks here or is that later?

Keeping this as it is to avoid jitter.
Will take care of this in the Animaitons patch.

[ajuma, 2014-04-10 14:26:59]

On 2014/04/10 14:04:57, Sikugu_ wrote:
> 3 scrolloffset tests of layeranimation got failed when running the
cc_unittests,
> this is because of the jitter created by to (In the unit tests) and fro (while
> passing to animations)conversion of double to Timedelta.

Is this still a problem if TimeDeltas are created using
base::TimeDelta::FromMilliseconds, rather than by converting a double to a
TimeTicks to a TimeDelta?

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:29: return
GetTimeTicks(offset) - base::TimeTicks();
Instead of this function, how about just using base::TimeDelta::FromMilliseconds
wherever you need to create a TimeDelta? Then this won't involve a double-to-int
conversion.

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:35: const base::TimeTicks
kInitialTickTime = GetTimeTicks(1.0);
Note that this is adding a static intializer. I'd suggesting getting rid of the
GetTimeTicks function altogether, and just having:
base::TimeTicks InitialTickTime() {
  return base::TimeTicks::FromInternalValue(base::Time::kMicrosecondsPerSecond);
}

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:735: double duration =
controller_impl->GetAnimation(
If we convert this duration to a TimeDelta, then we can directly use it below
(when we need to divide it by 2.0 and/or add it to the start time).

[Sikugu_, 2014-04-10 14:32:13]

On 2014/04/10 14:26:59, ajuma wrote:
> On 2014/04/10 14:04:57, Sikugu_ wrote:
> > 3 scrolloffset tests of layeranimation got failed when running the
> cc_unittests,
> > this is because of the jitter created by to (In the unit tests) and fro
(while
> > passing to animations)conversion of double to Timedelta.
> 
> Is this still a problem if TimeDeltas are created using
> base::TimeDelta::FromMilliseconds, rather than by converting a double to a
> TimeTicks to a TimeDelta?

Yes when i tried using TimeDeltas using 
base::TimeDelta::FromMilliseconds , 6 of the tests were failing.

I initially coded the same way, but giving the timeticks as diff solved the
problem.

[ajuma, 2014-04-10 14:41:33]

On 2014/04/10 14:32:13, Sikugu_ wrote:
> On 2014/04/10 14:26:59, ajuma wrote:
> > On 2014/04/10 14:04:57, Sikugu_ wrote:
> > > 3 scrolloffset tests of layeranimation got failed when running the
> > cc_unittests,
> > > this is because of the jitter created by to (In the unit tests) and fro
> (while
> > > passing to animations)conversion of double to Timedelta.
> > 
> > Is this still a problem if TimeDeltas are created using
> > base::TimeDelta::FromMilliseconds, rather than by converting a double to a
> > TimeTicks to a TimeDelta?
> 
> Yes when i tried using TimeDeltas using 
> base::TimeDelta::FromMilliseconds , 6 of the tests were failing.
> 
> I initially coded the same way, but giving the timeticks as diff solved the
> problem.

Using TimeTicks to create a TimeDelta seems wrong though. Do you have more
details about the conversions that are problematic?

[mithro-old, 2014-04-16 00:44:23]

On 2014/04/10 14:41:33, ajuma wrote:
> On 2014/04/10 14:32:13, Sikugu_ wrote:
> > On 2014/04/10 14:26:59, ajuma wrote:
> > > On 2014/04/10 14:04:57, Sikugu_ wrote:
> > > > 3 scrolloffset tests of layeranimation got failed when running the
> > > cc_unittests,
> > > > this is because of the jitter created by to (In the unit tests) and fro
> > (while
> > > > passing to animations)conversion of double to Timedelta.
> > > 
> > > Is this still a problem if TimeDeltas are created using
> > > base::TimeDelta::FromMilliseconds, rather than by converting a double to a
> > > TimeTicks to a TimeDelta?
> > 
> > Yes when i tried using TimeDeltas using 
> > base::TimeDelta::FromMilliseconds , 6 of the tests were failing.
> > 
> > I initially coded the same way, but giving the timeticks as diff solved the
> > problem.
> 
> Using TimeTicks to create a TimeDelta seems wrong though. Do you have more
> details about the conversions that are problematic?

Hi ajuma / danakj / Sikugu_,

Please see my comments on the https://codereview.chromium.org/235233003/ issue.
(It covers a bunch of thoughts about the double conversion issues.)

Thanks!

Tim

[mithro-old, 2014-04-16 13:33:17]

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:23: base::TimeTicks
GetTimeTicks(double time) {
A better name is "double seconds".

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:35: const base::TimeTicks
kInitialTickTime = GetTimeTicks(1.0);
Why didn't you do a 
-----------------------------------
  using base::TimeTicks;

  const TimeTicks kInitialTickTime = TimeTicks::FromInternalValue(1000000);
-----------------------------------

or maybe;
-----------------------------------
  const TimeTicks kInitialTickTime =
TimeTicks::FromInternalValue(base::Time::kMicrosecondsPerSecond);
-----------------------------------

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:103:
controller->Animate(kInitialTickTime + GetTimeDelta(0.5));
Why didn't you just do;
-----------------------------------
  base::TimeTicks::FromMilliseconds(500);
-----------------------------------

[tomhudson, 2014-04-16 14:03:57]

> Why didn't you just do;
> -----------------------------------
>   base::TimeTicks::FromMilliseconds(500);
> -----------------------------------

Every call to FromMilliseconds() requires a floating-point divide.
These have real measurable costs on ARM devices because they are done in
software.

[mithro-old, 2014-04-16 14:13:12]

On 2014/04/16 14:03:57, tomhudson wrote:
> > Why didn't you just do;
> > -----------------------------------
> >   base::TimeTicks::FromMilliseconds(500);
> > -----------------------------------
> 
> Every call to FromMilliseconds() requires a floating-point divide.
> These have real measurable costs on ARM devices because they are done in
> software.

That should have been base::TimeDelta::FromMilliseconds. Doing a
base::TimeDelta::FromMilliseconds(500) does has no floating point nor divides.

// static
inline TimeDelta TimeDelta::FromMilliseconds(int64 ms) {
  // Preserve max to prevent overflow.
  if (ms == std::numeric_limits<int64>::max())
    return Max();
  return TimeDelta(ms * Time::kMicrosecondsPerMillisecond);
}

[tomhudson, 2014-04-16 14:26:43]

Sorry, you're right, I was thinking of TimeDelta::InMillisecondsF() and
InSecondsF().

[danakj, 2014-04-16 15:06:58]

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:103:
controller->Animate(kInitialTickTime + GetTimeDelta(0.5));
On 2014/04/16 13:33:17, mithro wrote:
> Why didn't you just do;
> -----------------------------------
>   base::TimeTicks::FromMilliseconds(500);
> -----------------------------------

+1 FromMilliseconds instead of converting from doubles would be preferrable.

[Sikugu_, 2014-04-21 14:19:41]

Hi Mithro, Danakj, tomhudson

Sorry i was on leave last week.
I checked your comments. Will do the necessary changes.

As of now one unit test is failing 

LayerAnimationControllerTest.PauseResume

So i am trying to findout the possible root cause,
mean while i will try to upload the current working patch.

Thanks.

[Sikugu_, 2014-04-24 15:19:30]

PTAL at the refactoring..

This is an WIP.

One test case is failing as below.

[ RUN      ] LayerAnimationControllerTest.PauseResume
../../cc/animation/layer_animation_controller_unittest.cc:1257: Failure
Value of: controller->HasActiveAnimation()
  Actual: false
Expected: true
../../cc/animation/layer_animation_controller_unittest.cc:1263: Failure
Value of: dummy.opacity()
  Actual: 0.75
Expected: 1.f
Which is: 1
[  FAILED  ] LayerAnimationControllerTest.PauseResume (2 ms)


Trying to fix this. 

Thanks.

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:29: return
GetTimeTicks(offset) - base::TimeTicks();
On 2014/04/10 14:26:59, ajuma wrote:
> Instead of this function, how about just using
base::TimeDelta::FromMilliseconds
> wherever you need to create a TimeDelta? Then this won't involve a
double-to-int
> conversion.

Done.

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:35: const base::TimeTicks
kInitialTickTime = GetTimeTicks(1.0);
On 2014/04/10 14:26:59, ajuma wrote:
> Note that this is adding a static intializer. I'd suggesting getting rid of
the
> GetTimeTicks function altogether, and just having:
> base::TimeTicks InitialTickTime() {
>   return
base::TimeTicks::FromInternalValue(base::Time::kMicrosecondsPerSecond);
> }

Done.

https://codereview.chromium.org/231133002/diff/60001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:735: double duration =
controller_impl->GetAnimation(
On 2014/04/10 14:26:59, ajuma wrote:
> If we convert this duration to a TimeDelta, then we can directly use it below
> (when we need to divide it by 2.0 and/or add it to the start time).

Done.

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:23: base::TimeTicks
GetTimeTicks(double time) {
On 2014/04/16 13:33:17, mithro wrote:
> A better name is "double seconds".

Done.

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:35: const base::TimeTicks
kInitialTickTime = GetTimeTicks(1.0);
On 2014/04/16 13:33:17, mithro wrote:
> Why didn't you do a 
> -----------------------------------
>   using base::TimeTicks;
> 
>   const TimeTicks kInitialTickTime = TimeTicks::FromInternalValue(1000000);
> -----------------------------------
> 
> or maybe;
> -----------------------------------
>   const TimeTicks kInitialTickTime =
> TimeTicks::FromInternalValue(base::Time::kMicrosecondsPerSecond);
> -----------------------------------
> 

Done.

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:103:
controller->Animate(kInitialTickTime + GetTimeDelta(0.5));
On 2014/04/16 13:33:17, mithro wrote:
> Why didn't you just do;
> -----------------------------------
>   base::TimeTicks::FromMilliseconds(500);
> -----------------------------------

Done.

https://codereview.chromium.org/231133002/diff/80001/cc/animation/layer_anima...
cc/animation/layer_animation_controller_unittest.cc:103:
controller->Animate(kInitialTickTime + GetTimeDelta(0.5));
On 2014/04/16 15:06:58, danakj wrote:
> On 2014/04/16 13:33:17, mithro wrote:
> > Why didn't you just do;
> > -----------------------------------
> >   base::TimeTicks::FromMilliseconds(500);
> > -----------------------------------
> 
> +1 FromMilliseconds instead of converting from doubles would be preferrable.

Done.

[ajuma, 2014-04-24 20:41:35]

https://codereview.chromium.org/231133002/diff/120001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/231133002/diff/120001/base/time/time.h#newcod...
base/time/time.h:679: }
It's not obvious to me whether these are a good idea (since TimeTicks seems to
go out of its way to avoid committing to the units that its internal value
represents). Since the rest of the CL relies heavily on this, I'd suggest
putting these up for review separately first.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:67: start_time_(base::TimeTicks()),
This line can be removed (the default constructor for TimeTicks will be called
automatically).

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:69: time_offset_(base::TimeDelta()),
Can be removed.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:73: pause_time_(base::TimeTicks()),
Remove.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:74: total_paused_time_(base::TimeTicks()),
Remove.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:155: base::TimeTicks::InSecondsF((monotonic_time +
time_offset_) -
Duration should be compared to an amount of time, not to a point in time (which
is what TimeTicks represents). So instead, compute a TimeDelta using
"montonic_time - start_time() - total_paused_time_ + time_offset_" and then
convert this to seconds for the comparison (or, alternatively, convert
"iterations * curve_->Duration()" to a TimeDelta).

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:161: double trimmed =
base::TimeTicks::InSecondsF(monotonic_time + time_offset_);
Instead of converting to seconds here, wait until after start_time_,
pause_time_, total_pause_time_, and time_offset_ are taken into account below
(or else we lose some of the value of having those as TimeTicks and TimeDelta),
and until you really need to have a double. At that point, convert to a
TimeDelta (by subtracting TimeTicks()), and then use TimeDelta's conversion
functions to get a double.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation.h
File cc/animation/animation.h (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.h:179: base::TimeTicks total_paused_time_;
These should be TimeDelta (since they're measuring an amount of time, not a
point in time).

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:676:
controller->Animate(kInitialTickTime + time_delta / 2.0f);
Note that TimeDelta doesn't have an operator/ that takes a float, so this is
going to perform integer division. To make this clear, use 2 rather than 2.0f
here.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:681:
controller_impl->Animate(kInitialTickTime + time_delta / 2.0f);
Same comment as above about division.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:756:
controller->Animate(kInitialTickTime + time_delta / 2.0);
See comment above about division.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:761:
controller_impl->Animate(kInitialTickTime + time_delta / 2.0);
See comment above about division.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:814:
controller_impl->Animate(kInitialTickTime + time_delta / 2.0);
Here too.

https://codereview.chromium.org/231133002/diff/120001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1125: base::TimeTicks::FromInternalValue(time_offset *
The second argument to PauseAnimation should be a TimeDelta.

https://codereview.chromium.org/231133002/diff/120001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/231133002/diff/120001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:85: return
animation_->ticks_inseconds(animation_->start_time());
There's no need for a ticks_inseconds method. Just convert directly here in the
usual way (animation->start_time() - base::TimeTicks()).InSecondsF()).

https://codereview.chromium.org/231133002/diff/120001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:94: return
animation_->delta_inseconds(animation_->time_offset());
There's no need for a delta_inseconds method. Use TimeDelta's conversion methods
directly.

[Sikugu_, 2014-05-05 07:09:18]

PTAL..

All unittests are passed now.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:67: start_time_(base::TimeTicks()),
On 2014/04/24 20:41:36, ajuma wrote:
> This line can be removed (the default constructor for TimeTicks will be called
> automatically).

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:69: time_offset_(base::TimeDelta()),
On 2014/04/24 20:41:36, ajuma wrote:
> Can be removed.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:73: pause_time_(base::TimeTicks()),
On 2014/04/24 20:41:36, ajuma wrote:
> Remove.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:74: total_paused_time_(base::TimeTicks()),
On 2014/04/24 20:41:36, ajuma wrote:
> Remove.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:155: base::TimeTicks::InSecondsF((monotonic_time +
time_offset_) -
On 2014/04/24 20:41:36, ajuma wrote:
> Duration should be compared to an amount of time, not to a point in time
(which
> is what TimeTicks represents). So instead, compute a TimeDelta using
> "montonic_time - start_time() - total_paused_time_ + time_offset_" and then
> convert this to seconds for the comparison (or, alternatively, convert
> "iterations * curve_->Duration()" to a TimeDelta).
When we start adding the Timedelta to Timeticks we are loosing the precision as
the animation curve is operating still in double, converting first to  double
and then adding subtracting or dividing in 

Animation::IsFinishedAt 
Animation::TrimTimeToCurrentIteration

is giving the right values.
Once the complete code (Animation curves), this will work as expected.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:155: base::TimeTicks::InSecondsF((monotonic_time +
time_offset_) -
On 2014/04/24 20:41:36, ajuma wrote:
> Duration should be compared to an amount of time, not to a point in time
(which
> is what TimeTicks represents). So instead, compute a TimeDelta using
> "montonic_time - start_time() - total_paused_time_ + time_offset_" and then
> convert this to seconds for the comparison (or, alternatively, convert
> "iterations * curve_->Duration()" to a TimeDelta).

When we start adding the Timedelta to Timeticks we are loosing the precision as
the animation curve is operating still in double, converting first to  double
and then adding subtracting or dividing in 

Animation::IsFinishedAt 
Animation::TrimTimeToCurrentIteration

is giving the right values.
Once the complete code (Animation curves), this will work as expected.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:161: double trimmed =
base::TimeTicks::InSecondsF(monotonic_time + time_offset_);
On 2014/04/24 20:41:36, ajuma wrote:
> Instead of converting to seconds here, wait until after start_time_,
> pause_time_, total_pause_time_, and time_offset_ are taken into account below
> (or else we lose some of the value of having those as TimeTicks and
TimeDelta),
> and until you really need to have a double. At that point, convert to a
> TimeDelta (by subtracting TimeTicks()), and then use TimeDelta's conversion
> functions to get a double.

Same here.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation.h
File cc/animation/animation.h (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.h:179: base::TimeTicks total_paused_time_;
On 2014/04/24 20:41:36, ajuma wrote:
> These should be TimeDelta (since they're measuring an amount of time, not a
> point in time).

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:676:
controller->Animate(kInitialTickTime + time_delta / 2.0f);
On 2014/04/24 20:41:36, ajuma wrote:
> Note that TimeDelta doesn't have an operator/ that takes a float, so this is
> going to perform integer division. To make this clear, use 2 rather than 2.0f
> here.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:681:
controller_impl->Animate(kInitialTickTime + time_delta / 2.0f);
On 2014/04/24 20:41:36, ajuma wrote:
> Same comment as above about division.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:756:
controller->Animate(kInitialTickTime + time_delta / 2.0);
On 2014/04/24 20:41:36, ajuma wrote:
> See comment above about division.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:761:
controller_impl->Animate(kInitialTickTime + time_delta / 2.0);
On 2014/04/24 20:41:36, ajuma wrote:
> See comment above about division.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:814:
controller_impl->Animate(kInitialTickTime + time_delta / 2.0);
On 2014/04/24 20:41:36, ajuma wrote:
> Here too.

Done.

https://codereview.chromium.org/231133002/diff/120001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1125: base::TimeTicks::FromInternalValue(time_offset *
On 2014/04/24 20:41:36, ajuma wrote:
> The second argument to PauseAnimation should be a TimeDelta.

Done.

https://codereview.chromium.org/231133002/diff/120001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/231133002/diff/120001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:85: return
animation_->ticks_inseconds(animation_->start_time());
On 2014/04/24 20:41:36, ajuma wrote:
> There's no need for a ticks_inseconds method. Just convert directly here in
the
> usual way (animation->start_time() - base::TimeTicks()).InSecondsF()).

Done.

https://codereview.chromium.org/231133002/diff/120001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:94: return
animation_->delta_inseconds(animation_->time_offset());
On 2014/04/24 20:41:36, ajuma wrote:
> There's no need for a delta_inseconds method. Use TimeDelta's conversion
methods
> directly.

Done.

[ajuma, 2014-05-05 15:13:31]

Thanks, this is getting a lot closer.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:155: base::TimeTicks::InSecondsF((monotonic_time +
time_offset_) -
On 2014/05/05 07:09:19, Sikugu_ wrote:
> On 2014/04/24 20:41:36, ajuma wrote:
> > Duration should be compared to an amount of time, not to a point in time
> (which
> > is what TimeTicks represents). So instead, compute a TimeDelta using
> > "montonic_time - start_time() - total_paused_time_ + time_offset_" and then
> > convert this to seconds for the comparison (or, alternatively, convert
> > "iterations * curve_->Duration()" to a TimeDelta).
> 
> When we start adding the Timedelta to Timeticks we are loosing the precision
as
> the animation curve is operating still in double, converting first to  double
> and then adding subtracting or dividing in 
> 
> Animation::IsFinishedAt 
> Animation::TrimTimeToCurrentIteration
> 
> is giving the right values.
> Once the complete code (Animation curves), this will work as expected.

Adding TimeDelta to TimeTicks should not lose precision, since they both use
int64 internally (and with the sizes of the numbers we're using, overflow
shouldn't be an issue). Please give a specific example where you're seeing loss
of precision when this line is changed to:
return run_state == Running && iterations_ >= 0 &&
       iterations_ * curve_->Duration() <=
       (monotonic_time - start_time_ - total_paused_time_ +
time_offset_).InSecondsF();

(We need to figure out what's going wrong, since the fact that converting to
doubles happens to work might be masking some other bug.)

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:161: double trimmed =
base::TimeTicks::InSecondsF(monotonic_time + time_offset_);
On 2014/05/05 07:09:19, Sikugu_ wrote:
> On 2014/04/24 20:41:36, ajuma wrote:
> > Instead of converting to seconds here, wait until after start_time_,
> > pause_time_, total_pause_time_, and time_offset_ are taken into account
below
> > (or else we lose some of the value of having those as TimeTicks and
> TimeDelta),
> > and until you really need to have a double. At that point, convert to a
> > TimeDelta (by subtracting TimeTicks()), and then use TimeDelta's conversion
> > functions to get a double.
> 
> Same here.

And same response :)  Please give a specific example where using TimeTicks and
TimeDeltas causes a loss of precision. (Note that it's ok to use doubles once
you need to divide, but before that, when you're only adding or subtracting,
it's not clear why using TimeTicks and TimeDelta wouldn't work.)

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:208:
DCHECK(TimeUtil::TicksInSecondsF(last_tick_time_));
DCHECK(last_tick_time != base::TimeTicks())

(This is a good example of how having a conversion function available encourages
conversions where we don't really need them.)

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:673: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
It would make the rest of the test easier to read if this variable was
'duration' (and the previous variable was, say, duration_in_seconds).

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:768: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
Same comment, please rename this to duration.

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:827: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
Here too.

https://codereview.chromium.org/231133002/diff/160001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/231133002/diff/160001/cc/base/time_util.h#new...
cc/base/time_util.h:15: static double TicksInSecondsF(base::TimeTicks time) {
A previous CL that added similar functions was rejected here:
https://codereview.chromium.org/192433002/#msg10

Also, making these functions available would seem to encourage conversions
between TimeTicks and doubles, when we should really be trying to eliminate them
all.

Alternative suggestion:
In non-test code, use the existing way of converting (that is, (ticks -
base::TimeTicks()).InSecondsF() to convert from TimeTicks to double, and
base::TimeTicks::FromInternalValue(seconds * base::Time::kMicrosecondsPerSecond)
to convert from double to TimeTicks).

For test code, where we have a large number of these conversions, add these
conversion functions to cc/test/animation_test_common.{h, cc}.

https://codereview.chromium.org/231133002/diff/160001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.h (right):

https://codereview.chromium.org/231133002/diff/160001/cc/input/page_scale_ani...
cc/input/page_scale_animation.h:17: using base::TimeDelta;
Please do not add 'using' to .h files.

[Sikugu_, 2014-05-07 14:49:06]

PTAL..

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:155: base::TimeTicks::InSecondsF((monotonic_time +
time_offset_) -
On 2014/05/05 15:13:32, ajuma wrote:
> On 2014/05/05 07:09:19, Sikugu_ wrote:
> > On 2014/04/24 20:41:36, ajuma wrote:
> > > Duration should be compared to an amount of time, not to a point in time
> > (which
> > > is what TimeTicks represents). So instead, compute a TimeDelta using
> > > "montonic_time - start_time() - total_paused_time_ + time_offset_" and
then
> > > convert this to seconds for the comparison (or, alternatively, convert
> > > "iterations * curve_->Duration()" to a TimeDelta).
> > 
> > When we start adding the Timedelta to Timeticks we are loosing the precision
> as
> > the animation curve is operating still in double, converting first to 
double
> > and then adding subtracting or dividing in 
> > 
> > Animation::IsFinishedAt 
> > Animation::TrimTimeToCurrentIteration
> > 
> > is giving the right values.
> > Once the complete code (Animation curves), this will work as expected.
> 
> Adding TimeDelta to TimeTicks should not lose precision, since they both use
> int64 internally (and with the sizes of the numbers we're using, overflow
> shouldn't be an issue). Please give a specific example where you're seeing
loss
> of precision when this line is changed to:
> return run_state == Running && iterations_ >= 0 &&
>        iterations_ * curve_->Duration() <=
>        (monotonic_time - start_time_ - total_paused_time_ +
> time_offset_).InSecondsF();
> 
> (We need to figure out what's going wrong, since the fact that converting to
> doubles happens to work might be masking some other bug.)

Its a misinterpretation by me, it works fine with latest changes.

https://codereview.chromium.org/231133002/diff/120001/cc/animation/animation....
cc/animation/animation.cc:161: double trimmed =
base::TimeTicks::InSecondsF(monotonic_time + time_offset_);
On 2014/05/05 15:13:32, ajuma wrote:
> On 2014/05/05 07:09:19, Sikugu_ wrote:
> > On 2014/04/24 20:41:36, ajuma wrote:
> > > Instead of converting to seconds here, wait until after start_time_,
> > > pause_time_, total_pause_time_, and time_offset_ are taken into account
> below
> > > (or else we lose some of the value of having those as TimeTicks and
> > TimeDelta),
> > > and until you really need to have a double. At that point, convert to a
> > > TimeDelta (by subtracting TimeTicks()), and then use TimeDelta's
conversion
> > > functions to get a double.
> > 
> > Same here.
> 
> And same response :)  Please give a specific example where using TimeTicks and
> TimeDeltas causes a loss of precision. (Note that it's ok to use doubles once
> you need to divide, but before that, when you're only adding or subtracting,
> it's not clear why using TimeTicks and TimeDelta wouldn't work.)

Its a misinterpretation by me, it works fine with latest changes.

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:208:
DCHECK(TimeUtil::TicksInSecondsF(last_tick_time_));
On 2014/05/05 15:13:32, ajuma wrote:
> DCHECK(last_tick_time != base::TimeTicks())
> 
> (This is a good example of how having a conversion function available
encourages
> conversions where we don't really need them.)

Done.

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:673: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
On 2014/05/05 15:13:32, ajuma wrote:
> It would make the rest of the test easier to read if this variable was
> 'duration' (and the previous variable was, say, duration_in_seconds).

Done.

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:673: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
On 2014/05/05 15:13:32, ajuma wrote:
> It would make the rest of the test easier to read if this variable was
> 'duration' (and the previous variable was, say, duration_in_seconds).

Done.

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:768: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
On 2014/05/05 15:13:32, ajuma wrote:
> Same comment, please rename this to duration.

Done.

https://codereview.chromium.org/231133002/diff/160001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:827: TimeDelta time_delta =
TimeDelta::FromMicroseconds(
On 2014/05/05 15:13:32, ajuma wrote:
> Here too.

Done.

https://codereview.chromium.org/231133002/diff/160001/cc/base/time_util.h
File cc/base/time_util.h (right):

https://codereview.chromium.org/231133002/diff/160001/cc/base/time_util.h#new...
cc/base/time_util.h:15: static double TicksInSecondsF(base::TimeTicks time) {
On 2014/05/05 15:13:32, ajuma wrote:
> A previous CL that added similar functions was rejected here:
> https://codereview.chromium.org/192433002/#msg10
> 
> Also, making these functions available would seem to encourage conversions
> between TimeTicks and doubles, when we should really be trying to eliminate
them
> all.
> 
> Alternative suggestion:
> In non-test code, use the existing way of converting (that is, (ticks -
> base::TimeTicks()).InSecondsF() to convert from TimeTicks to double, and
> base::TimeTicks::FromInternalValue(seconds *
base::Time::kMicrosecondsPerSecond)
> to convert from double to TimeTicks).
> 
> For test code, where we have a large number of these conversions, add these
> conversion functions to cc/test/animation_test_common.{h, cc}.

Done.

https://codereview.chromium.org/231133002/diff/160001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.h (right):

https://codereview.chromium.org/231133002/diff/160001/cc/input/page_scale_ani...
cc/input/page_scale_animation.h:17: using base::TimeDelta;
On 2014/05/05 15:13:32, ajuma wrote:
> Please do not add 'using' to .h files.

Done.

[ajuma, 2014-05-07 17:04:41]

Please make sure this passes try jobs. Pay special attention to cc_unittests
(looks like you've already been doing this), 
webkit_compositor_bindings_unittests, and layout tests (specifically, those in
virtual/threaded/animations and virtual/threaded/transitions).

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:67: start_time_(),
This line isn't needed (the default constructor will automatically be used
without this line).

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:69: time_offset_(),
Not needed.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:73: pause_time_(),
Not needed.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:74: total_paused_time_(),
Not needed.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:171: trimmed -= start_time_ + total_paused_time_ -
base::TimeTicks();
trimmed -= (start_time_ - base::TimeTicks()) + total_paused_time_;
seems easier to understand.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation_...
File cc/animation/animation_unittest.cc (left):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation_...
cc/animation/animation_unittest.cc:6: 
This blank line shouldn't be removed.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:27: last_tick_time_(),
Not needed.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (left):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:665: 
Please keep this whitespace.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:678: 
Here too.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:29: duration_(),
Not needed.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:74: duration_(),
Not needed.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:171: return (start_time_ -
base::TimeTicks()).InSecondsF() >= 0;
return start_time_ > base::TimeTicks();

is more efficient, since it avoids the floating-point division in InSecondsF().

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:175: DCHECK_GT(0, (start_time_ -
base::TimeTicks()).InSecondsF());
DCHECK(start_time_ < base::TimeTicks());

is easier to read.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:181: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
DCHECK(start_time_>= base::TimeTicks());

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:186: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
DCHECK(start_time_ >= base::TimeTicks());

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:191: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
DCHECK(start_time_ >= base::TimeTicks());

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:196: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
DCHECK(start_time_ >= base::TimeTicks());

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:198: (start_time_ -
base::TimeTicks()).InSecondsF());
DCHECK(monotonic_time >= start_time_);

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:10: #include
"cc/base/time_util.h"
Please remove, since this file no longer exists in the latest patch set.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:20: using
cc::TimeUtil;
Please remove.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:87: return
TimeUtil::TicksInSecondsF(animation_->start_time());
TimeUtil no longer exists in the latest patch set.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:100:
animation_->set_time_offset(base::TimeDelta::FromSeconds(monotonic_time));
FromSecondsD (or else we'll lose precision).

[ajuma, 2014-05-07 17:06:49]

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:171: return (start_time_ -
base::TimeTicks()).InSecondsF() >= 0;
On 2014/05/07 17:04:42, ajuma wrote:
> return start_time_ > base::TimeTicks();
> 
> is more efficient, since it avoids the floating-point division in
InSecondsF().

Sorry, make that:

return start_time_ >= base::TimeTicks();

[Sikugu_, 2014-05-12 16:01:52]

PTAL..

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:67: start_time_(),
On 2014/05/07 17:04:42, ajuma wrote:
> This line isn't needed (the default constructor will automatically be used
> without this line).

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:69: time_offset_(),
On 2014/05/07 17:04:42, ajuma wrote:
> Not needed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:73: pause_time_(),
On 2014/05/07 17:04:42, ajuma wrote:
> Not needed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:74: total_paused_time_(),
On 2014/05/07 17:04:42, ajuma wrote:
> Not needed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation....
cc/animation/animation.cc:171: trimmed -= start_time_ + total_paused_time_ -
base::TimeTicks();
On 2014/05/07 17:04:42, ajuma wrote:
> trimmed -= (start_time_ - base::TimeTicks()) + total_paused_time_;
> seems easier to understand.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation_...
File cc/animation/animation_unittest.cc (left):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/animation_...
cc/animation/animation_unittest.cc:6: 
On 2014/05/07 17:04:42, ajuma wrote:
> This blank line shouldn't be removed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:27: last_tick_time_(),
On 2014/05/07 17:04:42, ajuma wrote:
> Not needed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (left):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:665: 
On 2014/05/07 17:04:42, ajuma wrote:
> Please keep this whitespace.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:678: 
On 2014/05/07 17:04:42, ajuma wrote:
> Here too.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/animation/scroll_off...
File cc/animation/scroll_offset_animation_curve.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/animation/scroll_off...
cc/animation/scroll_offset_animation_curve.cc:29: duration_(),
On 2014/05/07 17:04:42, ajuma wrote:
> Not needed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:74: duration_(),
On 2014/05/07 17:04:42, ajuma wrote:
> Not needed.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:171: return (start_time_ -
base::TimeTicks()).InSecondsF() >= 0;
On 2014/05/07 17:04:42, ajuma wrote:
> return start_time_ > base::TimeTicks();
> 
> is more efficient, since it avoids the floating-point division in
InSecondsF().

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:175: DCHECK_GT(0, (start_time_ -
base::TimeTicks()).InSecondsF());
On 2014/05/07 17:04:42, ajuma wrote:
> DCHECK(start_time_ < base::TimeTicks());
> 
> is easier to read.

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:181: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
On 2014/05/07 17:04:42, ajuma wrote:
> DCHECK(start_time_>= base::TimeTicks());

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:186: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
On 2014/05/07 17:04:42, ajuma wrote:
> DCHECK(start_time_ >= base::TimeTicks());

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:191: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
On 2014/05/07 17:04:42, ajuma wrote:
> DCHECK(start_time_ >= base::TimeTicks());

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:196: DCHECK_GE((start_time_ -
base::TimeTicks()).InSecondsF(), 0);
On 2014/05/07 17:04:42, ajuma wrote:
> DCHECK(start_time_ >= base::TimeTicks());

Done.

https://codereview.chromium.org/231133002/diff/180001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:198: (start_time_ -
base::TimeTicks()).InSecondsF());
On 2014/05/07 17:04:42, ajuma wrote:
> DCHECK(monotonic_time >= start_time_);

Done.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
File webkit/renderer/compositor_bindings/web_animation_impl.cc (right):

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:10: #include
"cc/base/time_util.h"
On 2014/05/07 17:04:42, ajuma wrote:
> Please remove, since this file no longer exists in the latest patch set.

Done.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:20: using
cc::TimeUtil;
On 2014/05/07 17:04:42, ajuma wrote:
> Please remove.

Done.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:87: return
TimeUtil::TicksInSecondsF(animation_->start_time());
On 2014/05/07 17:04:42, ajuma wrote:
> TimeUtil no longer exists in the latest patch set.

Done.

https://codereview.chromium.org/231133002/diff/180001/webkit/renderer/composi...
webkit/renderer/compositor_bindings/web_animation_impl.cc:100:
animation_->set_time_offset(base::TimeDelta::FromSeconds(monotonic_time));
On 2014/05/07 17:04:42, ajuma wrote:
> FromSecondsD (or else we'll lose precision).

Done.

[ajuma, 2014-05-12 18:12:10]

Thanks so much for patiently iterating on this.

cc/ and ui/compositor lgtm.

Please format your CL description so each line is at most 72 characters.

[Sikugu_, 2014-05-12 19:20:54]

Danakj, Piman, enne, aelias

PTAL..

[piman, 2014-05-12 20:07:02]

LGTM for ui/compositor if ajuma@ is happy.

[danakj, 2014-05-12 22:31:31]

ditto from me rslgtm

[Sikugu_, 2014-05-13 04:32:25]

@enne, nduca for webkit/renderer/compositor_bindings/
@aelias for cc/input

PTAL..

[aelias_OOO_until_Jul13, 2014-05-13 06:00:06]

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:73: -1.0 *
base::Time::kMicrosecondsPerSecond)),
This default value is a bad idea here, particularly as TimeTicks holds an
unsigned internally.  Please switch to the normal null value of
base::TimeTicks() and change the DCHECKs below to use > instead of >=.

[mithro-old, 2014-05-13 09:25:30]

Hi,

A bunch of minor comments plus one big question about "double
Animation::TrimTimeToCurrentIteration(".

I haven't been following the whole chain of ajuma's and danakj's reviews
closely, so if I conflict with anything they have said, please go mention it
with their suggestions.

Tim 'mithro' Ansell

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.cc:157: double Animation::TrimTimeToCurrentIteration(
Why does this function still return a double? Are you leaving the conversion of
this function until later?

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.cc:167: trimmed -= (start_time_ - base::TimeTicks()) +
total_paused_time_;
I don't think you need the "- base::TimeTicks()" here?

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.cc:175: double trimmed_in_seconds = (trimmed -
base::TimeTicks()).InSecondsF();
I don't see why you need to convert to seconds in this stage? Can't all the
following be done in TimeTicks/TimeDelta?

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation.h
File cc/animation/animation.h (right):

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.h:79: bool has_set_start_time() const { return
(start_time_ != base::TimeTicks()); }
start_time_.is_null()

https://codereview.chromium.org/231133002/diff/260001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/231133002/diff/260001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:126: DCHECK(monotonic_time !=
base::TimeTicks());
DCHECK(!monotonic_time.is_null())

BTW There is also a DCHECK_NE that should be used in the above.

[ajuma, 2014-05-13 13:44:48]

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:73: -1.0 *
base::Time::kMicrosecondsPerSecond)),
On 2014/05/13 06:00:07, aelias wrote:
> This default value is a bad idea here, particularly as TimeTicks holds an
> unsigned internally.  Please switch to the normal null value of
> base::TimeTicks() and change the DCHECKs below to use > instead of >=.

Fwiw, TimeTicks uses a (signed) int64 internally. That said, changing this to
the normal null value of base::TimeTicks() still seems like a good idea. Note
that IsAnimationStarted below will also have to be changed to use > rather than
>=.

[Sikugu_, 2014-05-14 14:23:18]

PTAL..

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation.cc
File cc/animation/animation.cc (right):

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.cc:157: double Animation::TrimTimeToCurrentIteration(
I would like to change this once we convert the animation curve functions from 
double to Ticks/Deltas, other wise again to and fro conversion from TimeDelta to
double has to be added.

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.cc:167: trimmed -= (start_time_ - base::TimeTicks()) +
total_paused_time_;
On 2014/05/13 09:25:30, mithro wrote:
> I don't think you need the "- base::TimeTicks()" here?

We can just add or delete TimeDeltas from TimeTicks.
TimeTicks is not having a overloaded operator to subtract/add its own type.

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.cc:175: double trimmed_in_seconds = (trimmed -
base::TimeTicks()).InSecondsF();
On 2014/05/13 09:25:30, mithro wrote:
> I don't see why you need to convert to seconds in this stage? Can't all the
> following be done in TimeTicks/TimeDelta?

fmod and division won't work well with TimeTicks.
and as of now the curve duration is not yet changed to TimeTicks.
Here it would be easy to use double.
I will take care of this in my next patch.

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation.h
File cc/animation/animation.h (right):

https://codereview.chromium.org/231133002/diff/260001/cc/animation/animation....
cc/animation/animation.h:79: bool has_set_start_time() const { return
(start_time_ != base::TimeTicks()); }
On 2014/05/13 09:25:30, mithro wrote:
> start_time_.is_null()

Done.

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:73: -1.0 *
base::Time::kMicrosecondsPerSecond)),
On 2014/05/13 06:00:07, aelias wrote:
> This default value is a bad idea here, particularly as TimeTicks holds an
> unsigned internally.  Please switch to the normal null value of
> base::TimeTicks() and change the DCHECKs below to use > instead of >=.

Done.

https://codereview.chromium.org/231133002/diff/260001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:73: -1.0 *
base::Time::kMicrosecondsPerSecond)),
On 2014/05/13 13:44:48, ajuma wrote:
> On 2014/05/13 06:00:07, aelias wrote:
> > This default value is a bad idea here, particularly as TimeTicks holds an
> > unsigned internally.  Please switch to the normal null value of
> > base::TimeTicks() and change the DCHECKs below to use > instead of >=.
> 
> Fwiw, TimeTicks uses a (signed) int64 internally. That said, changing this to
> the normal null value of base::TimeTicks() still seems like a good idea. Note
> that IsAnimationStarted below will also have to be changed to use > rather
than
> >=.

Done.

[ajuma, 2014-05-14 14:49:49]

still lgtm, with one nit:

https://codereview.chromium.org/231133002/diff/280001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/280001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:172: DCHECK(start_time_ <= base::TimeTicks());
This should just be DCHECK(start_time.is_null()), I think (since we no longer
assign negative values to start_time_).

[enne (OOO), 2014-05-14 17:00:43]

webkit/renderer/compositor_bindings lgtm

[aelias_OOO_until_Jul13, 2014-05-14 18:38:47]

lgtm for cc/input modulo ajuma's nit (actually all of the other DCHECKs would be
better phrased as !is_null() too).

[Sikugu_, 2014-05-15 08:41:22]

https://codereview.chromium.org/231133002/diff/280001/cc/input/page_scale_ani...
File cc/input/page_scale_animation.cc (right):

https://codereview.chromium.org/231133002/diff/280001/cc/input/page_scale_ani...
cc/input/page_scale_animation.cc:172: DCHECK(start_time_ <= base::TimeTicks());
On 2014/05/14 14:49:49, ajuma wrote:
> This should just be DCHECK(start_time.is_null()), I think (since we no longer
> assign negative values to start_time_).

Done.

[Sikugu_, 2014-05-15 08:41:37]

The CQ bit was checked by sivagunturi@chromium.org

[commit-bot: I haz the power, 2014-05-15 08:42:27]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sivagunturi@chromium.org/231133002/300001

[commit-bot: I haz the power, 2014-05-15 17:31:38]

Change committed as 270718