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

chrome/browser/ui/autofill/loading_animation.cc

Index: chrome/browser/ui/autofill/loading_animation.cc
diff --git a/chrome/browser/ui/autofill/loading_animation.cc b/chrome/browser/ui/autofill/loading_animation.cc
index 800ff3de8240b80c74c0aa492435724ead5a5c78..f11109adb58d75797169ff52101e8d668333c7a2 100644
--- a/chrome/browser/ui/autofill/loading_animation.cc
+++ b/chrome/browser/ui/autofill/loading_animation.cc
@@ -30,7 +30,7 @@ void LoadingAnimation::Step(base::TimeTicks time_now) {
   }
 }
 
-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;
@@ -45,7 +45,7 @@ double LoadingAnimation::GetCurrentValueForDot(size_t dot_i) {
 
   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 },
@@ -54,17 +54,24 @@ double LoadingAnimation::GetCurrentValueForDot(size_t dot_i) {
     { 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();