Fix crash on bookmark button delete.

Fix a bunch of "unrecognized selector sent to instance" NSLogs.

BUG=http://crbug.com/20937, http://crbug.com/20813
TEST=Right-click on a bookmark button and delete it.
Repeat a few times.  Make sure we don't crash.
Launch from the command line (.../Chromium.app/Contents/MacOS/Chromium)
Move the mouse around the toolbar hovering over the buttons and the chevron.
Make sure no "does not respond to selector" messages printed.

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

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

3 files changed, 75 insertions, 6 deletions

diff --git a/chrome/browser/cocoa/toolbar_controller.h b/chrome/browser/cocoa/toolbar_controller.h
index a442d88..d86b1fc 100644
--- a/chrome/browser/cocoa/toolbar_controller.h
+++ b/chrome/browser/cocoa/toolbar_controller.h
@@ -68,7 +68,13 @@ class ToolbarView;
 
   // Tracking area for mouse enter/exit/moved in the toolbar.
   scoped_nsobject<NSTrackingArea> trackingArea_;
-  NSButton* hoveredButton_;  // weak. Button under the mouse cursor.
+
+  // We retain/release the hover button since interaction with the
+  // button may make it go away (e.g. delete menu option over a
+  // bookmark button).  Thus this variable is not weak.  The
+  // hoveredButton_ is required to have an NSCell that responds to
+  // setMouseInside:animate:.
+  NSButton* hoveredButton_;
 
   // The ordering is important for unit tests. If new items are added or the
   // ordering is changed, make sure to update |-toolbarViews| and the
@@ -141,6 +147,8 @@ class ToolbarView;
 - (void)showOptionalHomeButton;
 - (void)showOptionalPageWrenchButtons;
 - (gfx::Rect)autocompletePopupPosition;
+// Return a hover button for the current event.
+- (NSButton*)hoverButtonForEvent:(NSEvent*)theEvent;
 @end
 
 #endif  // CHROME_BROWSER_COCOA_TOOLBAR_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/toolbar_controller.mm b/chrome/browser/cocoa/toolbar_controller.mm
index 4127615..f6aab53 100644
--- a/chrome/browser/cocoa/toolbar_controller.mm
+++ b/chrome/browser/cocoa/toolbar_controller.mm
@@ -193,17 +193,30 @@ class PrefObserverBridge : public NotificationObserver {
 }
 - (void)mouseExited:(NSEvent*)theEvent {
   [[hoveredButton_ cell] setMouseInside:NO animate:YES];
+  [hoveredButton_ release];
   hoveredButton_ = nil;
 }
 
-- (void)mouseMoved:(NSEvent *)theEvent {
-  NSButton *targetView = (NSButton *)[[self view]
-                                      hitTest:[theEvent locationInWindow]];
-  if (![targetView isKindOfClass:[NSButton class]]) targetView = nil;
+- (NSButton*)hoverButtonForEvent:(NSEvent*)theEvent {
+  NSButton* targetView = (NSButton *)[[self view]
+                                       hitTest:[theEvent locationInWindow]];
+
+  // Only interpret the view as a hoverButton_ if it's both button and has a
+  // button cell that cares.  GradientButtonCell derived cells care.
+  if (([targetView isKindOfClass:[NSButton class]]) &&
+      ([[targetView cell]
+         respondsToSelector:@selector(setMouseInside:animate:)]))
+    return targetView;
+  return nil;
+}
+
+- (void)mouseMoved:(NSEvent*)theEvent {
+  NSButton* targetView = [self hoverButtonForEvent:theEvent];
   if (hoveredButton_ != targetView) {
     [[hoveredButton_ cell] setMouseInside:NO animate:YES];
     [[targetView cell] setMouseInside:YES animate:YES];
-    hoveredButton_ = targetView;
+    [hoveredButton_ release];
+    hoveredButton_ = [targetView retain];
   }
 }
 
diff --git a/chrome/browser/cocoa/toolbar_controller_unittest.mm b/chrome/browser/cocoa/toolbar_controller_unittest.mm
index 4bc085c..21f9e61 100644
--- a/chrome/browser/cocoa/toolbar_controller_unittest.mm
+++ b/chrome/browser/cocoa/toolbar_controller_unittest.mm
@@ -8,6 +8,7 @@
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #import "chrome/browser/cocoa/cocoa_test_helper.h"
+#import "chrome/browser/cocoa/gradient_button_cell.h"
 #import "chrome/browser/cocoa/toolbar_controller.h"
 #import "chrome/browser/cocoa/view_resizer_pong.h"
 #include "chrome/common/pref_names.h"
@@ -15,6 +16,25 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
+// An NSView that fakes out hitTest:.
+@interface HitView : NSView {
+  id hitTestReturn_;
+}
+@end
+
+@implementation HitView
+
+- (void)setHitTestReturn:(id)rtn {
+  hitTestReturn_ = rtn;
+}
+
+- (NSView *)hitTest:(NSPoint)aPoint {
+  return hitTestReturn_;
+}
+
+@end
+
+
 namespace {
 
 class ToolbarControllerTest : public PlatformTest {
@@ -274,6 +294,34 @@ TEST_F(ToolbarControllerTest, AutocompletePopupPosition) {
   EXPECT_GE(popupFrame.bottom(), NSMinY(locationFrame));
 }
 
+TEST_F(ToolbarControllerTest, HoverButtonForEvent) {
+  scoped_nsobject<HitView> view([[HitView alloc]
+                                  initWithFrame:NSMakeRect(0,0,100,100)]);
+  [bar_ setView:view];
+  NSEvent* event = [NSEvent mouseEventWithType:NSMouseMoved
+                                      location:NSMakePoint(10,10)
+                                 modifierFlags:0
+                                     timestamp:0
+                                  windowNumber:0
+                                       context:nil
+                                   eventNumber:0
+                                    clickCount:0
+                                      pressure:0.0];
+
+  // NOT a match.
+  [view setHitTestReturn:bar_.get()];
+  EXPECT_FALSE([bar_ hoverButtonForEvent:event]);
+
+  // Not yet...
+  scoped_nsobject<NSButton> button([[NSButton alloc] init]);
+  [view setHitTestReturn:button];
+  EXPECT_FALSE([bar_ hoverButtonForEvent:event]);
+
+  // Now!
+  scoped_nsobject<GradientButtonCell> cell([[GradientButtonCell alloc] init]);
+  [button setCell:cell.get()];
+  EXPECT_TRUE([bar_ hoverButtonForEvent:nil]);
+}
 
 
 }  // namespace