Fix stuck highlight state when dragging other bookmarks folder, or when dragging something from the download shelf.

DraggingButton.mm was setting up non-draggable buttons to be ready for drag.

DownloadShelfButtons never called endDrag.

BUG=40594
TEST=Try to drag the Other Bookmarks button.  It shouldn't stay highlighted.

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

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

4 files changed, 216 insertions, 80 deletions

diff --git a/chrome/browser/cocoa/bookmark_button.mm b/chrome/browser/cocoa/bookmark_button.mm
index 4be967a..fea9d25 100644
--- a/chrome/browser/cocoa/bookmark_button.mm
+++ b/chrome/browser/cocoa/bookmark_button.mm
@@ -96,12 +96,6 @@ static const CGFloat kDragImageOpacity = 0.7;
   [super endDrag];
 }
 
-- (void)draggedImage:(NSImage*)anImage
-             endedAt:(NSPoint)aPoint
-           operation:(NSDragOperation)operation {
-  [self endDrag];
-}
-
 - (NSDragOperation)draggingSourceOperationMaskForLocal:(BOOL)isLocal {
   return isLocal ? NSDragOperationCopy | NSDragOperationMove
                  : NSDragOperationCopy;
diff --git a/chrome/browser/cocoa/draggable_button.h b/chrome/browser/cocoa/draggable_button.h
index ed9a94f..eee719a 100644
--- a/chrome/browser/cocoa/draggable_button.h
+++ b/chrome/browser/cocoa/draggable_button.h
@@ -10,21 +10,24 @@
 @interface DraggableButton : NSButton {
  @private
   BOOL draggable_;     // Is this a draggable type of button?
-  BOOL mayDragStart_;  // Set to YES on mouse down, NO on up or drag.
-  BOOL beingDragged_;
-
-  // Initial mouse-down to prevent a hair-trigger drag.
-  NSPoint initialMouseDownLocation_;
 }
 
 // Enable or disable dragability for special buttons like "Other Bookmarks".
 @property BOOL draggable;
 
-// Called when a drag starts. Subclasses must override this.
+// Called when a drag should start. Subclasses must override this to do any
+// pasteboard manipulation and begin the drag, usually with
+// -dragImage:at:offset:event:.  Subclasses must call one of the blocking
+// -drag* methods of NSView when overriding this method.
 - (void)beginDrag:(NSEvent*)dragEvent;
 
-// Subclasses should call this method to notify DraggableButton when a drag is
-// over.
+@end  // @interface DraggableButton
+
+@interface DraggableButton (Private)
+
+// Resets the draggable state of the button after dragging is finished.  This is
+// called by DraggableButton when the beginDrag call returns, it should not be
+// called by the subclass.
 - (void)endDrag;
 
-@end  // @interface DraggableButton
+@end  // @interface DraggableButton(Private)
diff --git a/chrome/browser/cocoa/draggable_button.mm b/chrome/browser/cocoa/draggable_button.mm
index 4ede10e..b45c2a8 100644
--- a/chrome/browser/cocoa/draggable_button.mm
+++ b/chrome/browser/cocoa/draggable_button.mm
@@ -13,6 +13,7 @@ namespace {
 // TODO(viettrungluu): Do we want common, standard code for drag hysteresis?
 const CGFloat kWebDragStartHysteresisX = 5.0;
 const CGFloat kWebDragStartHysteresisY = 5.0;
+const CGFloat kDragExpirationTimeout = 1.0;
 
 }
 
@@ -33,10 +34,81 @@ const CGFloat kWebDragStartHysteresisY = 5.0;
   }
   return self;
 }
+   
+// Determine whether a mouse down should turn into a drag; started as copy of
+// NSTableView code.
+- (BOOL)dragShouldBeginFromMouseDown:(NSEvent*)mouseDownEvent
+                      withExpiration:(NSDate*)expiration
+                         xHysteresis:(float)xHysteresis
+                         yHysteresis:(float)yHysteresis {
+  if ([mouseDownEvent type] != NSLeftMouseDown) {
+    return NO;
+  }
+
+  NSEvent* nextEvent = nil;
+  NSEvent* firstEvent = nil;
+  NSEvent* dragEvent = nil;
+  NSEvent* mouseUp = nil;
+  BOOL dragIt = NO;
+
+  while ((nextEvent = [[self window]
+      nextEventMatchingMask:(NSLeftMouseUpMask | NSLeftMouseDraggedMask)
+                  untilDate:expiration
+                     inMode:NSEventTrackingRunLoopMode
+                    dequeue:YES]) != nil) {
+    if (firstEvent == nil) {
+      firstEvent = nextEvent;
+    }
+    if ([nextEvent type] == NSLeftMouseDragged) {
+      float deltax = ABS([nextEvent locationInWindow].x -
+          [mouseDownEvent locationInWindow].x);
+      float deltay = ABS([nextEvent locationInWindow].y -
+          [mouseDownEvent locationInWindow].y);
+      dragEvent = nextEvent;
+      if (deltax >= xHysteresis) {
+        dragIt = YES;
+        break;
+      }
+      if (deltay >= yHysteresis) {
+        dragIt = YES;
+        break;
+      }
+    } else if ([nextEvent type] == NSLeftMouseUp) {
+      mouseUp = nextEvent;
+      break;
+    }
+  }
+
+  // Since we've been dequeuing the events (If we don't, we'll never see
+  // the mouse up...), we need to push some of the events back on. 
+  // It makes sense to put the first and last drag events and the mouse
+  // up if there was one.
+  if (mouseUp != nil) {
+    [NSApp postEvent:mouseUp atStart:YES];
+  }
+  if (dragEvent != nil) {
+    [NSApp postEvent:dragEvent atStart:YES];
+  }
+  if (firstEvent != mouseUp && firstEvent != dragEvent) {
+    [NSApp postEvent:firstEvent atStart:YES];
+  }
+
+  return dragIt;
+}
+
+- (BOOL)dragShouldBeginFromMouseDown:(NSEvent*)mouseDownEvent
+                      withExpiration:(NSDate*)expiration {
+  return [self dragShouldBeginFromMouseDown:mouseDownEvent
+                             withExpiration:expiration
+                                xHysteresis:kWebDragStartHysteresisX
+                                yHysteresis:kWebDragStartHysteresisY];
+}
 
 - (void)mouseUp:(NSEvent*)theEvent {
-  // Make sure that we can't start a drag until we see a mouse down again.
-  mayDragStart_ = NO;
+  if (!draggable_) {
+    [super mouseUp:theEvent];
+    return;
+  }
 
   // There are non-drag cases where a mouseUp: may happen
   // (e.g. mouse-down, cmd-tab to another application, move mouse,
@@ -45,48 +117,24 @@ const CGFloat kWebDragStartHysteresisY = 5.0;
                                 fromView:[[self window] contentView]];
   if (NSPointInRect(viewLocal, [self bounds])) {
     [self performClick:self];
-  } else {
-    // Make sure an ESC to end a drag doesn't trigger 2 endDrags.
-    if (beingDragged_) {
-      [self endDrag];
-    } else {
-      [super mouseUp:theEvent];
-    }
   }
 }
 
 // Mimic "begin a click" operation visually.  Do NOT follow through
 // with normal button event handling.
 - (void)mouseDown:(NSEvent*)theEvent {
-  mayDragStart_ = YES;
-  [[self cell] setHighlighted:YES];
-  initialMouseDownLocation_ = [theEvent locationInWindow];
-}
-
-// Return YES if we have crossed a threshold of movement after
-// mouse-down when we should begin a drag.  Else NO.
-- (BOOL)hasCrossedDragThreshold:(NSEvent*)theEvent {
-  NSPoint currentLocation = [theEvent locationInWindow];
-
-  if ((abs(currentLocation.x - initialMouseDownLocation_.x) >
-       kWebDragStartHysteresisX) ||
-      (abs(currentLocation.y - initialMouseDownLocation_.y) >
-       kWebDragStartHysteresisY)) {
-    return YES;
+  if (draggable_) {
+    [[self cell] setHighlighted:YES];
+    NSDate* date = [NSDate dateWithTimeIntervalSinceNow:kDragExpirationTimeout];
+    if ([self dragShouldBeginFromMouseDown:theEvent
+                            withExpiration:date]) {
+      [self beginDrag:theEvent];
+      [self endDrag];
+    } else {
+      [super mouseDown:theEvent];
+    }
   } else {
-    return NO;
-  }
-}
-
-- (void)mouseDragged:(NSEvent*)theEvent {
-  if (beingDragged_) {
-    [super mouseDragged:theEvent];
-  } else if (draggable_ && mayDragStart_ &&
-             [self hasCrossedDragThreshold:theEvent]) {
-    // Starting drag. Never start another drag until another mouse down.
-    mayDragStart_ = NO;
-    beingDragged_ = YES;
-    [self beginDrag:theEvent];
+    [super mouseDown:theEvent];
   }
 }
 
@@ -96,8 +144,7 @@ const CGFloat kWebDragStartHysteresisY = 5.0;
 }
 
 - (void)endDrag {
-  beingDragged_ = NO;
   [[self cell] setHighlighted:NO];
 }
 
-@end
+@end  // @interface DraggableButton
diff --git a/chrome/browser/cocoa/draggable_button_unittest.mm b/chrome/browser/cocoa/draggable_button_unittest.mm
index 9b892cc..6742721 100644
--- a/chrome/browser/cocoa/draggable_button_unittest.mm
+++ b/chrome/browser/cocoa/draggable_button_unittest.mm
@@ -5,41 +5,133 @@
 #include "base/scoped_nsobject.h"
 #import "chrome/browser/cocoa/bookmark_button.h"
 #import "chrome/browser/cocoa/cocoa_test_helper.h"
+#import "chrome/browser/cocoa/test_event_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
-NSEvent* Event(const NSPoint point, const NSEventType type) {
-  static NSUInteger eventNumber = 0;  // thx shess
-  return [NSEvent mouseEventWithType:type
-                            location:point
-                       modifierFlags:0
-                           timestamp:0
-                        windowNumber:183  // picked out of thin air.
-                             context:nil
-                         eventNumber:eventNumber++
-                          clickCount:1
-                            pressure:0.0];
+@interface TestableDraggableButton : DraggableButton {
+  NSUInteger dragCount_;
+  BOOL wasTriggered_;
 }
+- (void)trigger:(id)sender;
+- (BOOL)wasTriggered;
+- (NSUInteger)dragCount;
+@end
+
+@implementation TestableDraggableButton
+- (id)initWithFrame:(NSRect)frame {
+  if ((self = [super initWithFrame:frame])) {
+    dragCount_ = 0;
+    wasTriggered_ = NO;
+  }
+  return self;
+}
+- (void)beginDrag:(NSEvent*)theEvent {
+  dragCount_++;
+}
+
+- (void)trigger:(id)sender {
+  wasTriggered_ = YES;
+}
+
+- (BOOL)wasTriggered {
+  return wasTriggered_;
+}
+
+- (NSUInteger)dragCount {
+  return dragCount_;
+}
+@end
+
+class DraggableButtonTest : public CocoaTest {};
 
 // Make sure the basic case of "click" still works.
-TEST(DraggableButtonTest, DownUp) {
-  scoped_nsobject<NSMutableArray> array;
-  array.reset([[NSMutableArray alloc] init]);
-  [array addObject:@"foo"];
-  [array addObject:@"bar"];
+TEST_F(DraggableButtonTest, DownUp) {
+  scoped_nsobject<TestableDraggableButton> button(
+      [[TestableDraggableButton alloc] initWithFrame:NSMakeRect(0,0,500,500)]);
+  [[test_window() contentView] addSubview:button.get()];
+  [button setTarget:button];
+  [button setAction:@selector(trigger:)];
+  EXPECT_FALSE([button wasTriggered]);
+  NSEvent* downEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(10,10),
+                                          NSLeftMouseDown, 0);
+  NSEvent* upEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(10,10),
+                                          NSLeftMouseUp, 0);
+  [NSApp postEvent:upEvent atStart:YES];
+  [test_window() sendEvent:downEvent];
+  EXPECT_TRUE([button wasTriggered]);  // confirms target/action fired
+}
 
-  scoped_nsobject<DraggableButton> button;
-  button.reset([[DraggableButton alloc] initWithFrame:NSMakeRect(0,0,500,500)]);
+TEST_F(DraggableButtonTest, DraggableHysteresis) {
+  scoped_nsobject<TestableDraggableButton> button(
+      [[TestableDraggableButton alloc] initWithFrame:NSMakeRect(0,0,500,500)]);
+  [[test_window() contentView] addSubview:button.get()];
+  NSEvent* downEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(10,10),
+                                          NSLeftMouseDown,
+                                          0);
+  NSEvent* firstMove =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(11,11),
+                                          NSLeftMouseDragged,
+                                          0);
+  NSEvent* firstUpEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(11,11),
+                                          NSLeftMouseUp,
+                                          0);
+  NSEvent* secondMove =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(100,100),
+                                          NSLeftMouseDragged,
+                                          0);
+  NSEvent* secondUpEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(100,100),
+                                          NSLeftMouseUp,
+                                          0);
+  // If the mouse only moves one pixel in each direction
+  // it should not cause a drag.
+  [NSApp postEvent:firstUpEvent atStart:YES];
+  [NSApp postEvent:firstMove atStart:YES];
+  [button mouseDown:downEvent];
+  EXPECT_EQ(0U, [button dragCount]);
 
-  [button setTarget:array.get()];
-  [button setAction:@selector(removeAllObjects)];
-  EXPECT_FALSE([[button cell] isHighlighted]);
+  // If the mouse moves > 5 pixels in either direciton
+  // it should cause a drag.
+  [NSApp postEvent:secondUpEvent atStart:YES];
+  [NSApp postEvent:secondMove atStart:YES];
+  [button mouseDown:downEvent];
+  EXPECT_EQ(1U, [button dragCount]);
+}
+
+TEST_F(DraggableButtonTest, ResetState) {
+  scoped_nsobject<TestableDraggableButton> button(
+      [[TestableDraggableButton alloc] initWithFrame:NSMakeRect(0,0,500,500)]);
+  [[test_window() contentView] addSubview:button.get()];
+  NSEvent* downEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(10,10),
+                                          NSLeftMouseDown,
+                                          0);
+  NSEvent* moveEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(100,100),
+                                          NSLeftMouseDragged,
+                                          0);
+  NSEvent* upEvent =
+      test_event_utils::MouseEventAtPoint(NSMakePoint(100,100),
+                                          NSLeftMouseUp,
+                                          0);
+  // If the mouse moves > 5 pixels in either direciton it should cause a drag.
+  [NSApp postEvent:upEvent atStart:YES];
+  [NSApp postEvent:moveEvent atStart:YES];
+  [button mouseDown:downEvent];
 
-  NSEvent* downEvent(Event(NSMakePoint(10,10), NSLeftMouseDown));
-  NSEvent* upEvent(Event(NSMakePoint(10,10), NSLeftMouseDown));
+  // The button should not be highlighted after the drag finishes.
+  EXPECT_FALSE([[button cell] isHighlighted]);
+  EXPECT_EQ(1U, [button dragCount]);
+  
+  // We should be able to initiate another drag immediately after the first one.
+  [NSApp postEvent:upEvent atStart:YES];
+  [NSApp postEvent:moveEvent atStart:YES];
   [button mouseDown:downEvent];
-  EXPECT_TRUE([[button cell] isHighlighted]);
-  [button mouseUp:upEvent];
+  EXPECT_EQ(2U, [button dragCount]);
   EXPECT_FALSE([[button cell] isHighlighted]);
-  EXPECT_FALSE([array count]);  // confirms target/action fired
 }