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

BUG=NONE
TEST=Build, and test through UI.

Review URL: http://codereview.chromium.org/7466016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@96314 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/cocoa/image_button_cell.h

@@ -21,6 +21,13 @@ enum ButtonState {
 
 } // namespace ImageButtonCell
 
+@protocol ImageButton
+@optional
+// Sent from an ImageButtonCell to it's view when the mouse enters or exits the
+// cell.
+- (void)mouseInsideStateDidChange:(BOOL)isInside;
+@end
+
 // A button cell that can disable a different image for each possible button
 // state. Images are specified by image IDs.
 @interface ImageButtonCell : NSButtonCell {

chrome/browser/ui/cocoa/image_button_cell.mm

@@ -138,10 +138,25 @@ const CGFloat kImageNoFocusAlpha = 0.65;
 - (void)setIsMouseInside:(BOOL)isMouseInside {
   if (isMouseInside_ != isMouseInside) {
     isMouseInside_ = isMouseInside;
-    [[self controlView] setNeedsDisplay:YES];
+    NSView<ImageButton>* control =
+        static_cast<NSView<ImageButton>*>([self controlView]);
+    if ([control respondsToSelector:@selector(mouseInsideStateDidChange:)]) {
+      [control mouseInsideStateDidChange:isMouseInside];
+    }
+    [control setNeedsDisplay:YES];
   }
 }
 
+- (void)setShowsBorderOnlyWhileMouseInside:(BOOL)show {
+  VLOG_IF(1, !show) << "setShowsBorderOnlyWhileMouseInside:NO ignored";
+}
+
+- (BOOL)showsBorderOnlyWhileMouseInside {
+  // Always returns YES so that buttons always get mouse tracking even when
+  // disabled. The reload button (and possibly others) depend on this.
+  return YES;
+}
+
 - (void)mouseEntered:(NSEvent*)theEvent {
   [self setIsMouseInside:YES];
 }

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

@@ -8,26 +8,18 @@
 
 #import <Cocoa/Cocoa.h>
 
-#import "base/memory/scoped_nsobject.h"
+#import "chrome/browser/ui/cocoa/image_button_cell.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_button.h"
 
 // ToolbarButton subclass which defers certain state changes when the mouse
 // is hovering over it.
 
-@interface ReloadButton : ToolbarButton {
+@interface ReloadButton : ToolbarButton<ImageButton> {
  @private
-  // Tracks whether the mouse is hovering for purposes of not making
-  // unexpected state changes.
-  BOOL isMouseInside_;
-  scoped_nsobject<NSTrackingArea> trackingArea_;
-
   // Timer used when setting reload mode while the mouse is hovered.
-  scoped_nsobject<NSTimer> pendingReloadTimer_;
+  NSTimer* pendingReloadTimer_;
 }
 
-// Returns YES if the mouse is currently inside the bounds.
-- (BOOL)isMouseInside;
-
 // Update the tag, and the image and tooltip to match.  If |anInt|
 // matches the current tag, no action is taken.  |anInt| must be
 // either |IDC_STOP| or |IDC_RELOAD|.
@@ -43,9 +35,4 @@
 
 @end
 
-@interface ReloadButton (PrivateTestingMethods)
-+ (void)setPendingReloadTimeout:(NSTimeInterval)seconds;
-- (NSTrackingArea*)trackingArea;
-@end
-
 #endif  // CHROME_BROWSER_UI_COCOA_TOOLBAR_RELOAD_BUTTON_H_

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,157 +19,164 @@ NSTimeInterval kPendingReloadTimeout = 1.35;
 
 }  // namespace
 
+@interface ReloadButton ()
+- (void)invalidatePendingReloadTimer;
+- (void)forceReloadState:(NSTimer *)timer;
+@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])) {
+    // Since this is not a custom view, -awakeFromNib won't be called twice.
+    [self awakeFromNib];
   }
-  [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 {
+  // If this view is moved to a new window, reset its state.
+  [self setIsLoading:NO force:YES];
+  [super viewWillMoveToWindow:newWindow];
 }
 
 - (void)awakeFromNib {
-  [self updateTrackingAreas];
-
   // 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();
   }
 }
 
+- (id)accessibilityAttributeValue:(NSString *)attribute {
+  if ([attribute isEqualToString:NSAccessibilityEnabledAttribute] &&
+      pendingReloadTimer_) {
+    return [NSNumber numberWithBool:NO];
+  } else {
+    return [super accessibilityAttributeValue:attribute];
+  }
+}
+
 - (void)setIsLoading:(BOOL)isLoading force:(BOOL)force {
   // Can always transition to stop mode.  Only transition to reload
   // 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];
-
-    // 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];
   }
+  [self setEnabled:pendingReloadTimer_ == nil];
 }
 
-- (void)forceReloadState {
+- (void)forceReloadState:(NSTimer *)timer {
+  DCHECK_EQ(timer, pendingReloadTimer_);
   [self setIsLoading:NO force:YES];
+  // Verify that |pendingReloadTimer_| is nil so it is not left dangling.
+  DCHECK(!pendingReloadTimer_);
 }
 
 - (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_) {
+      // If |pendingReloadTimer_| then the control is currently being
+      // drawn in a disabled state, so just return. The control is NOT actually
+      // disabled, otherwise mousetracking (courtesy of the NSButtonCell)
+      // would not work.
+      return YES;
+    } 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.]
+      BOOL ret = [super sendAction:theAction to:theTarget];
+      if (ret)
+        [self forceReloadState:pendingReloadTimer_];
+      return ret;
+    }
   }
 
   return [super sendAction:theAction to:theTarget];
 }
 
-- (void)mouseEntered:(NSEvent*)theEvent {
-  isMouseInside_ = YES;
-}
-
-- (void)mouseExited:(NSEvent*)theEvent {
-  isMouseInside_ = NO;
-
-  // Reload mode was requested during the hover.
-  if (pendingReloadTimer_)
-    [self forceReloadState];
-}
-
-- (BOOL)isMouseInside {
-  return isMouseInside_;
-}
-
 - (ViewID)viewID {
   return VIEW_ID_RELOAD_BUTTON;
 }
 
+- (void)mouseInsideStateDidChange:(BOOL)isInside {
+  [pendingReloadTimer_ fire];
+}
+
 @end  // ReloadButton
 
 @implementation ReloadButton (Testing)
@@ -179,8 +185,4 @@ NSTimeInterval kPendingReloadTimeout = 1.35;
   kPendingReloadTimeout = seconds;
 }
 
-- (NSTrackingArea*)trackingArea {
-  return trackingArea_;
-}
-
 @end

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

@@ -9,11 +9,16 @@
 #include "base/memory/scoped_nsobject.h"
 #include "chrome/app/chrome_command_ids.h"
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
+#import "chrome/browser/ui/cocoa/image_button_cell.h"
 #import "chrome/browser/ui/cocoa/test_event_utils.h"
 #import "testing/gtest_mac.h"
 #include "testing/platform_test.h"
 #import "third_party/ocmock/OCMock/OCMock.h"
 
+@interface ReloadButton (Testing)
++ (void)setPendingReloadTimeout:(NSTimeInterval)seconds;
+@end
+
 @protocol TargetActionMock <NSObject>
 - (void)anAction:(id)sender;
 @end
@@ -35,6 +40,18 @@ class ReloadButtonTest : public CocoaTest {
     [[test_window() contentView] addSubview:button_];
   }
 
+  bool IsMouseInside() {
+    return [[button_ cell] isMouseInside];
+  }
+
+  void MouseEnter() {
+    [[button_ cell] mouseEntered:nil];
+  }
+
+  void MouseExit() {
+    [[button_ cell] mouseExited:nil];
+  }
+
   ReloadButton* button_;
 };
 
@@ -42,12 +59,10 @@ TEST_VIEW(ReloadButtonTest, button_)
 
 // Test that mouse-tracking is setup and does the right thing.
 TEST_F(ReloadButtonTest, IsMouseInside) {
-  EXPECT_TRUE([[button_ trackingAreas] containsObject:[button_ trackingArea]]);
-
-  EXPECT_FALSE([button_ isMouseInside]);
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
-  [button_ mouseExited:nil];
+  EXPECT_FALSE(IsMouseInside());
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
+  MouseExit();
 }
 
 // Verify that multiple clicks do not result in multiple messages to
@@ -92,7 +107,7 @@ TEST_F(ReloadButtonTest, UpdateTag) {
 // Test that when forcing the mode, it takes effect immediately,
 // regardless of whether the mouse is hovering.
 TEST_F(ReloadButtonTest, SetIsLoadingForce) {
-  EXPECT_FALSE([button_ isMouseInside]);
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
 
   // Changes to stop immediately.
@@ -105,29 +120,29 @@ TEST_F(ReloadButtonTest, SetIsLoadingForce) {
 
   // Changes to stop immediately when the mouse is hovered, and
   // doesn't change when the mouse exits.
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:YES force:YES];
   EXPECT_EQ(IDC_STOP, [button_ tag]);
-  [button_ mouseExited:nil];
-  EXPECT_FALSE([button_ isMouseInside]);
+  MouseExit();
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_STOP, [button_ tag]);
 
   // Changes to reload immediately when the mouse is hovered, and
   // doesn't change when the mouse exits.
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:NO force:YES];
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
-  [button_ mouseExited:nil];
-  EXPECT_FALSE([button_ isMouseInside]);
+  MouseExit();
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
 }
 
 // Test that without force, stop mode is set immediately, but reload
 // is affected by the hover status.
 TEST_F(ReloadButtonTest, SetIsLoadingNoForceUnHover) {
-  EXPECT_FALSE([button_ isMouseInside]);
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
 
   // Changes to stop immediately when the mouse is not hovering.
@@ -140,22 +155,22 @@ TEST_F(ReloadButtonTest, SetIsLoadingNoForceUnHover) {
 
   // Changes to stop immediately when the mouse is hovered, and
   // doesn't change when the mouse exits.
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:YES force:NO];
   EXPECT_EQ(IDC_STOP, [button_ tag]);
-  [button_ mouseExited:nil];
-  EXPECT_FALSE([button_ isMouseInside]);
+  MouseExit();
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_STOP, [button_ tag]);
 
   // Does not change to reload immediately when the mouse is hovered,
   // changes when the mouse exits.
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:NO force:NO];
   EXPECT_EQ(IDC_STOP, [button_ tag]);
-  [button_ mouseExited:nil];
-  EXPECT_FALSE([button_ isMouseInside]);
+  MouseExit();
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
 }
 
@@ -178,21 +193,21 @@ TEST_F(ReloadButtonTest, DISABLED_SetIsLoadingNoForceTimeout) {
   const NSTimeInterval kShortTimeout = 0.1;
   [ReloadButton setPendingReloadTimeout:kShortTimeout];
 
-  EXPECT_FALSE([button_ isMouseInside]);
+  EXPECT_FALSE(IsMouseInside());
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
 
   // Move the mouse into the button and press it.
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:YES force:NO];
   EXPECT_EQ(IDC_STOP, [button_ tag]);
 
   // Does not change to reload immediately when the mouse is hovered.
-  EXPECT_TRUE([button_ isMouseInside]);
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:NO force:NO];
-  EXPECT_TRUE([button_ isMouseInside]);
+  EXPECT_TRUE(IsMouseInside());
   EXPECT_EQ(IDC_STOP, [button_ tag]);
-  EXPECT_TRUE([button_ isMouseInside]);
+  EXPECT_TRUE(IsMouseInside());
 
   // Spin event loop until the timeout passes.
   NSDate* pastTimeout = [NSDate dateWithTimeIntervalSinceNow:2 * kShortTimeout];
@@ -203,7 +218,7 @@ TEST_F(ReloadButtonTest, DISABLED_SetIsLoadingNoForceTimeout) {
 
   // Mouse is still hovered, button is in reload mode.  If the mouse
   // is no longer hovered, see comment at top of function.
-  EXPECT_TRUE([button_ isMouseInside]);
+  EXPECT_TRUE(IsMouseInside());
   EXPECT_EQ(IDC_RELOAD, [button_ tag]);
 }
 
@@ -214,7 +229,7 @@ TEST_F(ReloadButtonTest, StopAfterReloadSet) {
   [button_ setTarget:mock_target];
   [button_ setAction:@selector(anAction:)];
 
-  EXPECT_FALSE([button_ isMouseInside]);
+  EXPECT_FALSE(IsMouseInside());
 
   // Get to stop mode.
   [button_ setIsLoading:YES force:YES];
@@ -240,8 +255,8 @@ TEST_F(ReloadButtonTest, StopAfterReloadSet) {
 
   // If hover prevented reload mode immediately taking effect, clicks should do
   // nothing, because the button should be disabled.
-  [button_ mouseEntered:nil];
-  EXPECT_TRUE([button_ isMouseInside]);
+  MouseEnter();
+  EXPECT_TRUE(IsMouseInside());
   [button_ setIsLoading:NO force:NO];
   EXPECT_EQ(IDC_STOP, [button_ tag]);
   EXPECT_FALSE([button_ isEnabled]);