Clean up the tab animations (part 2)

chrome/browser/ui/views/tabs/tab.cc

Index: chrome/browser/ui/views/tabs/tab.cc
===================================================================
--- chrome/browser/ui/views/tabs/tab.cc	(revision 177059)
+++ chrome/browser/ui/views/tabs/tab.cc	(working copy)
@@ -417,7 +417,6 @@
       favicon_hiding_offset_(0),
       loading_animation_frame_(0),
       should_display_crashed_favicon_(false),
-      throbber_disabled_(false),
       theme_provider_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(hover_controller_(this)),
       showing_icon_(false),
@@ -496,6 +495,7 @@
   } else if ((data_.capture_state != TabRendererData::CAPTURE_STATE_NONE) &&
              (old.capture_state == TabRendererData::CAPTURE_STATE_NONE)) {
     StartRecordingAnimation();
+

[sky, 2013/01/23 16:10:02]

nuke this, in fact nuke 494 and 490 as well.

   } else {
     if (IsPerformingCrashAnimation())
       StopCrashAnimation();
@@ -516,14 +516,6 @@
 }
 
 void Tab::UpdateLoadingAnimation(TabRendererData::NetworkState state) {
-  // If this is an extension app and a command line flag is set,
-  // then disable the throbber.
-  throbber_disabled_ = data().app &&
-      CommandLine::ForCurrentProcess()->HasSwitch(switches::kAppsNoThrob);
-
-  if (throbber_disabled_)
-    return;
-
   if (state == data_.network_state &&
       state == TabRendererData::NETWORK_STATE_NONE) {
     // If the network state is none and hasn't changed, do nothing. Otherwise we
@@ -537,26 +529,27 @@
 }
 
 void Tab::StartPulse() {
-  if (!tab_animation_.get()) {
-    tab_animation_.reset(new ui::ThrobAnimation(this));
-    tab_animation_->SetSlideDuration(kPulseDurationMs);
-    if (animation_container_.get())
-      tab_animation_->SetContainer(animation_container_.get());
-  }
-  tab_animation_->Reset();
-  tab_animation_->StartThrobbing(std::numeric_limits<int>::max());
+  ui::ThrobAnimation* animation = new ui::ThrobAnimation(this);
+  animation->SetSlideDuration(kPulseDurationMs);
+  if (animation_container_.get())
+    animation->SetContainer(animation_container_.get());
+  animation->StartThrobbing(std::numeric_limits<int>::max());
+  tab_animation_.reset(animation);
 }
 
 void Tab::StopPulse() {
   if (!tab_animation_.get())
     return;
-
-  tab_animation_->Stop();  // Do stop so we get notified.
+  tab_animation_->Stop();
   tab_animation_.reset(NULL);
 }
 
 void Tab::StartMiniTabTitleAnimation() {
-  if (!mini_title_animation_.get()) {
+  // We can only do this animation if the tab is mini because we will
+  // upcast tab_animation back to MultiAnimation when we draw.
+  if (!data().mini)
+    return;
+  if (!tab_animation_.get()) {
     ui::MultiAnimation::Parts parts;
     parts.push_back(
         ui::MultiAnimation::Part(kMiniTitleChangeAnimationDuration1MS,
@@ -571,19 +564,22 @@
     parts[0].end_time_ms = kMiniTitleChangeAnimationEnd1MS;
     parts[2].start_time_ms = kMiniTitleChangeAnimationStart3MS;
     parts[2].end_time_ms = kMiniTitleChangeAnimationEnd3MS;
-    mini_title_animation_.reset(new ui::MultiAnimation(
-        parts,
-        base::TimeDelta::FromMilliseconds(
-            kMiniTitleChangeAnimationIntervalMS)));
-    mini_title_animation_->SetContainer(animation_container());
-    mini_title_animation_->set_delegate(this);
+    base::TimeDelta timeout =
+        base::TimeDelta::FromMilliseconds(kMiniTitleChangeAnimationIntervalMS);
+    ui::MultiAnimation* animation = new ui::MultiAnimation(parts, timeout);
+    if (animation_container_.get())
+      animation->SetContainer(animation_container_.get());
+    animation->set_delegate(this);
+    tab_animation_.reset(animation);
   }
-  mini_title_animation_->Start();
+  tab_animation_->Start();
 }
 
 void Tab::StopMiniTabTitleAnimation() {
-  if (mini_title_animation_.get())
-    mini_title_animation_->Stop();
+  if (!tab_animation_.get())
+    return;
+  tab_animation_->Stop();
+  tab_animation_.reset(NULL);
 }
 
 void Tab::UpdateIconDominantColor() {
@@ -1076,10 +1072,15 @@
   if (IsActive()) {
     PaintActiveTabBackground(canvas);
   } else {
-    if (mini_title_animation_.get() && mini_title_animation_->is_animating())
-      PaintInactiveTabBackgroundWithTitleChange(canvas);
-    else
+    if (tab_animation_.get() &&
+        tab_animation_->is_animating() &&
+        data().mini) {

[sky, 2013/01/23 16:10:02]

What happens if we start animating while not mini, then change to mini?

[cpu_(ooo_6.6-7.5), 2013/01/23 18:31:40]

Visually, nothing bad. The behavior is though a bit different:

Currently, if the minitab is animating and we unpin, the animation continues on
the regular tab, which imho its a bug, with this patch the animation stops.

Not just because of that data().mini check but also because at unpin we call
TabStrip::StopAllHighlighting() which calls Tab::StopPulse() which deletes the
animation.

I am guessing in the existing code the mini_title_animation does not get
canceled. Now, to be fair that might have been something I introduced in the
last cleanup or might have been there, no idea.

You can test with
alienrancher.appspot.com/html/title_change.html

[sky, 2013/01/23 19:33:02]

Doesn't this mean you could do the wrong cast though?

[cpu_(ooo_6.6-7.5), 2013/01/23 20:35:07]

Ah, I see what you mean. I have the check of line 550. Let me know if that is
enough.

[sky, 2013/01/23 21:07:22]

What happens if we StartPulse() and the tab is mini, or not-mini then becomes
mini? Seems like we should force the animation to stop when changing mini state.

+      ui::MultiAnimation* animation =
+          static_cast<ui::MultiAnimation*>(tab_animation_.get());
+      PaintInactiveTabBackgroundWithTitleChange(canvas, animation);
+    } else {
       PaintInactiveTabBackground(canvas);
+    }
 
     double throb_value = GetThrobValue();
     if (throb_value > 0) {
@@ -1091,7 +1092,8 @@
   }
 }
 
-void Tab::PaintInactiveTabBackgroundWithTitleChange(gfx::Canvas* canvas) {
+void Tab::PaintInactiveTabBackgroundWithTitleChange(
+    gfx::Canvas* canvas, ui::MultiAnimation* animation) {
   // Render the inactive tab background. We'll use this for clipping.
   gfx::Canvas background_canvas(size(), canvas->scale_factor(), false);
   PaintInactiveTabBackground(&background_canvas);
@@ -1105,12 +1107,12 @@
   int x1 = radius;
   int x2 = -radius;
   int x;
-  if (mini_title_animation_->current_part_index() == 0) {
-    x = mini_title_animation_->CurrentValueBetween(x0, x1);
-  } else if (mini_title_animation_->current_part_index() == 1) {
+  if (animation->current_part_index() == 0) {
+    x = animation->CurrentValueBetween(x0, x1);
+  } else if (animation->current_part_index() == 1) {
     x = x1;
   } else {
-    x = mini_title_animation_->CurrentValueBetween(x1, x2);
+    x = animation->CurrentValueBetween(x1, x2);
   }
   SkPoint center_point;
   center_point.iset(x, 0);
@@ -1133,8 +1135,8 @@
   canvas->DrawImageInt(background_image, 0, 0);
 
   // And then the gradient on top of that.
-  if (mini_title_animation_->current_part_index() == 2) {
-    uint8 alpha = mini_title_animation_->CurrentValueBetween(255, 0);
+  if (animation->current_part_index() == 2) {
+    uint8 alpha = animation->CurrentValueBetween(255, 0);
     canvas->DrawImageInt(hover_image, 0, 0, alpha);
   } else {
     canvas->DrawImageInt(hover_image, 0, 0);
@@ -1496,8 +1498,10 @@
   double min = is_selected ? kSelectedTabOpacity : 0;
   double scale = is_selected ? kSelectedTabThrobScale : 1;
 
-  if (tab_animation_.get() && tab_animation_->is_animating())
-    return tab_animation_->GetCurrentValue() * kHoverOpacity * scale + min;
+  if (!data().mini) {
+    if (tab_animation_.get() && tab_animation_->is_animating())
+      return tab_animation_->GetCurrentValue() * kHoverOpacity * scale + min;
+  }
 
   if (hover_controller_.ShouldDraw()) {
     return kHoverOpacity * hover_controller_.GetAnimationValue() * scale +