Clean up the tab animations (part 2)

Clean up the tab animations (part 2)

Here we reach the end goal of only having
one animation object for the tab and one
animation object for the icon

Also removed dead flag kAppsNoThrob.

BUG=3541
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178460

[cpu_(ooo_6.6-7.5), 2013-01-22 21:06:36]

not ready for review but I need some advice.

[cpu_(ooo_6.6-7.5), 2013-01-23 03:07:05]

ready for review. I figured out my problem.

This CL also fixes the bug that if you unpin a tab that is animating (because of
title change) it stops animating.

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

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:498: 
nuke this, in fact nuke 494 and 490 as well.

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1077: data().mini) {
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]

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1077: data().mini) {
On 2013/01/23 16:10:02, sky wrote:
> What happens if we start animating while not mini, then change to mini?

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]

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1077: data().mini) {
On 2013/01/23 18:31:40, cpu wrote:
> On 2013/01/23 16:10:02, sky wrote:
> > What happens if we start animating while not mini, then change to mini?
> 
> 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

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

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

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1077: data().mini) {
On 2013/01/23 19:33:02, sky wrote:
> On 2013/01/23 18:31:40, cpu wrote:
> > On 2013/01/23 16:10:02, sky wrote:
> > > What happens if we start animating while not mini, then change to mini?

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


> > 
> > 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
> 
> Doesn't this mean you could do the wrong cast though?

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

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/12033027/diff/5001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1077: data().mini) {
On 2013/01/23 20:35:07, cpu wrote:
> On 2013/01/23 19:33:02, sky wrote:
> > On 2013/01/23 18:31:40, cpu wrote:
> > > On 2013/01/23 16:10:02, sky wrote:
> > > > What happens if we start animating while not mini, then change to mini?
> 
> Ah, I see what you mean. I have the check of line 550. Let me know if that is
> enough.
> 
> 
> > > 
> > > 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
> > 
> > Doesn't this mean you could do the wrong cast though?
> 

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.

[cpu_(ooo_6.6-7.5), 2013-01-23 21:27:49]

our caller, the tabstrip seems to be doing the right thing, in fact calling stop
at the drop of a hat but I see your concern that it might be fragile. 

I added what you suggested when data changes. Please take another look.

[sky, 2013-01-23 22:16:31]

LGTM

https://codereview.chromium.org/12033027/diff/3004/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/12033027/diff/3004/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1103: gfx::Canvas* canvas,
ui::MultiAnimation* animation) {
When you warp, each param on its own line.

https://codereview.chromium.org/12033027/diff/3004/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1508: if (!data().mini) {
Combine into a single if.