Fix up reload button to not flicker, and simplify implementation.

chrome/browser/ui/cocoa/toolbar/reload_button.mm

Index: chrome/browser/ui/cocoa/toolbar/reload_button.mm
diff --git a/chrome/browser/ui/cocoa/toolbar/reload_button.mm b/chrome/browser/ui/cocoa/toolbar/reload_button.mm
index 10cd47ff4cb33a7819c6b33a9c012ba42d192823..6f968a4f6ba432d80e21ad1a9ed642d629f73aa9 100644
--- a/chrome/browser/ui/cocoa/toolbar/reload_button.mm
+++ b/chrome/browser/ui/cocoa/toolbar/reload_button.mm
@@ -5,7 +5,6 @@
 #import "chrome/browser/ui/cocoa/toolbar/reload_button.h"
 
 #include "chrome/app/chrome_command_ids.h"
-#import "chrome/browser/ui/cocoa/image_button_cell.h"
 #import "chrome/browser/ui/cocoa/view_id_util.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
@@ -20,77 +19,76 @@ NSTimeInterval kPendingReloadTimeout = 1.35;
 
 }  // namespace
 
+@interface ReloadButton ()
+- (void)commonInit;

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

-sharedInit is used in two cases, -commonInit is used in one.  Which honestly
makes me wonder if it wouldn't be better to use an init function in the
anonymous namespace.

[dmac, 2011/07/29 20:01:00]

Not sure what you meant by the anonymous namespace, but it turns out the
sharedInit is unnecessary and initWithFrame: can call awakeFromNib.

[Scott Hess - ex-Googler, 2011/08/04 23:49:44]

Mostly I meant that hidden objc methods like this aren't namespaced, which is
annoying.  It could have been a function in an anonymous namespace and then it
wouldn't have that problem (but would be 23% more annoying).  But no method at
all is better!

+- (void)invalidatePendingReloadTimer;
+@end
+
 @implementation ReloadButton
 
 + (Class)cellClass {
   return [ImageButtonCell class];
 }
 
