Mac: Fix history swiping bug on Yosemite.

There are 3 different sets of callbacks generated by AppKit in response to
swipe events. The previous code relied on assumptions about the timing of the
callbacks. It expected to receive the beginGestureWithEvent: callback before
receiving any touchesMovedWithEvent: callbacks. Yosemite AppKit broke this
assumption.

The new code stops using beginGestureWithEvent: except to determine edge cases
associated with the Magic Mouse, thus removing this implicit assumption.

Because history swiping direction is determined very early on in the life cycle
of the gesture, this CL adds some logic to ensure that the direction isn't
determined prematurely.

BUG=414103
TEST=History swiping should work as expected with both magic mouse and
trackpad.

Review URL: https://codereview.chromium.org/600743003

Cr-Commit-Position: refs/heads/master@{#296795}

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.h

@@ -120,10 +120,11 @@ enum RecognitionState {
   NSPoint gestureStartPoint_;
   // The current location of the fingers in the gesture.
   NSPoint gestureCurrentPoint_;
-  // A flag that indicates that there is an ongoing gesture.
-  // The method [NSEvent touchesMatchingPhase:inView:] is only valid for events
-  // that are part of a gesture.
+  // A flag that indicates that there is an ongoing gesture. Only used to
+  // determine whether swipe events are coming from a Magic Mouse.
   BOOL inGesture_;
+  // A flag that indicates that Chrome is receiving a series of touch events.
+  BOOL receivingTouches_;
   // Each time a new gesture begins, we must get a new start point.
   // This ivar determines whether the start point is valid.
   int gestureStartPointValid_;
@@ -141,16 +142,6 @@ enum RecognitionState {
 
   id<HistorySwiperDelegate> delegate_;
 
-  // Magic mouse and touchpad swipe events are identical except magic mouse
-  // events do not generate NSTouch callbacks. Since we rely on NSTouch
-  // callbacks to determine vertical scrolling, magic mouse swipe events use an
-  // entirely different set of logic.
-  //
-  // The two event types do not play well together. Just calling the API
-  // `[NSEvent trackSwipeEventWithOptions:]` will block touches{Began, Moved, *}
-  // callbacks for a non-deterministic period of time (even after the swipe has
-  // completed).
-  BOOL receivedTouch_;
   // Cumulative scroll delta since scroll gesture start. Only valid during
   // scroll gesture handling. Used for history swiping.
   NSSize mouseScrollDelta_;

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm

@@ -94,7 +94,7 @@ BOOL forceMagicMouse = NO;
   // History swiping is possible. By default, disallow rubberbanding.  If the
   // user has both started, and then cancelled history swiping for this
   // gesture, allow rubberbanding.
-  return inGesture_ && recognitionState_ == history_swiper::kCancelled;
+  return receivingTouches_ && recognitionState_ == history_swiper::kCancelled;
 }
 
 - (BOOL)canRubberbandRight:(NSView*)view {
@@ -107,23 +107,11 @@ BOOL forceMagicMouse = NO;
   // History swiping is possible. By default, disallow rubberbanding.  If the
   // user has both started, and then cancelled history swiping for this
   // gesture, allow rubberbanding.
-  return inGesture_ && recognitionState_ == history_swiper::kCancelled;
+  return receivingTouches_ && recognitionState_ == history_swiper::kCancelled;
 }
 
-// Is is theoretically possible for multiple simultaneous gestures to occur, if
-// the user has multiple input devices. There will be 2 beginGesture events, but
-// only 1 endGesture event. The unfinished gesture will continue to send
-// touchesMoved events, but when the gesture finishes there is not endGesture
-// callback. We ignore this case, because it is sufficiently unlikely to occur.
 - (void)beginGestureWithEvent:(NSEvent*)event {
   inGesture_ = YES;
-  ++currentGestureId_;
-  // Reset state pertaining to previous gestures.
-  gestureStartPointValid_ = NO;
-  receivedTouch_ = NO;
-  mouseScrollDelta_ = NSZeroSize;
-  beganEventUnconsumed_ = NO;
-  recognitionState_ = history_swiper::kPending;
 }
 
 - (void)endGestureWithEvent:(NSEvent*)event {
@@ -152,17 +140,13 @@ BOOL forceMagicMouse = NO;
 }
 
 - (void)updateGestureCurrentPointFromEvent:(NSEvent*)event {
-  // The points in an event are not valid unless the event is part of
-  // a gesture.
-  if (inGesture_) {
-    // Update the current point of the gesture.
-    gestureCurrentPoint_ = [self averagePositionInEvent:event];
-
-    // If the gesture doesn't have a start point, set one.
-    if (!gestureStartPointValid_) {
-      gestureStartPointValid_ = YES;
-      gestureStartPoint_ = gestureCurrentPoint_;
-    }
+  // Update the current point of the gesture.
+  gestureCurrentPoint_ = [self averagePositionInEvent:event];
+
+  // If the gesture doesn't have a start point, set one.
+  if (!gestureStartPointValid_) {
+    gestureStartPointValid_ = YES;
+    gestureStartPoint_ = gestureCurrentPoint_;
   }
 }
 
@@ -170,8 +154,14 @@ BOOL forceMagicMouse = NO;
 // called before the gesture begins, and the touches in an event are only
 // available after the gesture begins.
 - (void)touchesBeganWithEvent:(NSEvent*)event {
-  receivedTouch_ = YES;
-  // Do nothing.
+  receivingTouches_ = YES;
+  ++currentGestureId_;
+
+  // Reset state pertaining to previous gestures.
+  gestureStartPointValid_ = NO;
+  mouseScrollDelta_ = NSZeroSize;
+  beganEventUnconsumed_ = NO;
+  recognitionState_ = history_swiper::kPending;
 }
 
 - (void)touchesMovedWithEvent:(NSEvent*)event {
@@ -179,6 +169,8 @@ BOOL forceMagicMouse = NO;
 }
 
 - (void)touchesCancelledWithEvent:(NSEvent*)event {
+  receivingTouches_ = NO;
+
   if (![self processTouchEventForHistorySwiping:event])
     return;
 
@@ -186,6 +178,8 @@ BOOL forceMagicMouse = NO;
 }
 
 - (void)touchesEndedWithEvent:(NSEvent*)event {
+  receivingTouches_ = NO;
+
   if (![self processTouchEventForHistorySwiping:event])
     return;
 
@@ -204,8 +198,6 @@ BOOL forceMagicMouse = NO;
 }
 
 - (BOOL)processTouchEventForHistorySwiping:(NSEvent*)event {
-  receivedTouch_ = YES;
-
   NSEventType type = [event type];
   if (type != NSEventTypeBeginGesture && type != NSEventTypeEndGesture &&
       type != NSEventTypeGesture) {
@@ -293,6 +285,7 @@ BOOL forceMagicMouse = NO;
   // If the swipe is a backwards gesture, we need to invert progress.
   if (historySwipeDirection_ == history_swiper::kBackwards)
     progress *= -1;
+
   // If the user has directions reversed, we need to invert progress.
   if (historySwipeDirectionInverted_)
     progress *= -1;
@@ -533,13 +526,25 @@ BOOL forceMagicMouse = NO;
   if (!beganEventUnconsumed_)
     return NO;
 
-  if (!inGesture_)
-    return NO;
-
-  if (!receivedTouch_ || forceMagicMouse)
+  // Magic mouse and touchpad swipe events are identical except magic mouse
+  // events do not generate NSTouch callbacks. Since we rely on NSTouch
+  // callbacks to perform history swiping, magic mouse swipe events use an
+  // entirely different set of logic.
+  if ((inGesture_ && !receivingTouches_) || forceMagicMouse)
     return [self maybeHandleMagicMouseHistorySwiping:theEvent];
 
+  // The scrollWheel: callback is only relevant if it happens while the user is
+  // still actively using the touchpad.
+  if (!receivingTouches_)
+    return NO;
+
+  // TODO(erikchen): Ideally, the direction of history swiping should not be
+  // determined this early in a gesture, when it's unclear what the user is
+  // intending to do. Since it is determined this early, make sure that there
+  // is at least a minimal amount of horizontal motion.
   CGFloat xDelta = gestureCurrentPoint_.x - gestureStartPoint_.x;
+  if (fabs(xDelta) < 0.001)
+    return NO;
 
   BOOL isRightScroll = xDelta > 0;
   BOOL inverted = [self isEventDirectionInverted:theEvent];

chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_unit_test.mm

@@ -283,6 +283,7 @@ TEST_F(MacHistorySwiperTest, SwipeDiagonal) {
 
   startGestureInMiddle();
   moveGestureInMiddle();
+  moveGestureInMiddle();
   moveGestureAtPoint(makePoint(0.6, 0.59));
   endGestureAtPoint(makePoint(0.6, 0.59));
 
@@ -372,8 +373,12 @@ TEST_F(MacHistorySwiperTest, NoSwipe) {
 
   startGestureInMiddle();
   moveGestureInMiddle();
-  moveGestureAtPoint(makePoint(0.5, 0.5));
-  endGestureAtPoint(makePoint(0.5, 0.5));
+
+  // Starts the gesture.
+  moveGestureAtPoint(makePoint(0.44, 0.44));
+
+  // No movement.
+  endGestureAtPoint(makePoint(0.44, 0.44));
 
   EXPECT_EQ(begin_count_, 1);
   EXPECT_EQ(end_count_, 1);
@@ -405,3 +410,44 @@ TEST_F(MacHistorySwiperTest, TouchEventAfterGestureFinishes) {
   NSEvent* beganEvent = scrollWheelEventWithPhase(NSEventPhaseBegan);
   EXPECT_FALSE([historySwiper_ handleEvent:beganEvent]);
 }
+
+// The history swipe logic should be resilient against the timing of the
+// different callbacks that result from scrolling.
+TEST_F(MacHistorySwiperTest, SwipeRightEventOrdering) {
+  // These tests require 10.7+ APIs.
+  if (![NSEvent
+          respondsToSelector:@selector(isSwipeTrackingFromScrollEventsEnabled)])
+    return;
+
+  // Touches began.
+  NSEvent* scrollEvent = scrollWheelEventWithPhase(NSEventPhaseBegan);
+  NSEvent* event = mockEventWithPoint(makePoint(0.5, 0.5), NSEventTypeGesture);
+  [historySwiper_ touchesBeganWithEvent:event];
+  [historySwiper_ handleEvent:scrollEvent];
+  rendererACKForBeganEvent();
+
+  // Touches moved.
+  moveGestureAtPoint(makePoint(0.5, 0.5));
+  EXPECT_EQ(begin_count_, 0);
+  EXPECT_EQ(end_count_, 0);
+
+  // Touches moved.
+  moveGestureAtPoint(makePoint(0.52, 0.5));
+
+  // Begin gesture callback is delayed.
+  [historySwiper_ beginGestureWithEvent:event];
+
+  // Touches moved.
+  moveGestureAtPoint(makePoint(0.52, 0.5));
+
+  // Complete the rest of the gesture.
+  moveGestureAtPoint(makePoint(0.60, 0.5));
+  scrollEvent = scrollWheelEventWithPhase(NSEventPhaseChanged);
+  [historySwiper_ handleEvent:scrollEvent];
+  endGestureAtPoint(makePoint(0.70, 0.5));
+
+  EXPECT_EQ(begin_count_, 1);
+  EXPECT_EQ(end_count_, 1);
+  EXPECT_TRUE(navigated_right_);
+  EXPECT_FALSE(navigated_left_);
+}