Change the conditions on which the popup blocked animation is shown to be more reliable.

Change the conditions on which the popup blocked animation is shown to be more reliable.
This also prevents AnimatableImage from running after the window has been closed.

BUG=50000, 50395
TEST=Get a blocked popup. Open it. Animation not shown.
TEST=Go to a page that creates a popup on document.unload. Close the tab. Animation not shown.

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

[Nico, 2010-08-19 22:36:47]

LG, but "This also prevents on Mac the animation after the window has
been shown." parses weirdly for this non-native speaker.

On Thu, Aug 19, 2010 at 3:34 PM,  <rsesek@chromium.org> wrote:
> Reviewers: Nico, shess,
>
> Description:
> Change the conditions on which the popup blocked animation is shown to be
> more
> reliable.
> This also prevents on Mac the animation after the window has been shown.
>
> BUG=50000,50395
> TEST=Get a blocked popup. Open it. Animation not shown.
> TEST=Go to a page that creates a popup on document.unload. Close the tab.
> Animation not shown.
>
> Please review this at http://codereview.chromium.org/3169030/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M chrome/browser/cocoa/popup_blocked_animation_mac.mm
>  M chrome/browser/tab_contents/tab_contents.cc
>
>
> Index: chrome/browser/cocoa/popup_blocked_animation_mac.mm
> diff --git a/chrome/browser/cocoa/popup_blocked_animation_mac.mm
> b/chrome/browser/cocoa/popup_blocked_animation_mac.mm
> index
>
44a765b003a16d4fc78f9021edcbff675f3da594..b8b1640b039209e507315d43dd72984b9f5cbe3a
> 100644
> --- a/chrome/browser/cocoa/popup_blocked_animation_mac.mm
> +++ b/chrome/browser/cocoa/popup_blocked_animation_mac.mm
> @@ -169,6 +169,7 @@ class PopupBlockedAnimationObserver : public
> NotificationObserver {
>
>  - (void)animationComplete {
>   [[animation_ parentWindow] removeChildWindow:animation_];
> +  [animation_ orderOut:self];
>   [self release];
>  }
>
> Index: chrome/browser/tab_contents/tab_contents.cc
> diff --git a/chrome/browser/tab_contents/tab_contents.cc
> b/chrome/browser/tab_contents/tab_contents.cc
> index
>
2386994331e392ef19b5a842809d73a65907760f..33260e505e113f93357e185d2535f572362e43aa
> 100644
> --- a/chrome/browser/tab_contents/tab_contents.cc
> +++ b/chrome/browser/tab_contents/tab_contents.cc
> @@ -1019,8 +1019,6 @@ void
> TabContents::PopupNotificationVisibilityChanged(bool visible) {
>   content_settings_delegate_->SetPopupsBlocked(visible);
>   if (!dont_notify_render_view_)
>     render_view_host()->AllowScriptToClose(!visible);
> -  if (visible)
> -    PopupBlockedAnimation::Show(this);
>  }
>
>  gfx::NativeView TabContents::GetContentNativeView() const {
> @@ -1483,6 +1481,8 @@ void TabContents::AddPopup(TabContents* new_contents,
>       blocked_popups_ = new BlockedPopupContainer(this);
>     blocked_popups_->AddTabContents(new_contents, initial_pos);
>
> content_settings_delegate_->OnContentBlocked(CONTENT_SETTINGS_TYPE_POPUPS);
> +    if (!is_being_destroyed())
> +      PopupBlockedAnimation::Show(this);
>   }
>  }
>
>
>
>

[Robert Sesek, 2010-08-20 15:51:50]

I've addressed shess' comments from the bug.

[Nico, 2010-08-20 16:02:29]

http://codereview.chromium.org/3169030/diff/4001/5002
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3169030/diff/4001/5002#newcode135
chrome/browser/cocoa/animatable_image.mm:135: }
what's this needed for?

[Robert Sesek, 2010-08-20 16:25:46]

http://codereview.chromium.org/3169030/diff/4001/5002
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3169030/diff/4001/5002#newcode135
chrome/browser/cocoa/animatable_image.mm:135: }
On 2010/08/20 16:02:29, Nico wrote:
> what's this needed for?

Scott recommended it in the attached bug.  I do not think it's
necessary-necessary (as he put it), but this will explicitly stop all animations
if the animation has a long duration and the window is closed.

