Fix the Mac status bubble.

chrome/browser/ui/cocoa/status_bubble_mac.mm

Index: chrome/browser/ui/cocoa/status_bubble_mac.mm
diff --git a/chrome/browser/ui/cocoa/status_bubble_mac.mm b/chrome/browser/ui/cocoa/status_bubble_mac.mm
index c772f4818d909eebbf3d382e9dd1fef493a75c20..8fb8715c7cd6815b95b47f32a484a23985e580bf 100644
--- a/chrome/browser/ui/cocoa/status_bubble_mac.mm
+++ b/chrome/browser/ui/cocoa/status_bubble_mac.mm
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/mac/mac_util.h"
+#include "base/mac/scoped_block.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ -38,9 +39,6 @@ const int kMousePadding = 20;
 
 const int kTextPadding = 3;
 
-// The animation key used for fade-in and fade-out transitions.
-NSString* const kFadeAnimationKey = @"alphaValue";
-
 // The status bubble's maximum opacity, when fully faded in.
 const CGFloat kBubbleOpacity = 1.0;
 
@@ -63,17 +61,20 @@ const CGFloat kExpansionDurationSeconds = 0.125;
 
 }  // namespace
 
+#if !defined(MAC_OS_X_VERSION_10_7) || \
+    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
+@interface NSAnimationContext (LionSDK)

[Mark Mentovai, 2014/02/14 20:34:04]

I think we like to unify these in base/mac/sdk_forward_declarations.h now.

[Avi (use Gerrit), 2014/02/14 20:44:33]

Done.

++ (void)runAnimationGroup:(void (^)(NSAnimationContext *context))changes

[Mark Mentovai, 2014/02/14 20:34:04]

This file, like most Chrome code, prefers “Type* variable” to “Type *variable”.
You’ve mixed styles in your own code.

[Avi (use Gerrit), 2014/02/14 20:44:33]

Done.

+        completionHandler:(void (^)(void))completionHandler;
+@end
+#endif  // MAC_OS_X_VERSION_10_7
+
 @interface StatusBubbleAnimationDelegate : NSObject {
  @private
-  StatusBubbleMac* statusBubble_;  // weak; owns us indirectly
+  base::mac::ScopedBlock<void (^)(void)> completionHandler_;
 }
 
-- (id)initWithStatusBubble:(StatusBubbleMac*)statusBubble;
-
-// Invalidates this object so that no further calls will be made to
-// statusBubble_.  This should be called when statusBubble_ is released, to
-// prevent attempts to call into the released object.
-- (void)invalidate;
+- (id)initWithCompletionHandler:(void (^)(void))completionHandler;
 
 // CAAnimation delegate method
 - (void)animationDidStop:(CAAnimation*)animation finished:(BOOL)finished;
@@ -81,21 +82,67 @@ const CGFloat kExpansionDurationSeconds = 0.125;
 
 @implementation StatusBubbleAnimationDelegate
 
