[Mac] Display a quick animation when a popup is blocked so the user notices it in the Omnibox.

[Mac] Display a quick animation when a popup is blocked so the user notices it in the Omnibox.

This refactors the animation code of the DownloadStartedAnimationMac into AnimatableImage.

BUG=49341
TEST=Go to http://www.popuptest.com/popuptest3.html. When a popup is opened, see animation.
TEST=Download a file. Animation still plays.

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

[Nico, 2010-07-15 22:26:56]

Please open a bug for this. Land the pdf in a separate CL, so that you can use
trybots for this one.

I will review this for code and let andybons and you figure out the politics.

[Nico, 2010-07-15 22:40:15]

initial nits

http://codereview.chromium.org/3014005/diff/1/3
File chrome/browser/cocoa/animatable_image.h (right):

http://codereview.chromium.org/3014005/diff/1/3#newcode16
chrome/browser/cocoa/animatable_image.h:16: // window for easier window
management.
mention somewhere that this releases itself on close

http://codereview.chromium.org/3014005/diff/1/3#newcode41
chrome/browser/cocoa/animatable_image.h:41: // Designated initialzer. Do not use
any other NSWindow initializers. Creates
initializer

http://codereview.chromium.org/3014005/diff/1/4
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/1/4#newcode10
chrome/browser/cocoa/animatable_image.mm:10: 
needs test

http://codereview.chromium.org/3014005/diff/1/4#newcode64
chrome/browser/cocoa/animatable_image.mm:64: [self startFrame].size.width,
cgrectgetwidth

also elsewhere

[Nico, 2010-07-15 22:41:49]

will look again once these are addressed. looks pretty good!

http://codereview.chromium.org/3014005/diff/1/8
File chrome/browser/popup_blocked_animation_stubs.cc (right):

http://codereview.chromium.org/3014005/diff/1/8#newcode8
chrome/browser/popup_blocked_animation_stubs.cc:8: #if !defined(OS_MACOSX)
i'd create a win and a linux file and do this via gyp instead of via
preprocessor

[Robert Sesek, 2010-07-15 23:42:22]

All nits addressed. Added a test. Good idea about splitting the image out into a
separate CL. I'll do that before trying/landing.

http://codereview.chromium.org/3014005/diff/1/8
File chrome/browser/popup_blocked_animation_stubs.cc (right):

http://codereview.chromium.org/3014005/diff/1/8#newcode8
chrome/browser/popup_blocked_animation_stubs.cc:8: #if !defined(OS_MACOSX)
On 2010/07/15 22:41:50, Nico wrote:
> i'd create a win and a linux file and do this via gyp instead of via
> preprocessor

We didn't do temp_scaffolding_stubs like this when it existed, so I'm OK with
this as-is.

[Nico, 2010-07-16 00:06:10]

Almost there.

http://codereview.chromium.org/3014005/diff/1/8
File chrome/browser/popup_blocked_animation_stubs.cc (right):

http://codereview.chromium.org/3014005/diff/1/8#newcode8
chrome/browser/popup_blocked_animation_stubs.cc:8: #if !defined(OS_MACOSX)
On 2010/07/15 23:42:23, rsesek wrote:
> On 2010/07/15 22:41:50, Nico wrote:
> > i'd create a win and a linux file and do this via gyp instead of via
> > preprocessor
> 
> We didn't do temp_scaffolding_stubs like this when it existed, so I'm OK with
> this as-is.

The days of temp_scaffolding_stubs are over, and they should never return.
Imagine how you would feel if a windows guy added something and didn't even
bother to stub out linux and mac parts. Rage!

http://codereview.chromium.org/3014005/diff/8001/2003
File chrome/browser/cocoa/animatable_image.h (right):

http://codereview.chromium.org/3014005/diff/8001/2003#newcode45
chrome/browser/cocoa/animatable_image.h:45: // but does not show the blank
animation window of the given size.
Nit: Maybe mention that |animationFrame| should usually be big enough to contain
both |startFrame| and |endFrame|.

