[rAc OSX] Animate the dots in the "Loading ..." message.

chrome/browser/ui/autofill/loading_animation.cc

 // Copyright 2013 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 "chrome/browser/ui/autofill/loading_animation.h"
 
 #include "base/logging.h"
 #include "ui/gfx/animation/tween.h"
 
 namespace autofill {
@@ skipping 12 matching lines @@
 
 void LoadingAnimation::Step(base::TimeTicks time_now) {
   LinearAnimation::Step(time_now);
 
   if (!is_animating()) {
     first_cycle_ = false;
     Start();
   }
 }
 
-double LoadingAnimation::GetCurrentValueForDot(size_t dot_i) {
+double LoadingAnimation::GetCurrentValueForDot(size_t dot_i) const {
   double base_value = gfx::LinearAnimation::GetCurrentValue();
 
   const double kSecondDotDelayMs = 100.0;
   const double kThirdDotDelayMs = 300.0;
   if (dot_i == 1)
     base_value -= kSecondDotDelayMs / kDurationMs;
   else if (dot_i == 2)
     base_value -= kThirdDotDelayMs / kDurationMs;
 
   if (base_value < 0.0)
     base_value = first_cycle_ ? 0.0 : base_value + 1.0;
 
   double value = gfx::Tween::CalculateValue(gfx::Tween::EASE_OUT, base_value);
 
-  static AnimationFrame animation_frames[] = {
+  static AnimationFrame kAnimationFrames[] = {
     { 0.0, 0.0 },
     { 0.55, 0.0 },
     { 0.6, -1.0 },
     { 0.8, 0.3 },
     { 0.9, -0.2 },
     { 0.95, 0.1 },
     { 1.0, 0.0 },
   };
+  static double kScale = 10.0;

[Evan Stade, 2013/11/15 21:58:46]

nit: I don't really see the benefit in doing this here --- the value depends on
the font size, so I'd prefer it be defined near where the font size is set (i.e.
in each view), even though that means defining it twice. Alternately, make this
class's ctor take a font size param then scale based on that.

[groby-ooo-7-16, 2013/11/15 23:03:38]

+1 on the ctor doing htis.

[Ilya Sherman, 2013/11/16 00:31:13]

Done.

 
-  for (size_t i = 0; i < arraysize(animation_frames); ++i) {
-    if (value > animation_frames[i].value)
+  for (size_t i = 0; i < arraysize(kAnimationFrames); ++i) {
+    if (value > kAnimationFrames[i].value)
       continue;
-    else if (i == 0)
-      return animation_frames[i].position;
 
-    return animation_frames[i - 1].position +
-        (animation_frames[i].position - animation_frames[i - 1].position) *
-            (value - animation_frames[i - 1].value) /
-                (animation_frames[i].value - animation_frames[i - 1].value);
+    double position;
+    if (i == 0) {
+      position = kAnimationFrames[i].position;
+    } else {
+      double distance =

[Evan Stade, 2013/11/15 21:58:46]

nit: IMO, the distance and progress variable names don't really help me
understand this code, so it doesn't come out any more readable than when it was
all on one line. Please either leave it as one line or come up with more helpful
variable names.

[Ilya Sherman, 2013/11/16 00:31:13]

Done.

+          kAnimationFrames[i].position - kAnimationFrames[i - 1].position;
+      double progress =
+          (value - kAnimationFrames[i - 1].value) /
+          (kAnimationFrames[i].value - kAnimationFrames[i - 1].value);
+      position = kAnimationFrames[i - 1].position + distance * progress;

[groby-ooo-7-16, 2013/11/15 23:03:38]

I kind of like clearing up the computation, because it's not immediately obvious
what's happening in the 4-line formula. So maybe progress->frameRelativeTime?

Also, if you keep the split, i thinkTween::FloatValueBetween(frameRelativeTime,
kAnimationFrames[i-1].position, kAnimationFrames[i].position) does the position
computation nicely and skips distance.

[Ilya Sherman, 2013/11/16 00:31:13]

Done.  Good call on using Tween::FloatValueBetween :)

+    }
+    return position * kScale;
   }
 
   NOTREACHED();
   return 0.0;
 }
 
 void LoadingAnimation::Reset() {
   Stop();
   first_cycle_ = true;
 }
 
 }  // namespace autofill