-- (void)dealloc {
-  if (trackingArea_) {
-    [self removeTrackingArea:trackingArea_];
-    trackingArea_.reset();
+- (id)initWithFrame:(NSRect)frameRect {
+  if ((self = [super initWithFrame:frameRect])) {
+    [self commonInit];
   }
-  [super dealloc];
+  return self;
 }
 
-- (void)updateTrackingAreas {
-  // If the mouse is hovering when the tracking area is updated, the
-  // control could end up locked into inappropriate behavior for
-  // awhile, so unwind state.
-  if (isMouseInside_)
-    [self mouseExited:nil];
-
-  if (trackingArea_) {
-    [self removeTrackingArea:trackingArea_];
-    trackingArea_.reset();
-  }
-  trackingArea_.reset([[NSTrackingArea alloc]
-                        initWithRect:[self bounds]
-                             options:(NSTrackingMouseEnteredAndExited |
-                                      NSTrackingActiveInActiveApp)
-                               owner:self
-                            userInfo:nil]);
-  [self addTrackingArea:trackingArea_];
+- (void)viewWillMoveToWindow:(NSWindow *)newWindow {
+  // Since |pendingReloadTimer_| retains |self|, it should be invalidated
+  // if it is active when the view is moved. |newWindow| is nil when |self|
+  // is removed from the view hierarchy.

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

Is this being added for a specific reason?  I can see it in the remove case, but
if |newWindow| is non-nil, it should have other refs before and after.

Mostly I ask because I'm trying to think of what the state should be after this
is called.  Other calls to -invalidatePendingReloadTimer come directly before
code which explicitly sets the state.

[dmac, 2011/07/29 20:01:00]

Enforces a good state whereever the view is placed now.

+  [self invalidatePendingReloadTimer];
+  [super viewWillMoveToWindow:newWindow];
 }
 
 - (void)awakeFromNib {
-  [self updateTrackingAreas];
+  [self commonInit];
+}
 
+- (void)commonInit {
   // Don't allow multi-clicks, because the user probably wouldn't ever
   // want to stop+reload or reload+stop.
   [self setIgnoresMultiClick:YES];
 }
 
+- (void)invalidatePendingReloadTimer {
+  [pendingReloadTimer_ invalidate];
+  pendingReloadTimer_ = nil;
+}
+
 - (void)updateTag:(NSInteger)anInt {
   if ([self tag] == anInt)
     return;
 
   // Forcibly remove any stale tooltip which is being displayed.
   [self removeAllToolTips];
-
+  id cell = [self cell];
   [self setTag:anInt];
   if (anInt == IDC_RELOAD) {
-    [[self cell] setImageID:IDR_RELOAD
-             forButtonState:image_button_cell::kDefaultState];
-    [[self cell] setImageID:IDR_RELOAD_H
-             forButtonState:image_button_cell::kHoverState];
-    [[self cell] setImageID:IDR_RELOAD_P
-             forButtonState:image_button_cell::kPressedState];
+    [cell setImageID:IDR_RELOAD
+      forButtonState:image_button_cell::kDefaultState];
+    [cell setImageID:IDR_RELOAD_H
+      forButtonState:image_button_cell::kHoverState];
+    [cell setImageID:IDR_RELOAD_P
+      forButtonState:image_button_cell::kPressedState];
     // The stop button has a disabled image but the reload button doesn't. To
     // unset it we have to explicilty change the image ID to 0.
-    [[self cell] setImageID:0
-             forButtonState:image_button_cell::kDisabledState];
+    [cell setImageID:0
+      forButtonState:image_button_cell::kDisabledState];
     [self setToolTip:l10n_util::GetNSStringWithFixup(IDS_TOOLTIP_RELOAD)];
   } else if (anInt == IDC_STOP) {
-    [[self cell] setImageID:IDR_STOP
-             forButtonState:image_button_cell::kDefaultState];
-    [[self cell] setImageID:IDR_STOP_H
-             forButtonState:image_button_cell::kHoverState];
-    [[self cell] setImageID:IDR_STOP_P
-             forButtonState:image_button_cell::kPressedState];
-    [[self cell] setImageID:IDR_STOP_D
-             forButtonState:image_button_cell::kDisabledState];
+    [cell setImageID:IDR_STOP
+      forButtonState:image_button_cell::kDefaultState];
+    [cell setImageID:IDR_STOP_H
+      forButtonState:image_button_cell::kHoverState];
+    [cell setImageID:IDR_STOP_P
+      forButtonState:image_button_cell::kPressedState];
+    [cell setImageID:IDR_STOP_D
+      forButtonState:image_button_cell::kDisabledState];
     [self setToolTip:l10n_util::GetNSStringWithFixup(IDS_TOOLTIP_STOP)];
   } else {
     NOTREACHED();
@@ -102,73 +100,76 @@ NSTimeInterval kPendingReloadTimeout = 1.35;
   // mode if forced or if the mouse isn't hovering.  Otherwise, note
   // that reload mode is desired and disable the button.
   if (isLoading) {
-    pendingReloadTimer_.reset();
+    [self invalidatePendingReloadTimer];
     [self updateTag:IDC_STOP];
-    [self setEnabled:YES];
-  } else if (force || ![self isMouseInside]) {
-    pendingReloadTimer_.reset();
+  } else if (force) {
+    [self invalidatePendingReloadTimer];
     [self updateTag:IDC_RELOAD];

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

I _think_ that given the final else clause you added, now you could add !force
&& to the following if(), and then it would all work out.

[Basically, that would take the previous (force || !isMouseInside) -> (!force &&
isMouseInside) in the next if() clause.]

WAIT!  I _think_ that the final fall-through case in the previous code may have
actually been functional.  I think you could be called with !isLoading && !force
multiple times in some cases, so letting the existing timer continue was
important.  I think.  I'll try to remember the case, but it's been awhile.

[I was just going to suggest invalidating the timer at the top of the function,
but then was wondering why the previous code didn't release the timer at the top
of the function, and I think that was why.  You're right, there should be a
comment or something.]

[dmac, 2011/07/29 20:01:00]

This appears to be working for me (after pretty extensive testing). Have you
come up with any case it doesn't work?

-
-    // This button's cell may not have received a mouseExited event, and
-    // therefore it could still think that the mouse is inside the button.  Make
-    // sure the cell's sense of mouse-inside matches the local sense, to prevent
-    // drawing artifacts.
+  } else if ([self tag] == IDC_STOP &&
+             !pendingReloadTimer_ &&
+             [[self cell] isMouseInside]) {
     id cell = [self cell];
-    if ([cell respondsToSelector:@selector(setIsMouseInside:)])
-      [cell setIsMouseInside:[self isMouseInside]];
-    [self setEnabled:YES];
-  } else if ([self tag] == IDC_STOP && !pendingReloadTimer_) {
-    [self setEnabled:NO];
-    pendingReloadTimer_.reset(
-        [[NSTimer scheduledTimerWithTimeInterval:kPendingReloadTimeout
-                                          target:self
-                                        selector:@selector(forceReloadState)
-                                        userInfo:nil
-                                         repeats:NO] retain]);
+    [cell setImageID:IDR_STOP_D
+      forButtonState:image_button_cell::kDefaultState];
+    [cell setImageID:IDR_STOP_D
+      forButtonState:image_button_cell::kDisabledState];
+    [cell setImageID:IDR_STOP_D
+      forButtonState:image_button_cell::kHoverState];
+    [cell setImageID:IDR_STOP_D
+      forButtonState:image_button_cell::kPressedState];
+    pendingReloadTimer_ =
+        [NSTimer timerWithTimeInterval:kPendingReloadTimeout
+                                target:self
+                              selector:@selector(forceReloadState)
+                              userInfo:nil
+                               repeats:NO];
+    // Must add the timer to |NSRunLoopCommonModes| because
+    // it should run in |NSEventTrackingRunLoopMode| as well as
+    // |NSDefaultRunLoopMode|.
+    [[NSRunLoop currentRunLoop] addTimer:pendingReloadTimer_
+                                 forMode:NSRunLoopCommonModes];
+  } else {
+    [self invalidatePendingReloadTimer];
+    [self updateTag:IDC_RELOAD];
   }
 }
 
+- (BOOL)isEnabled {
+  return [super isEnabled] && !([self tag] == IDC_STOP && pendingReloadTimer_);

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

pendingReloadTimer_ should imply [self tag] == IDC_STOP.  It might make sense to
make pendingReloadTimer_ imply disabled more explicitly.

[dmac, 2011/07/29 20:01:00]

No longer needed due to mouse tracking having been cleaned up.

+}
+
 - (void)forceReloadState {
+  [self invalidatePendingReloadTimer];

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

-setIsLoading:force: already handles the timer.

[dmac, 2011/07/29 20:01:00]

Done, and added a bit more checking.

   [self setIsLoading:NO force:YES];
 }
 
 - (BOOL)sendAction:(SEL)theAction to:(id)theTarget {
   if ([self tag] == IDC_STOP) {
-    // When the timer is started, the button is disabled, so this
-    // should not be possible.
-    DCHECK(!pendingReloadTimer_.get());
-
-    // When the stop is processed, immediately change to reload mode,
-    // even though the IPC still has to bounce off the renderer and
-    // back before the regular |-setIsLoaded:force:| will be called.
-    // [This is how views and gtk do it.]
-    const BOOL ret = [super sendAction:theAction to:theTarget];
-    if (ret)
-      [self forceReloadState];
-    return ret;
+    if (pendingReloadTimer_) {
+      return YES;

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

In the old code, this couldn't happen because the button was disabled.  Now it
looks like that's being handled by short-circuiting - what does -isEnabled do
for the new code, then?

[dmac, 2011/07/29 20:01:00]

isEnabled has been removed.

+    } else {
+      // When the stop is processed, immediately change to reload mode,
+      // even though the IPC still has to bounce off the renderer and
+      // back before the regular |-setIsLoaded:force:| will be called.
+      // [This is how views and gtk do it.]
+      const BOOL ret = [super sendAction:theAction to:theTarget];
+      if (ret)
+        [self forceReloadState];
+      return ret;
+    }
   }
 
   return [super sendAction:theAction to:theTarget];
 }
 
-- (void)mouseEntered:(NSEvent*)theEvent {
-  isMouseInside_ = YES;
+- (ViewID)viewID {
+  return VIEW_ID_RELOAD_BUTTON;
 }
 
-- (void)mouseExited:(NSEvent*)theEvent {
-  isMouseInside_ = NO;
-
-  // Reload mode was requested during the hover.
-  if (pendingReloadTimer_)
+- (void)mouseInsideStateDidChange:(BOOL)isInside {
+  if (!isInside && pendingReloadTimer_) {
     [self forceReloadState];
-}
-
-- (BOOL)isMouseInside {
-  return isMouseInside_;
-}
-
-- (ViewID)viewID {
-  return VIEW_ID_RELOAD_BUTTON;
+  }

[Scott Hess - ex-Googler, 2011/07/21 00:48:40]

pendingReloadTimer_ implies that the new state is !isInside.

[pendingReloadTimer_ fire] would nicely make sure that what happens on exit
matches what happens when the timer fires.

[dmac, 2011/07/29 20:01:00]

Done. Good call.

 }
 
 @end  // ReloadButton
@@ -179,8 +180,4 @@ NSTimeInterval kPendingReloadTimeout = 1.35;
   kPendingReloadTimeout = seconds;
 }
 
-- (NSTrackingArea*)trackingArea {
-  return trackingArea_;
-}
-
 @end