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

cc/animation/animation.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/animation/animation.h"
 
 #include <cmath>
 
 #include "base/debug/trace_event.h"
 #include "base/strings/string_util.h"
@@ skipping 46 matching lines @@
 Animation::Animation(scoped_ptr<AnimationCurve> curve,
                      int animation_id,
                      int group_id,
                      TargetProperty target_property)
     : curve_(curve.Pass()),
       id_(animation_id),
       group_(group_id),
       target_property_(target_property),
       run_state_(WaitingForTargetAvailability),
       iterations_(1),
-      start_time_(0),
+      start_time_(base::TimeTicks()),

[ajuma, 2014/04/24 20:41:36]

This line can be removed (the default constructor for TimeTicks will be called
automatically).

[Sikugu_, 2014/05/05 07:09:19]

Done.

       alternates_direction_(false),
-      time_offset_(0),
+      time_offset_(base::TimeDelta()),

[ajuma, 2014/04/24 20:41:36]

Can be removed.

[Sikugu_, 2014/05/05 07:09:19]

Done.

       needs_synchronized_start_time_(false),
       received_finished_event_(false),
       suspended_(false),
-      pause_time_(0),
-      total_paused_time_(0),
+      pause_time_(base::TimeTicks()),

[ajuma, 2014/04/24 20:41:36]

Remove.

[Sikugu_, 2014/05/05 07:09:19]

Done.

+      total_paused_time_(base::TimeTicks()),

[ajuma, 2014/04/24 20:41:36]

Remove.

[Sikugu_, 2014/05/05 07:09:19]

Done.

       is_controlling_instance_(false),
-      is_impl_only_(false) {}
+      is_impl_only_(false) {
+}
 
 Animation::~Animation() {
   if (run_state_ == Running || run_state_ == Paused)
-    SetRunState(Aborted, 0);
+    SetRunState(Aborted, base::TimeTicks());
 }
 
-void Animation::SetRunState(RunState run_state, double monotonic_time) {
+void Animation::SetRunState(RunState run_state,
+                            base::TimeTicks monotonic_time) {
   if (suspended_)
     return;
 
   char name_buffer[256];
   base::snprintf(name_buffer,
                  sizeof(name_buffer),
                  "%s-%d%s",
                  s_targetPropertyNames[target_property_],
                  group_,
                  is_controlling_instance_ ? "(impl)" : "");
@@ skipping 30 matching lines @@
 
   TRACE_EVENT_INSTANT2("cc",
                        "LayerAnimationController::SetRunState",
                        TRACE_EVENT_SCOPE_THREAD,
                        "Name",
                        TRACE_STR_COPY(name_buffer),
                        "State",
                        TRACE_STR_COPY(state_buffer));
 }
 
-void Animation::Suspend(double monotonic_time) {
+void Animation::Suspend(base::TimeTicks monotonic_time) {
   SetRunState(Paused, monotonic_time);
   suspended_ = true;
 }
 
-void Animation::Resume(double monotonic_time) {
+void Animation::Resume(base::TimeTicks monotonic_time) {
   suspended_ = false;
   SetRunState(Running, monotonic_time);
 }
 
-bool Animation::IsFinishedAt(double monotonic_time) const {
+bool Animation::IsFinishedAt(base::TimeTicks monotonic_time) const {
   if (is_finished())
     return true;
 
   if (needs_synchronized_start_time_)
     return false;
 
-  return run_state_ == Running &&
-         iterations_ >= 0 &&
-         iterations_ * curve_->Duration() <= (monotonic_time -
-                                              start_time() -
-                                              total_paused_time_ +
-                                              time_offset_);
+  return run_state_ == Running && iterations_ >= 0 &&
+         iterations_ * curve_->Duration() <=
+             base::TimeTicks::InSecondsF((monotonic_time + time_offset_) -

[ajuma, 2014/04/24 20:41:36]

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).

[Sikugu_, 2014/05/05 07:09:19]

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.

[Sikugu_, 2014/05/05 07:09:19]

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.

[ajuma, 2014/05/05 15:13:32]

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.)

[Sikugu_, 2014/05/07 14:49:07]

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

+                                         (start_time_ - total_paused_time_));
 }
 
-double Animation::TrimTimeToCurrentIteration(double monotonic_time) const {
-  double trimmed = monotonic_time + time_offset_;
+double Animation::TrimTimeToCurrentIteration(
+    base::TimeTicks monotonic_time) const {
+  double trimmed = base::TimeTicks::InSecondsF(monotonic_time + time_offset_);

[ajuma, 2014/04/24 20:41:36]

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.

[Sikugu_, 2014/05/05 07:09:19]

Same here.

[ajuma, 2014/05/05 15:13:32]

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.)

[Sikugu_, 2014/05/07 14:49:07]

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

 
   // If we're paused, time is 'stuck' at the pause time.
   if (run_state_ == Paused)
-    trimmed = pause_time_;
+    trimmed = base::TimeTicks::InSecondsF(pause_time_);
 
   // Returned time should always be relative to the start time and should
   // subtract all time spent paused.
-  trimmed -= start_time_ + total_paused_time_;
+  trimmed -= base::TimeTicks::InSecondsF(start_time_) +
+             base::TimeTicks::InSecondsF(total_paused_time_);
 
   // If we're just starting or we're waiting on receiving a start time,
   // time is 'stuck' at the initial state.
   if ((run_state_ == Starting && !has_set_start_time()) ||
       needs_synchronized_start_time())
-    trimmed = time_offset_;
+    trimmed = time_offset_.InSecondsF();
 
   // Zero is always the start of the animation.
   if (trimmed <= 0)
     return 0;
 
   // Always return zero if we have no iterations.
   if (!iterations_)
     return 0;
 
   // Don't attempt to trim if we have no duration.
@@ skipping 46 matching lines @@
   // the main thread.
   if (run_state_ == Animation::Paused ||
       other->run_state_ == Animation::Paused) {
     other->run_state_ = run_state_;
     other->pause_time_ = pause_time_;
     other->total_paused_time_ = total_paused_time_;
   }
 }
 
 }  // namespace cc