[Nico, 2010-08-20 16:27:12]

http://codereview.chromium.org/3169030/diff/4001/5002
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3169030/diff/4001/5002#newcode135
chrome/browser/cocoa/animatable_image.mm:135: }
On 2010/08/20 16:25:46, rsesek wrote:
> On 2010/08/20 16:02:29, Nico wrote:
> > what's this needed for?
> 
> Scott recommended it in the attached bug.  I do not think it's
> necessary-necessary (as he put it), but this will explicitly stop all
animations
> if the animation has a long duration and the window is closed.

Shouldn't closing the window release all layers in the window which will cancel
all animations anyway?

[Robert Sesek, 2010-08-20 19:01:47]

http://codereview.chromium.org/3169030/diff/4001/5002
File chrome/browser/cocoa/animatable_image.mm (right):

http://codereview.chromium.org/3169030/diff/4001/5002#newcode135
chrome/browser/cocoa/animatable_image.mm:135: }
On 2010/08/20 16:27:12, Nico wrote:
> On 2010/08/20 16:25:46, rsesek wrote:
> > On 2010/08/20 16:02:29, Nico wrote:
> > > what's this needed for?
> > 
> > Scott recommended it in the attached bug.  I do not think it's
> > necessary-necessary (as he put it), but this will explicitly stop all
> animations
> > if the animation has a long duration and the window is closed.
> 
> Shouldn't closing the window release all layers in the window which will
cancel
> all animations anyway?

Strictly speaking, this isn't necessary, but it's good safety.  The object that
runs the AnimatableImage usually releases itself from the |-windowWillClose:|
notification of the animation, which is broadcast after the
|-animationDidStop:finished:| (when the animation tells itself to |-close|). 
That way each object is responsible for closing itself.  An alternative path to
closing the object that runs the AnimatableImage is if it observes the
TabContents closing, it will tell the animation to cancel (which will close
itself) and the runner will release itself.

[Nico, 2010-08-20 21:40:48]

On 2010/08/20 19:01:47, rsesek wrote:
> http://codereview.chromium.org/3169030/diff/4001/5002
> File chrome/browser/cocoa/animatable_image.mm (right):
> 
> http://codereview.chromium.org/3169030/diff/4001/5002#newcode135
> chrome/browser/cocoa/animatable_image.mm:135: }
> On 2010/08/20 16:27:12, Nico wrote:
> > On 2010/08/20 16:25:46, rsesek wrote:
> > > On 2010/08/20 16:02:29, Nico wrote:
> > > > what's this needed for?
> > > 
> > > Scott recommended it in the attached bug.  I do not think it's
> > > necessary-necessary (as he put it), but this will explicitly stop all
> > animations
> > > if the animation has a long duration and the window is closed.
> > 
> > Shouldn't closing the window release all layers in the window which will
> cancel
> > all animations anyway?
> 
> Strictly speaking, this isn't necessary, but it's good safety.  The object
that
> runs the AnimatableImage usually releases itself from the |-windowWillClose:|
> notification of the animation, which is broadcast after the
> |-animationDidStop:finished:| (when the animation tells itself to |-close|). 
> That way each object is responsible for closing itself. 

When the animation is done, the window closes itself, and it's
autorelease-on-close, which releases the animation, which stops the animation.

> An alternative path to
> closing the object that runs the AnimatableImage is if it observes the
> TabContents closing, it will tell the animation to cancel (which will close
> itself) and the runner will release itself.

When the TabContents are closed, the window is released, which implicitly
cancels the animation.

Scott, can you say why you think this is required?

[Scott Hess, 2010-08-30 17:07:38]

[I cannot fully remember what I was talking about, but ...]

When I was experimenting with this, I was finding that ordering the window out
left the animation running, but now it was not visible, so I was still seeing
CPU usage.  It could be that my experiments were flawed.  My belief at the time
was that the animation layer was keeping a reference or two while the animation
was in progress, so things didn't unwind when _we_ released our last reference. 
Also, I think there might have been a difference between 10.5 and 10.6, because
I was seeing an obviously-pegged CPU in all of this, and I don't think Robert
was seeing that.

This was like a month ago, and I got distracted by some sort of M6 work, so I
could be completely mis-understanding what I was seeing.

-scott