-- (id)initWithStatusBubble:(StatusBubbleMac*)statusBubble {
+- (id)initWithCompletionHandler:(void (^)(void))completionHandler {
   if ((self = [super init])) {
-    statusBubble_ = statusBubble;
+    completionHandler_.reset(completionHandler, base::scoped_policy::RETAIN);
   }
 
   return self;
 }
 
-- (void)invalidate {
-  statusBubble_ = NULL;
+- (void)animationDidStop:(CAAnimation*)animation finished:(BOOL)finished {
+  completionHandler_.get()();
 }
 
-- (void)animationDidStop:(CAAnimation*)animation finished:(BOOL)finished {
-  if (statusBubble_)
-    statusBubble_->AnimationDidStop(animation, finished);
+@end
+
+@interface StatusBubbleWindow : NSWindow {
+ @private
+  void (^completionHandler_)(void);
+}
+
+- (id)animationForKey:(NSString *)key;
+- (void)runAnimationGroup:(void (^)(NSAnimationContext *context))changes
+        completionHandler:(void (^)(void))completionHandler;
+@end
+
+@implementation StatusBubbleWindow
+
+- (id)animationForKey:(NSString *)key {
+  CAAnimation* animation = [super animationForKey:key];
+  // If completionHandler_ isn't nil, then this is the first of (potentially)
+  // multiple animations in a grouping; give it the completion handler. If
+  // completionHandler_ is nil, then some other animation was tagged with the
+  // completion handler.
+  if (completionHandler_) {

[Mark Mentovai, 2014/02/14 20:34:04]

Can you DCHECK in this block that you’re on 10.6? Or DCHECK that
NSAnimationContext has -runAnimationGroup:completionHandler:? I want a slightly
stronger guarantee that we don’t enter the funky (but neat) fallback path by
accident on systems where we shouldn’t need to.

[Avi (use Gerrit), 2014/02/14 20:44:33]

Sure.

(You do mean that NSAnimationContext DOESN'T have
-runAnimationGroup:completionHandler:, right?)

[Mark Mentovai, 2014/02/14 20:49:21]

Avi wrote:
Yeah, of course.

Remember when we were talking about that exercise yesterday and I said you
weren’t allowed to use “not?” I must have applied that same constraint to what I
wrote.

+    StatusBubbleAnimationDelegate* animation_delegate =
+        [[StatusBubbleAnimationDelegate alloc]
+             initWithCompletionHandler:completionHandler_];
+    [animation setDelegate:animation_delegate];
+    completionHandler_ = nil;

[Mark Mentovai, 2014/02/14 20:34:04]

Crafty.

[Avi (use Gerrit), 2014/02/14 20:44:33]

Thank you.

+  }
+  return animation;
+}
+
+- (void)runAnimationGroup:(void (^)(NSAnimationContext *context))changes
+        completionHandler:(void (^)(void))completionHandler {
+  if ([NSAnimationContext respondsToSelector:
+          @selector(runAnimationGroup:completionHandler:)]) {
+    [NSAnimationContext runAnimationGroup:changes
+                        completionHandler:completionHandler];
+  } else {
+    // Mac OS 10.6 does not have completion handler callbacks at the Cocoa
+    // level, only at the CoreAnimation level. So intercept calls made to
+    // -animationForKey: and tag one of the animations with a delegate that will
+    // execute the completion handler.
+    completionHandler_ = completionHandler;
+    [NSAnimationContext beginGrouping];
+    changes([NSAnimationContext currentContext]);
+    // At this point, -animationForKey should have been called by CoreAnimation
+    // to set up the animation to run. Verify this.
+    DCHECK(completionHandler_ == nil);
+    [NSAnimationContext endGrouping];
+  }
 }
 
 @end
@@ -103,6 +150,7 @@ const CGFloat kExpansionDurationSeconds = 0.125;
 StatusBubbleMac::StatusBubbleMac(NSWindow* parent, id delegate)
     : timer_factory_(this),
       expand_timer_factory_(this),
+      completion_handler_factory_(this),
       parent_(parent),
       delegate_(delegate),
       window_(nil),
@@ -120,7 +168,7 @@ StatusBubbleMac::~StatusBubbleMac() {
 
   Hide();
 
-  [[[window_ animationForKey:kFadeAnimationKey] delegate] invalidate];
+  completion_handler_factory_.InvalidateWeakPtrs();
   Detach();
   [window_ release];
   window_ = nil;
@@ -237,10 +285,7 @@ void StatusBubbleMac::Hide() {
       // An animation is in progress.  Cancel it by starting a new animation.
       // Use kMinimumTimeInterval to set the opacity as rapidly as possible.
       fade_out = true;
-      [NSAnimationContext beginGrouping];
-      [[NSAnimationContext currentContext] setDuration:kMinimumTimeInterval];
-      [[window_ animator] setAlphaValue:0.0];
-      [NSAnimationContext endGrouping];
+      AnimateWindowAlpha(0.0, kMinimumTimeInterval);
     }
   }
 
@@ -364,10 +409,11 @@ void StatusBubbleMac::UpdateDownloadShelfVisibility(bool visible) {
 void StatusBubbleMac::Create() {
   DCHECK(!window_);
 
-  window_ = [[NSWindow alloc] initWithContentRect:ui::kWindowSizeDeterminedLater
-                                        styleMask:NSBorderlessWindowMask
-                                          backing:NSBackingStoreBuffered
-                                            defer:YES];
+  window_ = [[StatusBubbleWindow alloc]
+      initWithContentRect:ui::kWindowSizeDeterminedLater
+                styleMask:NSBorderlessWindowMask
+                  backing:NSBackingStoreBuffered
+                    defer:YES];
   [window_ setMovableByWindowBackground:NO];
   [window_ setBackgroundColor:[NSColor clearColor]];
   [window_ setLevel:NSNormalWindowLevel];
@@ -388,21 +434,6 @@ void StatusBubbleMac::Create() {
   [window_ accessibilitySetOverrideValue:NSAccessibilityUnknownRole
                             forAttribute:NSAccessibilityRoleAttribute];
 
-  // Set a delegate for the fade-in and fade-out transitions to be notified
-  // when fades are complete.  The ownership model is for window_ to own
-  // animation_dictionary, which owns animation, which owns
-  // animation_delegate.
-  CAAnimation* animation = [[window_ animationForKey:kFadeAnimationKey] copy];
-  [animation autorelease];
-  StatusBubbleAnimationDelegate* animation_delegate =
-      [[StatusBubbleAnimationDelegate alloc] initWithStatusBubble:this];
-  [animation_delegate autorelease];
-  [animation setDelegate:animation_delegate];
-  NSMutableDictionary* animation_dictionary =
-      [NSMutableDictionary dictionaryWithDictionary:[window_ animations]];
-  [animation_dictionary setObject:animation forKey:kFadeAnimationKey];
-  [window_ setAnimations:animation_dictionary];
-
   [view setCornerFlags:kRoundedTopRightCorner];
   MouseMoved(gfx::Point(), false);
 }
@@ -427,23 +458,17 @@ void StatusBubbleMac::Detach() {
   [[window_ contentView] setThemeProvider:nil];
 }
 
-void StatusBubbleMac::AnimationDidStop(CAAnimation* animation, bool finished) {
+void StatusBubbleMac::AnimationDidStop() {
   DCHECK([NSThread isMainThread]);
   DCHECK(state_ == kBubbleShowingFadeIn || state_ == kBubbleHidingFadeOut);
   DCHECK(is_attached());
 
-  if (finished) {
-    // Because of the mechanism used to interrupt animations, this is never
-    // actually called with finished set to false.  If animations ever become
-    // directly interruptible, the check will ensure that state_ remains
-    // properly synchronized.
-    if (state_ == kBubbleShowingFadeIn) {
-      DCHECK_EQ([[window_ animator] alphaValue], kBubbleOpacity);
-      SetState(kBubbleShown);
-    } else {
-      DCHECK_EQ([[window_ animator] alphaValue], 0.0);
-      SetState(kBubbleHidden);
-    }
+  if (state_ == kBubbleShowingFadeIn) {
+    DCHECK_EQ([[window_ animator] alphaValue], kBubbleOpacity);
+    SetState(kBubbleShown);
+  } else {
+    DCHECK_EQ([[window_ animator] alphaValue], 0.0);
+    SetState(kBubbleHidden);
   }
 }
 
@@ -506,13 +531,24 @@ void StatusBubbleMac::Fade(bool show) {
   if (duration == 0.0)
     duration = kMinimumTimeInterval;
 
-  // This will cancel an in-progress transition and replace it with this fade.
-  [NSAnimationContext beginGrouping];
-  // Don't use the GTM additon for the "Steve" slowdown because this can happen
-  // async from user actions and the effects could be a surprise.
-  [[NSAnimationContext currentContext] setDuration:duration];
-  [[window_ animator] setAlphaValue:opacity];
-  [NSAnimationContext endGrouping];
+  // Cancel an in-progress transition and replace it with this fade.
+  AnimateWindowAlpha(opacity, duration);
+}
+
+void StatusBubbleMac::AnimateWindowAlpha(CGFloat alpha,
+                                         NSTimeInterval duration) {
+  completion_handler_factory_.InvalidateWeakPtrs();
+  base::WeakPtr<StatusBubbleMac> weak_ptr(
+      completion_handler_factory_.GetWeakPtr());
+  [window_
+      runAnimationGroup:^(NSAnimationContext* context) {
+          [context setDuration:duration];
+          [[window_ animator] setAlphaValue:alpha];
+      }
+      completionHandler:^{
+          if (weak_ptr)
+            weak_ptr->AnimationDidStop();
+      }];
 }
 
 void StatusBubbleMac::StartTimer(int64 delay_ms) {