Maybe the user should pass in |startFrame| and |endFrame| and this could figure
out its size itself?

http://codereview.chromium.org/3014005/diff/8001/2004
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/8001/2004#newcode38
chrome/browser/cocoa/animatable_image.mm:38: -
(void)startAnimationNamed:(NSString*)key {
Is it useful to pass in |key|?

http://codereview.chromium.org/3014005/diff/8001/2004#newcode74
chrome/browser/cocoa/animatable_image.mm:74: }
General point (maybe not necessary here because you remove the window when the
animation is done) (sorry if you know this already, took me a while to figure it
out):

CAAnimatons affect only the presentational value of a layer, not the model
value, and that only while the animation is running. I.e. if you have a layer
that has opacity 1 and you add an animation that fades it from 1 to 0, then the
opacity will return to 1 once the animation is done. That's usually not what you
want.

So you also need to change the model value, like this:

  [layer addAnimaton:opacityAnim forKey:@"foobarbar"];
  layer.opacity = 0;  // update model value

Now _that_ in turn creates an implicit animation for the model value change, so
the pattern is to do the model value change first and then add the explicit
animation at the same key the implicit animation is at, overriding it:

  layer.opacity = 0;  // update model value
  [layer addAnimaton:opacityAnim forKey:@"opacity"];  // add animation,
overwriting the implicit animation

Your code here doesn't follow that pattern, but if it looks ok, that's ok.

I haven't figured out how you do this for both |bounds| and |position| with an
animation group (only one key!), so I don't use an animation group for frame
changes but add the two animations under @"position" and @"bounds" respectively.

Maybe my mental model is still not quite right here, but it explains a lot of
stuff I previously didn't understand.

http://codereview.chromium.org/3014005/diff/8001/2004#newcode85
chrome/browser/cocoa/animatable_image.mm:85: [positionAnimation
setTimingFunction:mediaFunction];
It's sad that it's so hard to animate |frame|. I couldn't find an easier way
either tho :-(

http://codereview.chromium.org/3014005/diff/8001/2004#newcode102
chrome/browser/cocoa/animatable_image.mm:102: [animationGroup
setAnimations:animations];
animationGroup.animations = [NSArray arrayWithObjects:positionAnimation,
opacityAnimation, boundsAnimation, nil]?

http://codereview.chromium.org/3014005/diff/8001/2006
File chrome/browser/cocoa/download_started_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/8001/2006#newcode185
chrome/browser/cocoa/download_started_animation_mac.mm:185:
[controller->animation_ startAnimationNamed:@"downloadAnimation"];
Add "// Releases itself when it's done."

http://codereview.chromium.org/3014005/diff/8001/2007
File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/8001/2007#newcode60
chrome/browser/cocoa/popup_blocked_animation_mac.mm:60:
Source<TabContents>(tab_contents_));
do you need this? |animationComplete| ends up deleting this, and the registrar's
destructor should do this for you, right?

http://codereview.chromium.org/3014005/diff/8001/2007#newcode106
chrome/browser/cocoa/popup_blocked_animation_mac.mm:106:
animationFrame:[parentWindow frame]];
Sounds like the upper 1/4th of the rect is sufficient? If so, don't burn so much
ram for the backing store here and make the window only as large as necessary

http://codereview.chromium.org/3014005/diff/8001/2007#newcode117
chrome/browser/cocoa/popup_blocked_animation_mac.mm:117:
[BrowserWindowController class]]);
I usually put this below the cast:

BWC* foo = static_cast<BWC*>(bar);
DCHECK([foo isKindOfClass…]

That way, I can use the name "foo" in the DCHECK line already

http://codereview.chromium.org/3014005/diff/8001/2007#newcode119
chrome/browser/cocoa/popup_blocked_animation_mac.mm:119: [parentWindow
windowController];
Huh, is there no cast necessary?

[Robert Sesek, 2010-07-16 16:20:17]

http://codereview.chromium.org/3014005/diff/8001/2003
File chrome/browser/cocoa/animatable_image.h (right):

http://codereview.chromium.org/3014005/diff/8001/2003#newcode45
chrome/browser/cocoa/animatable_image.h:45: // but does not show the blank
animation window of the given size.
On 2010/07/16 00:06:10, Nico wrote:
> Nit: Maybe mention that |animationFrame| should usually be big enough to
contain
> both |startFrame| and |endFrame|.
> 
> Maybe the user should pass in |startFrame| and |endFrame| and this could
figure
> out its size itself?

Done.  I considered auto-calculating the |animationFrame|, but I think the
client should have the control here.

http://codereview.chromium.org/3014005/diff/8001/2004
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/8001/2004#newcode38
chrome/browser/cocoa/animatable_image.mm:38: -
(void)startAnimationNamed:(NSString*)key {
On 2010/07/16 00:06:10, Nico wrote:
> Is it useful to pass in |key|?

Probably not here. Removed.

http://codereview.chromium.org/3014005/diff/8001/2004#newcode74
chrome/browser/cocoa/animatable_image.mm:74: }
On 2010/07/16 00:06:10, Nico wrote:
> General point (maybe not necessary here because you remove the window when the
> animation is done) (sorry if you know this already, took me a while to figure
it
> out):

Thanks for the info.  Sometimes the animation would flicker at the end; this
seems to have fixed that.

http://codereview.chromium.org/3014005/diff/8001/2004#newcode102
chrome/browser/cocoa/animatable_image.mm:102: [animationGroup
setAnimations:animations];
On 2010/07/16 00:06:10, Nico wrote:
> animationGroup.animations = [NSArray arrayWithObjects:positionAnimation,
> opacityAnimation, boundsAnimation, nil]?

Dot notation not allowed.

http://codereview.chromium.org/3014005/diff/8001/2006
File chrome/browser/cocoa/download_started_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/8001/2006#newcode185
chrome/browser/cocoa/download_started_animation_mac.mm:185:
[controller->animation_ startAnimationNamed:@"downloadAnimation"];
On 2010/07/16 00:06:10, Nico wrote:
> Add "// Releases itself when it's done."

Done.

http://codereview.chromium.org/3014005/diff/8001/2007
File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/8001/2007#newcode60
chrome/browser/cocoa/popup_blocked_animation_mac.mm:60:
Source<TabContents>(tab_contents_));
On 2010/07/16 00:06:10, Nico wrote:
> do you need this? |animationComplete| ends up deleting this, and the
registrar's
> destructor should do this for you, right?

You're correct. Removed here and in the DSAM.

http://codereview.chromium.org/3014005/diff/8001/2007#newcode106
chrome/browser/cocoa/popup_blocked_animation_mac.mm:106:
animationFrame:[parentWindow frame]];
On 2010/07/16 00:06:10, Nico wrote:
> Sounds like the upper 1/4th of the rect is sufficient? If so, don't burn so
much
> ram for the backing store here and make the window only as large as necessary

Done.

http://codereview.chromium.org/3014005/diff/8001/2007#newcode117
chrome/browser/cocoa/popup_blocked_animation_mac.mm:117:
[BrowserWindowController class]]);
On 2010/07/16 00:06:10, Nico wrote:
> I usually put this below the cast:
> 
> BWC* foo = static_cast<BWC*>(bar);
> DCHECK([foo isKindOfClass…]
> 
> That way, I can use the name "foo" in the DCHECK line already

Done.

http://codereview.chromium.org/3014005/diff/8001/2007#newcode119
chrome/browser/cocoa/popup_blocked_animation_mac.mm:119: [parentWindow
windowController];
On 2010/07/16 00:06:10, Nico wrote:
> Huh, is there no cast necessary?

It compiles.

[Nico, 2010-07-16 16:55:35]

LG once CL is updated with a bug and the comment about |bounds| below is
addressed.

http://codereview.chromium.org/3014005/diff/14001/15003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15003#newcode117
chrome/browser/cocoa/animatable_image.mm:117: CGRectGetHeight([self endFrame]));
Is this missing a

  [layer setBounds:bounds]

? You don't use |bounds|.

http://codereview.chromium.org/3014005/diff/14001/15005
File chrome/browser/cocoa/download_started_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15005#newcode64
chrome/browser/cocoa/download_started_animation_mac.mm:64: [owner_
animationComplete];
Maybe add a comment "// This ends up deleting us."

http://codereview.chromium.org/3014005/diff/14001/15006
File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15006#newcode52
chrome/browser/cocoa/popup_blocked_animation_mac.mm:52: [owner_
animationComplete];
likewise

http://codereview.chromium.org/3014005/diff/14001/15006#newcode93
chrome/browser/cocoa/popup_blocked_animation_mac.mm:93: // Create the animation
object to be the size of the entire window. The
update comment

http://codereview.chromium.org/3014005/diff/14001/15010
File chrome/browser/views/popup_blocked_animation_win.cc (right):

http://codereview.chromium.org/3014005/diff/14001/15010#newcode9
chrome/browser/views/popup_blocked_animation_win.cc:9: // TODO(port): Implement.
NOTIMPLEMENTED()? Also, mention the bug number of the bug I asked you to file.
Also in the linux file

[Nico, 2010-07-16 16:56:56]

*once CL description

On Fri, Jul 16, 2010 at 9:55 AM,  <thakis@chromium.org> wrote:
> LG once CL is updated with a bug and the comment about |bounds| below is
> addressed.
>
>
> http://codereview.chromium.org/3014005/diff/14001/15003
> File chrome/browser/cocoa/animatable_image.mm (right):
>
> http://codereview.chromium.org/3014005/diff/14001/15003#newcode117
> chrome/browser/cocoa/animatable_image.mm:117: CGRectGetHeight([self
> endFrame]));
> Is this missing a
>
>  [layer setBounds:bounds]
>
> ? You don't use |bounds|.
>
> http://codereview.chromium.org/3014005/diff/14001/15005
> File chrome/browser/cocoa/download_started_animation_mac.mm (right):
>
> http://codereview.chromium.org/3014005/diff/14001/15005#newcode64
> chrome/browser/cocoa/download_started_animation_mac.mm:64: [owner_
> animationComplete];
> Maybe add a comment "// This ends up deleting us."
>
> http://codereview.chromium.org/3014005/diff/14001/15006
> File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):
>
> http://codereview.chromium.org/3014005/diff/14001/15006#newcode52
> chrome/browser/cocoa/popup_blocked_animation_mac.mm:52: [owner_
> animationComplete];
> likewise
>
> http://codereview.chromium.org/3014005/diff/14001/15006#newcode93
> chrome/browser/cocoa/popup_blocked_animation_mac.mm:93: // Create the
> animation object to be the size of the entire window. The
> update comment
>
> http://codereview.chromium.org/3014005/diff/14001/15010
> File chrome/browser/views/popup_blocked_animation_win.cc (right):
>
> http://codereview.chromium.org/3014005/diff/14001/15010#newcode9
> chrome/browser/views/popup_blocked_animation_win.cc:9: // TODO(port):
> Implement.
> NOTIMPLEMENTED()? Also, mention the bug number of the bug I asked you to
> file. Also in the linux file
>
> http://codereview.chromium.org/3014005/show
>

[Robert Sesek, 2010-07-16 18:56:00]

All nits addressed.

http://codereview.chromium.org/3014005/diff/14001/15003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15003#newcode117
chrome/browser/cocoa/animatable_image.mm:117: CGRectGetHeight([self endFrame]));
On 2010/07/16 16:55:35, Nico wrote:
> Is this missing a
> 
>   [layer setBounds:bounds]
> 
> ? You don't use |bounds|.

Do'h. This actually breaks the animation. Removed.

http://codereview.chromium.org/3014005/diff/14001/15005
File chrome/browser/cocoa/download_started_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15005#newcode64
chrome/browser/cocoa/download_started_animation_mac.mm:64: [owner_
animationComplete];
On 2010/07/16 16:55:35, Nico wrote:
> Maybe add a comment "// This ends up deleting us."

Done.

http://codereview.chromium.org/3014005/diff/14001/15006
File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15006#newcode52
chrome/browser/cocoa/popup_blocked_animation_mac.mm:52: [owner_
animationComplete];
On 2010/07/16 16:55:35, Nico wrote:
> likewise

Done.

http://codereview.chromium.org/3014005/diff/14001/15006#newcode93
chrome/browser/cocoa/popup_blocked_animation_mac.mm:93: // Create the animation
object to be the size of the entire window. The
On 2010/07/16 16:55:35, Nico wrote:
> update comment

Done.

[Nico, 2010-07-16 18:59:01]

http://codereview.chromium.org/3014005/diff/14001/15003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15003#newcode117
chrome/browser/cocoa/animatable_image.mm:117: CGRectGetHeight([self endFrame]));
On 2010/07/16 18:56:01, rsesek wrote:
> On 2010/07/16 16:55:35, Nico wrote:
> > Is this missing a
> > 
> >   [layer setBounds:bounds]
> > 
> > ? You don't use |bounds|.
> 
> Do'h. This actually breaks the animation. Removed.

I believe this breaks the animation because it schedules an implicit animation
which you don't override. If you don't use an animation group but instead do

[layer addAnimation:boundsAnimation forKey:@"bounds"];
[layer addAnimation:positionAnimation forKey:@"position"];

below, this should override the implicit animations (cf my original comment).
Does that work?

[Robert Sesek, 2010-07-16 19:18:34]

http://codereview.chromium.org/3014005/diff/14001/15003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/14001/15003#newcode117
chrome/browser/cocoa/animatable_image.mm:117: CGRectGetHeight([self endFrame]));
On 2010/07/16 18:59:02, Nico wrote:
> On 2010/07/16 18:56:01, rsesek wrote:
> > On 2010/07/16 16:55:35, Nico wrote:
> > > Is this missing a
> > > 
> > >   [layer setBounds:bounds]
> > > 
> > > ? You don't use |bounds|.
> > 
> > Do'h. This actually breaks the animation. Removed.
> 
> I believe this breaks the animation because it schedules an implicit animation
> which you don't override. If you don't use an animation group but instead do
> 
> [layer addAnimation:boundsAnimation forKey:@"bounds"];
> [layer addAnimation:positionAnimation forKey:@"position"];
> 
> below, this should override the implicit animations (cf my original comment).
> Does that work?

Nope. Removing them from the animation group also does not do what we want. I
also don't get why doing opacity with the implicit animation and then explicitly
through the animation group would work differently for doing the same with
bounds.

[Robert Sesek, 2010-07-19 16:58:36]

Did some research into CoreAnimation this weekend. I've removed the animation
group. Calling |-[CALayer setBounds:]| still does not do what's expected, but
I'm confident that it doesn't matter because we close the animating window right
as the animation ends.

[Nico, 2010-07-19 17:16:42]

On 2010/07/19 16:58:36, rsesek wrote:
> Did some research into CoreAnimation this weekend. I've removed the animation
> group. Calling |-[CALayer setBounds:]| still does not do what's expected, but
> I'm confident that it doesn't matter because we close the animating window
right
> as the animation ends.

I patched this in to investigate myself, but I don't see any animation for
blocked popups after patching this in

[Robert Sesek, 2010-07-19 17:49:43]

On 2010/07/19 17:16:42, Nico wrote:
> On 2010/07/19 16:58:36, rsesek wrote:
> > Did some research into CoreAnimation this weekend. I've removed the
animation
> > group. Calling |-[CALayer setBounds:]| still does not do what's expected,
but
> > I'm confident that it doesn't matter because we close the animating window
> right
> > as the animation ends.
> 
> I patched this in to investigate myself, but I don't see any animation for
> blocked popups after patching this in

The image was removed in Patch Set 5 for the trybots. I've added it back in 6.

[Nico, 2010-07-19 22:35:17]

http://codereview.chromium.org/3014005/diff/32001/33003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/32001/33003#newcode48
chrome/browser/cocoa/animatable_image.mm:48: [layer setContents:image_.get()];
contents should be a CGImageRef; sorry about not catching this earlier. I
recommend adding an NSImage->CGImageRef method somewhere and calling that.

It could look like


  NSSize s = [image_ size];
  CGContextRef bitmapCtx = CGBitmapContextCreate(NULL/*data - pass NULL to let
CG allocate the memory*/, 
                    s.width,  
                    s.height, 
                    8 /*bitsPerComponent*/, 
                    0 /*bytesPerRow - CG will calculate it for you if it's
allocating the data.  This might get padded out a bit for better alignment*/, 
                    [[NSColorSpace genericRGBColorSpace] CGColorSpace], 
                    kCGBitmapByteOrder32Host|kCGImageAlphaPremultipliedFirst);
  
  [NSGraphicsContext saveGraphicsState];
  [NSGraphicsContext setCurrentContext:[NSGraphicsContext
graphicsContextWithGraphicsPort:bitmapCtx flipped:NO]];
  [image_ drawInRect:NSMakeRect(0,0, s.width, s.height)
fromRect:NSZeroRect/*sentinel, means "the whole thing*/
operation:NSCompositeCopy fraction:1.0];
  [NSGraphicsContext restoreGraphicsState];

  CGImageRef cgImage = CGBitmapContextCreateImage(bitmapCtx);  // FIXME: leak
  CGContextRelease(bitmapCtx);

on 10.5 and call  CGImageForProposedRect:context:hints: on 10.6 or so.

http://codereview.chromium.org/3014005/diff/32001/33006
File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):

http://codereview.chromium.org/3014005/diff/32001/33006#newcode88
chrome/browser/cocoa/popup_blocked_animation_mac.mm:88: if (imageWidth >=
windowFrame.size.width ||
nswidth

http://codereview.chromium.org/3014005/diff/32001/33006#newcode89
chrome/browser/cocoa/popup_blocked_animation_mac.mm:89: imageHeight >=
windowFrame.size.height) {
nsheight

[Robert Sesek, 2010-07-19 22:50:30]

New CL up that rasterizes the PDF to a CGImage.

[Nico, 2010-07-19 22:52:08]

LG with the patch that works on 10.5.

Note that this LG is only for the code and not for the feature itself, which
requires an ok from the UI leads.

If you get ok from them, you should get a pdf version of the current popup
blocker icon and use that in the animation. (and let the animation end where the
icon is, once the icons match).

http://codereview.chromium.org/3014005/diff/32001/33003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/32001/33003#newcode105
chrome/browser/cocoa/animatable_image.mm:105: [layer setOpacity:[self
endOpacity]];
Please add a comment that this should ideally be done for |bounds| as well, but
that that breaks the animation for reasons we don't understand.

[Nico, 2010-07-19 23:00:35]

LG (with the same comments I sent with my previous LG)

http://codereview.chromium.org/3014005/diff/37001/38003
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3014005/diff/37001/38003#newcode149
chrome/browser/cocoa/animatable_image.mm:149: [NSGraphicsContext
restoreGraphicsState];
Please put this into a utility function somewhere at some time (not in this CL,
but this week if possible)