Mac: make favicon and title visible during in/out animation.

Mac: make favicon and title visible during in/out animation.

BUG=50307
TEST=Look.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68787

[viettrungluu, 2010-12-09 19:09:30]

http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
File chrome/browser/ui/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:956: if (showZoom) {
Can you make the three cases of |if (showZoom) { ...| share code?

[Nico, 2010-12-09 19:12:03]

http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
File chrome/browser/ui/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:956: if (showZoom) {
On 2010/12/09 19:09:30, viettrungluu wrote:
> Can you make the three cases of |if (showZoom) { ...| share code?

I did, I extracted the common code to a function!

Kidding aside: What do you have in mind? Most of the parameters are different,
what would you share?

[viettrungluu, 2010-12-09 20:15:42]

http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
File chrome/browser/ui/cocoa/tabpose_window.mm (right):

http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
chrome/browser/ui/cocoa/tabpose_window.mm:956: if (showZoom) {
On 2010/12/09 19:12:03, Nico wrote:
> On 2010/12/09 19:09:30, viettrungluu wrote:
> > Can you make the three cases of |if (showZoom) { ...| share code?
> 
> I did, I extracted the common code to a function!
> 
> Kidding aside: What do you have in mind? Most of the parameters are different,
> what would you share?

I'd like an "AnimateOrSetFrame()" (or whatever) function. (You could even push
|interval| into this function, with |slomo| as a parameter.)

So, instead of these if-else blocks, you'd just have:
AnimateOrSetFrame(layer, showZoom, slomo, foo.GetStartRect..., foo.end_rect(),
delegate);
(or something like that).

Incidentally, why doesn't the first one get an opacity animation? (I suppose if
that's right, then whether or not the opacity should be animate should also be a
parameter.)

[Nico, 2010-12-09 20:45:23]

On Thu, Dec 9, 2010 at 12:15 PM,  <viettrungluu@chromium.org> wrote:
>
>
http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
> File chrome/browser/ui/cocoa/tabpose_window.mm (right):
>
>
http://codereview.chromium.org/5618007/diff/2001/chrome/browser/ui/cocoa/tabp...
> chrome/browser/ui/cocoa/tabpose_window.mm:956: if (showZoom) {
> On 2010/12/09 19:12:03, Nico wrote:
>>
>> On 2010/12/09 19:09:30, viettrungluu wrote:
>> > Can you make the three cases of |if (showZoom) { ...| share code?
>
>> I did, I extracted the common code to a function!
>
>> Kidding aside: What do you have in mind? Most of the parameters are
>
> different,
>>
>> what would you share?
>
> I'd like an "AnimateOrSetFrame()" (or whatever) function. (You could
> even push |interval| into this function, with |slomo| as a parameter.)
>
> So, instead of these if-else blocks, you'd just have:
> AnimateOrSetFrame(layer, showZoom, slomo, foo.GetStartRect...,
> foo.end_rect(), delegate);
> (or something like that).
>
> Incidentally, why doesn't the first one get an opacity animation? (I
> suppose if that's right, then whether or not the opacity should be
> animate should also be a parameter.)

The  thumb layer should stay opaque during the animation. The text
layer stuff will probably change soon because Cole wants the favicon
and title layers to zoom in too, but you can't animatie a text layer's
font size. Having a function to encapsulate an if statement that
happens in two places seems like overkill.

[viettrungluu, 2010-12-09 23:14:20]

Fine then, be that way. LBGETM.