On 2010/08/20 21:40:48, Nico wrote:
> On 2010/08/20 19:01:47, rsesek wrote:
> > http://codereview.chromium.org/3169030/diff/4001/5002
> > File chrome/browser/cocoa/animatable_image.mm (right):
> > 
> > http://codereview.chromium.org/3169030/diff/4001/5002#newcode135
> > chrome/browser/cocoa/animatable_image.mm:135: }
> > On 2010/08/20 16:27:12, Nico wrote:
> > > On 2010/08/20 16:25:46, rsesek wrote:
> > > > On 2010/08/20 16:02:29, Nico wrote:
> > > > > what's this needed for?
> > > > 
> > > > Scott recommended it in the attached bug.  I do not think it's
> > > > necessary-necessary (as he put it), but this will explicitly stop all
> > > animations
> > > > if the animation has a long duration and the window is closed.
> > > 
> > > Shouldn't closing the window release all layers in the window which will
> > cancel
> > > all animations anyway?
> > 
> > Strictly speaking, this isn't necessary, but it's good safety.  The object
> that
> > runs the AnimatableImage usually releases itself from the
|-windowWillClose:|
> > notification of the animation, which is broadcast after the
> > |-animationDidStop:finished:| (when the animation tells itself to |-close|).

> > That way each object is responsible for closing itself. 
> 
> When the animation is done, the window closes itself, and it's
> autorelease-on-close, which releases the animation, which stops the animation.
> 
> > An alternative path to
> > closing the object that runs the AnimatableImage is if it observes the
> > TabContents closing, it will tell the animation to cancel (which will close
> > itself) and the runner will release itself.
> 
> When the TabContents are closed, the window is released, which implicitly
> cancels the animation.
> 
> Scott, can you say why you think this is required?

[Scott Hess, 2010-08-30 17:08:37]

lgtm, i think.

http://codereview.chromium.org/3169030/diff/3002/11005
File chrome/browser/cocoa/popup_blocked_animation_mac.mm (right):

http://codereview.chromium.org/3169030/diff/3002/11005#newcode173
chrome/browser/cocoa/popup_blocked_animation_mac.mm:173: [animation_
orderOut:self];
Seeing this pattern in two files makes me wonder if it shouldn't just be pushed
into AnimatableImage.

[Robert Sesek, 2010-08-31 13:22:08]

Can I get another look?  I've confirmed that |-close|ing the window tears down
the layers immediately, so I've dispensed with removing sublayers.

[Scott Hess, 2010-08-31 14:29:47]

LGTM, though suggest minor comment tweaks.

http://codereview.chromium.org/3169030/diff/24001/25002
File chrome/browser/cocoa/download_started_animation_mac.mm (right):

http://codereview.chromium.org/3169030/diff/24001/25002#newcode166
chrome/browser/cocoa/download_started_animation_mac.mm:166: -
(void)closeAnimation {
+1 for the name change, -animationComplete never quite felt comfortable.

http://codereview.chromium.org/3169030/diff/24001/25002#newcode170
chrome/browser/cocoa/download_started_animation_mac.mm:170: // When the
animation closes, close the parent window.
Either this comment is too subtle for me to understand, or it's not describing
the following code.  [I'm fine w/o the comment, given that it just duplicates
the comment when the notification is registered.]

http://codereview.chromium.org/3169030/diff/24001/25002#newcode185
chrome/browser/cocoa/download_started_animation_mac.mm:185: // The |animation_|
releaes itself when done.
"releases", while we're in here.

[Nico, 2010-08-31 15:14:54]

LG

http://codereview.chromium.org/3169030/diff/24001/25001
File chrome/browser/cocoa/animatable_image_unittest.mm (right):

http://codereview.chromium.org/3169030/diff/24001/25001#newcode43
chrome/browser/cocoa/animatable_image_unittest.mm:43: [animation_ close];
this just checks that nothing crashes in the cancel codepath, not that it works,
right?

[Robert Sesek, 2010-08-31 15:15:52]

http://codereview.chromium.org/3169030/diff/24001/25001
File chrome/browser/cocoa/animatable_image_unittest.mm (right):

http://codereview.chromium.org/3169030/diff/24001/25001#newcode43
chrome/browser/cocoa/animatable_image_unittest.mm:43: [animation_ close];
On 2010/08/31 15:14:54, Nico wrote:
> this just checks that nothing crashes in the cancel codepath, not that it
works,
> right?

Correct.