MacViews: Handle three-finger swipe gestures for navigation.

chrome/browser/ui/views/frame/browser_view.cc

Index: chrome/browser/ui/views/frame/browser_view.cc
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index f4dbf87a9b3440d7ce19808cf32bf6b2c92f3fe7..222aa7a8d3016e2de0810fc1d5fc5e681ad4f80a 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -1980,6 +1980,26 @@ void BrowserView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   chrome::MaybeShowInvertBubbleView(this);
 }
 
+void BrowserView::OnGestureEvent(ui::GestureEvent* event) {
+#if defined(OS_MACOSX)
+  if (event->details().type() == ui::ET_GESTURE_SWIPE) {

[sky, 2017/02/10 19:08:33]

Can you structure this code to be a bit more readable, e.g.:

int command = NO_COMMAND (hopefully there is a define for it).
if (GetGestureCommand(*event, &command) && IsCommandE...)
  ExecuteCommand...

[themblsha, 2017/02/15 17:35:34]

base::Optional<int> should be a better way to do it rather than a NO_COMMAND
(and there seem to be no such thing in chrome_command_ids.h).

+    unsigned int command = 0;

[sky, 2017/02/10 19:08:33]

Aren't command ids ints?

[themblsha, 2017/02/15 17:35:34]

The code was adapted from the browser_window_controller.mm. Updated to an int.

+    if (event->details().swipe_left())

[tapted, 2017/02/13 11:26:24]

-[BrowserWindowController swipeWithEvent:] only triggers these commands when the
delta is over a threshold (i.e. 0.5). but GestureEvent sets these bools with any
strictly positive value - I don't think this will give a good UX.

[themblsha, 2017/02/15 17:35:34]

In my testing the -swipeWithEvent: is always called with a value of "+/-1"
without taking gesture speed into account. This matches Apple's documentation
that the delta should be simply taken as positive/negative.

Only a single event is send per three-finger swipe. The UX is exactly the same
as with the other three-finger swiping apps :-)

[tapted, 2017/02/15 22:25:52]

Can you comment about this where you construct the GestureEvent? - that's a
great breadcrumb to leave for a future reader.
oh weird - yeah even Safari has no feedback for this. (yuck)

[themblsha, 2017/02/16 15:27:49]

Done.
They're probably from the ancient times when the GestureRecognizers haven't been
invented yet. And some people probably enjoy the immediacy and relative lack of
feedback :-)

+      command = IDC_FORWARD;
+    else if (event->details().swipe_right())

[tapted, 2017/02/13 11:26:24]

I really think left/right around the right are around the wrong way here.

"left" should go back, "right" should go forward.

The problem is that GestureEventDetails does

      data_.swipe.left = delta_x < 0;
      data_.swipe.right = delta_x > 0;

but BrowserWindowController does

  if (deltaX > 0.5) {
    command = IDC_BACK;
  } else if (deltaX < -0.5) {
    command = IDC_FORWARD;
  }

I'd say it's the NSEvents that are "weird" so you'll need to put on a negative
sign before passing [event deltaX] to the GestureEvent constructor.

Please test deltaY manually to see what should be done with that (i.e. ensure
swipe_up()/down() actually correspond to whether you're swiping up or down

[themblsha, 2017/02/15 17:35:34]

I originally tested the behaviour to match Safari and Cocoa Chrome. Inverted the
deltaX / deltaY in the constructor call, thanks!

+      command = IDC_BACK;
+
+    if (chrome::IsCommandEnabled(browser(), command)) {
+      chrome::ExecuteCommandWithDisposition(
+          browser(), command, ui::DispositionFromEventFlags(event->flags()));
+    }
+    return;
+  }
+#endif  // OS_MACOSX
+
+  ClientView::OnGestureEvent(event);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // BrowserView, ui::AcceleratorTarget overrides: