mac: Only let two-finger-scrolling trigger history if web doesn't swallow gesture

chrome/browser/renderer_host/render_widget_host_view_mac.mm

Index: chrome/browser/renderer_host/render_widget_host_view_mac.mm
diff --git a/chrome/browser/renderer_host/render_widget_host_view_mac.mm b/chrome/browser/renderer_host/render_widget_host_view_mac.mm
index badd17fd62903c1344f9bf37e044bb381ca78ff9..a6aa41e739153e3c8da6d4ce69c63b305766d36f 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/chrome/browser/renderer_host/render_widget_host_view_mac.mm
@@ -20,6 +20,8 @@
 #import "chrome/browser/renderer_host/accelerated_plugin_view_mac.h"
 #import "chrome/browser/renderer_host/text_input_client_mac.h"
 #include "chrome/browser/spellchecker/spellchecker_platform_engine.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
 #import "chrome/browser/ui/cocoa/rwhvm_editcommand_helper.h"
 #import "chrome/browser/ui/cocoa/view_id_util.h"
 #include "chrome/common/render_messages.h"
@@ -56,6 +58,46 @@ using WebKit::WebMouseEvent;
 using WebKit::WebMouseWheelEvent;
 using WebKit::WebGestureEvent;
 
+// Forward-declare symbols that are part of the 10.7 SDK.
+#if !defined(MAC_OS_X_VERSION_10_7) || \
+    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
+
+enum {
+    NSEventPhaseNone        = 0, // event not associated with a phase.
+    NSEventPhaseBegan       = 0x1 << 0,
+    NSEventPhaseStationary  = 0x1 << 1,
+    NSEventPhaseChanged     = 0x1 << 2,
+    NSEventPhaseEnded       = 0x1 << 3,
+    NSEventPhaseCancelled   = 0x1 << 4,
+};
+typedef NSUInteger NSEventPhase;
+
+enum {
+     NSEventSwipeTrackingLockDirection = 0x1 << 0,
+     NSEventSwipeTrackingClampGestureAmount = 0x1 << 1
+};
+typedef NSUInteger NSEventSwipeTrackingOptions;
+
+
+@interface NSEvent(LionAPI)

[Robert Sesek, 2011/08/15 14:46:27]

nit: space before (

[Nico, 2011/08/15 16:39:46]

Done.

+// This is in the appkit 10.7 release notes, but not in

[Mark Mentovai, 2011/08/15 14:43:57]

If this comment were true, this declaration should come out of the #ifdef
because it would be needed with the 10.7 SDK too. However, I don’t believe this
comment.

mark@leo bash$ grep isSwipeTrackingFromScrollEventsEnabled
/Developer/SDKs/MacOSX10.7.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSEvent.h
+ (BOOL)isSwipeTrackingFromScrollEventsEnabled NS_AVAILABLE_MAC(10_7);
mark@leo bash$ grep isSwipeTrackingFromScrollEventsEnabled
/Xcode4.2/SDKs/MacOSX10.7.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSEvent.h
+ (BOOL)isSwipeTrackingFromScrollEventsEnabled NS_AVAILABLE_MAC(10_7);

/Developer is Xcode 4.1 4B110 and /Xcode4.2 is Xcode 4.2 4D75. Are you using a
different version? Can you recheck your headers? Based on what I have on my
system, I think this comment should be removed.

[Nico, 2011/08/15 16:39:46]

You're right. It's not documented at
http://developer.apple.com/library/mac/#documentation/cocoa/reference/Applica...
though. Removed comment.

+// the public nsevent headers?
++ (BOOL)isSwipeTrackingFromScrollEventsEnabled;
+
+- (NSEventPhase)phase;
+- (CGFloat)scrollingDeltaX;
+- (CGFloat)scrollingDeltaY;
+- (void)trackSwipeEventWithOptions:(NSEventSwipeTrackingOptions)options
+          dampenAmountThresholdMin:(CGFloat)minDampenThreshold
+                               max:(CGFloat)maxDampenThreshold
+                      usingHandler:(void (^)(CGFloat gestureAmount,

[Mark Mentovai, 2011/08/15 14:43:57]

You must #include "chrome/browser/mac/closure_blocks_leopard_compat.h" to ensure
that references to the block symbols are marked for weak import.

(Actually, the point at which this #include is required is the point of the use
of a ^block, not at this declaration, but you use a ^block later in this file,
and even if you had stuck the declaration in a header, it’d be a good idea to
#include the compatibility header there to ensure that anyone adhering to the
API got the right behavior.)

[Nico, 2011/08/15 16:39:46]

Good news: I was aware of that and thought I had done it :-P

+                                             NSEventPhase phase,
+                                             BOOL isComplete,
+                                             BOOL *stop))trackingHandler;
+@end
+
+#endif  // 10.7
+
 static inline int ToWebKitModifiers(NSUInteger flags) {
   int modifiers = 0;
   if (flags & NSControlKeyMask) modifiers |= WebInputEvent::ControlKey;
@@ -1018,6 +1060,22 @@ void RenderWidgetHostViewMac::SetVisuallyDeemphasized(const SkColor* color,
   // This is not used on mac.
 }
 
+void RenderWidgetHostViewMac::UnhandledWheelEvent(
+    const WebKit::WebMouseWheelEvent& event) {
+  [cocoa_view_ setGotUnhandledWheelEvent:YES];
+}
+
+void RenderWidgetHostViewMac::SetHasHorizontalScrollbar(
+    bool has_horizontal_scrollbar) {
+  [cocoa_view_ setHasHorizontalScrollbar:has_horizontal_scrollbar];
+}
+
+void RenderWidgetHostViewMac::SetScrollOffsetPinning(
+    bool is_pinned_to_left, bool is_pinned_to_right) {
+  [cocoa_view_ setPinnedLeft:is_pinned_to_left];
+  [cocoa_view_ setPinnedRight:is_pinned_to_right];
+}
+
 void RenderWidgetHostViewMac::ShutdownHost() {
   shutdown_factory_.RevokeAll();
   render_widget_host_->Shutdown();
@@ -1101,6 +1159,10 @@ void RenderWidgetHostViewMac::SetTextInputActive(bool active) {
 
 @synthesize selectedRange = selectedRange_;
 @synthesize markedRange = markedRange_;
+@synthesize gotUnhandledWheelEvent = gotUnhandledWheelEvent_;
+@synthesize isPinnedLeft = isPinnedLeft_;
+@synthesize isPinnedRight = isPinnedRight_;
+@synthesize hasHorizontalScrollbar = hasHorizontalScrollbar_;
 
 - (id)initWithRenderWidgetHostViewMac:(RenderWidgetHostViewMac*)r {
   self = [super initWithFrame:NSZeroRect];
@@ -1497,9 +1559,72 @@ void RenderWidgetHostViewMac::SetTextInputActive(bool active) {
     [NSCursor setHiddenUntilMouseMoves:YES];
 }
 
-- (void)scrollWheel:(NSEvent *)theEvent {
+- (void)scrollWheel:(NSEvent*)theEvent {
   [self cancelChildPopups];
 
+  bool canUseLionApis = [theEvent respondsToSelector:@selector(phase)];

[Mark Mentovai, 2011/08/15 14:43:57]

Stick with a BOOL for this one because that’s the type that the thing you’re
calling returns.

You can leave the bools below because those are just logic and you don’t care
about matching return types.

[Nico, 2011/08/15 16:39:46]

Done.

+
+  // Scroll events always go to the web first, and can only trigger history
+  // swiping if they come back unhandled.
+  if (canUseLionApis && [theEvent phase] == NSEventPhaseBegan) {

[Mark Mentovai, 2011/08/15 14:43:57]

Is there any reason you don’t wrap this block and the next into a larger block
named “if (canUseLionApis)”? Because…

(continued on line 1574)

+    totalScrollDelta_ = NSZeroSize;
+    gotUnhandledWheelEvent_ = false;

[Mark Mentovai, 2011/08/15 14:43:57]

This property is a BOOL, so use NO.

+  }
+
+  if (canUseLionApis &&  // Must be the first condition!

[Mark Mentovai, 2011/08/15 14:43:57]

(continued from line 1569)

…it’d eliminate this tricky “must be the first condition!” thing. I don’t see
the sense in checking canUseLionApis twice when its value won’t change between
the two checks.

[Robert Sesek, 2011/08/15 14:46:27]

Is there any reason to not put this and the previous block in an outer |if
(canUseLionApis)|?

[Nico, 2011/08/15 16:39:46]

The reason was indentation. I moved the history tracking into its own function,
so Done.

+      gotUnhandledWheelEvent_ &&
+      [NSEvent isSwipeTrackingFromScrollEventsEnabled] &&

[Robert Sesek, 2011/08/15 14:46:27]

Doing this the proper way means that you can use this. Yay!

+      [theEvent phase] == NSEventPhaseChanged) {
+    totalScrollDelta_.width += [theEvent scrollingDeltaX];
+    totalScrollDelta_.height += [theEvent scrollingDeltaY];
+
+    bool isHorizontalGesture =

[Robert Sesek, 2011/08/15 14:46:27]

über/optional-nit: Cocoa BOOLs?

+      fabs(totalScrollDelta_.width) > fabs(totalScrollDelta_.height);

[Mark Mentovai, 2011/08/15 14:43:57]

#include <cmath> and use std::abs instead of fabs.

[Robert Sesek, 2011/08/15 14:46:27]

std::abs

[Nico, 2011/08/15 16:39:46]

Done.

+
+    bool isRightScroll = [theEvent scrollingDeltaX] < 0;
+    bool goForward = isRightScroll;
+
+    if (isHorizontalGesture &&
+        // If "forward" is inactive and the user rubber-bands to the right,
+        // "isPinnedLeft" will be false.  When the user then rubber-bands to
+        // the left in the same gesture, that should trigger history
+        // immediately if there's no scrollbar, hence the check for
+        // hasHorizontalScrollbar_.
+        // One would think we have to check canGoBack / canGoForward here, but
+        // that's actually done in the renderer
+        // (ChromeClientImpl::shouldRubberBand()), when it decides if it should
+        // rubberband or send back an event unhandled.
+        (((isPinnedLeft_ || !hasHorizontalScrollbar_) && !isRightScroll) ||

[Mark Mentovai, 2011/08/15 14:43:57]

I think you can simplify this entire thing by pulling !hasHorizontalScrollbar
out of this portion and listing it only once.

[Nico, 2011/08/15 16:39:46]

Done.

+            ((isPinnedRight_ || !hasHorizontalScrollbar_) && isRightScroll))) {
+      // The way this api works: gestureAmount is between -1 and 1 (float). If
+      // the user does the gesture for more than about 25% (i.e. < -0.25 or >
+      // 0.25) and then lets go, it is accepted, we get a NSEventPhaseEnded,
+      // and after that the block is called with amounts animating towards 1
+      // (or -1, depending on the direction).  If the user lets go below that
+      // threshold, we get NSEventPhaseCancelled, and the amount animates
+      // toward 0.
+      [theEvent trackSwipeEventWithOptions:0
+                 dampenAmountThresholdMin:-1

[Mark Mentovai, 2011/08/15 14:43:57]

Nit: align colons.

[Mark Mentovai, 2011/08/15 14:43:57]

Do you know how AppKit uses these values? I suppose that -1 and 1 is fine for
how we’re (ab)using this, but “real” values would make for a smoother experience
for things that put all scrolling through this, or animating CoverFlow, or that
sort of thing.

[Nico, 2011/08/15 16:39:46]

Done.

[Nico, 2011/08/15 16:39:46]

I don't really understand what these values do. I tried -1,1 and -500,500 and
couldn't feel a difference. Will play with this again once I have some UI.

+                                      max:1
+                             usingHandler:^(CGFloat gestureAmount,
+                                            NSEventPhase phase,
+                                            BOOL isComplete,

[Robert Sesek, 2011/08/15 14:46:27]

Why do you not reference isComplete in this? Are you saving that for the UI
portion?

[Nico, 2011/08/15 16:39:46]

Yes. The way this works is that as soon as the user lets go their fingers, we
get an NSEventPhaseEnded, and then the block is called with |gestureAmount|
animating towards 1, and only then isComplete is set to true. Doing the
navigation on isComplete is too late. (With UI, we'll do the navigation on
NSEventPhaseEnded, and then continue to show the gesture UI until isComplete
becomes true)

+                                            BOOL *stop) {
+          // |gestureAmount| obeys -[NSEvent isDirectionInvertedFromDevice]
+          // automatically.
+          // TODO(thakis): UI.
+          Browser* browser = BrowserList::GetLastActive();
+          if (phase == NSEventPhaseEnded && browser) {
+            if (goForward)
+              browser->GoForward(CURRENT_TAB);
+            else
+              browser->GoBack(CURRENT_TAB);
+          }
+        }];
+      return;
+    }
+  }
+
   const WebMouseWheelEvent& event =
       WebInputEventFactory::mouseWheelEvent(theEvent, self);
   if (renderWidgetHostView_->render_widget_host_)