Reduce hit test on ShowPress by moving event targeting to WebViewImpl

Source/web/WebViewImpl.cpp

Index: Source/web/WebViewImpl.cpp
diff --git a/Source/web/WebViewImpl.cpp b/Source/web/WebViewImpl.cpp
index 07d9e9eaf5ad1196ffc2606b3726b006ce9488b7..5c7446330ef8c2c1e10f3b99af3d7b8db18e8ff5 100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -71,7 +71,6 @@
 #include "core/page/DragData.h"
 #include "core/page/DragSession.h"
 #include "core/page/EventHandler.h"
-#include "core/page/EventWithHitTestResults.h"
 #include "core/page/FocusController.h"
 #include "core/page/FrameTree.h"
 #include "core/page/InjectedStyleSheets.h"
@@ -631,49 +630,37 @@ bool WebViewImpl::scrollBy(const WebFloatSize& delta, const WebFloatSize& veloci
 
 bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
 {
+    if (event.isScrollEvent())
+        return handleGestureScrollEvent(event);
+
     bool eventSwallowed = false;
     bool eventCancelled = false; // for disambiguation
 
-    // Special handling for slow-path fling gestures.

[Rick Byers, 2014/08/22 14:49:57]

These are really special WebGestureEvent types that blink doesn't know about. 
Would this be simpler if you left their handling here (instead of moving them
into handleGestureScrollEvent)?  Then you could have a single
PlatformGestureEventBuilder call below and call into handleGestureScrollEvent
based on PlatformGestureEvent::isScrollEvent (instead of needing to add an
isScrollEvent to WebGestureEvent).

This would allow us to have a very precise justification for isScrollEvent
methods - it's only to separate the different handling that we have in blink, a
detail that's irrelevant to code outside of blink.  Eg. I could imagine some
debate about the precise choices of what is and isn't scrolling related in
WebGestureEvent.

If they way you've done it helps make it cleaner somehow then that's OK with me
too.  But it seems like the change can be smaller/simpler...

[Zeeshan Qureshi, 2014/08/22 15:30:50]

Having a single source of truth is definitely better.

-    switch (event.type) {
-    case WebInputEvent::GestureFlingStart: {
-        if (mainFrameImpl()->frame()->eventHandler().isScrollbarHandlingGestures())
-            break;
-        m_client->cancelScheduledContentIntents();
-        m_positionOnFlingStart = WebPoint(event.x / pageScaleFactor(), event.y / pageScaleFactor());
-        m_globalPositionOnFlingStart = WebPoint(event.globalX, event.globalY);
-        m_flingModifier = event.modifiers;
-        m_flingSourceDevice = event.sourceDevice;
-        OwnPtr<WebGestureCurve> flingCurve = adoptPtr(Platform::current()->createFlingAnimationCurve(event.sourceDevice, WebFloatPoint(event.data.flingStart.velocityX, event.data.flingStart.velocityY), WebSize()));
-        ASSERT(flingCurve);
-        m_gestureAnimation = WebActiveGestureAnimation::createAtAnimationStart(flingCurve.release(), this);
-        scheduleAnimation();
-        eventSwallowed = true;
-
-        m_client->didHandleGestureEvent(event, eventCancelled);
-        return eventSwallowed;
-    }
-    case WebInputEvent::GestureFlingCancel:
-        if (endActiveFlingAnimation())
-            eventSwallowed = true;
+    PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event);
 
+    // Special handling for double tap as we don't want to hit test for it.

[Rick Byers, 2014/08/22 14:49:57]

Right - GestureDoubleTap is similar to GestureFlingStart/GestureFlingCancel in
that it's handled above 'core'.  Keeping all three in the same place that
earlies out of this function seems logical.

+    if (event.type == WebInputEvent::GestureDoubleTap) {
+        if (m_webSettings->doubleTapToZoomEnabled() && minimumPageScaleFactor() != maximumPageScaleFactor()) {
+            m_client->cancelScheduledContentIntents();
+            animateDoubleTapZoom(platformEvent.position());
+        }
+        // GestureDoubleTap is currently only used by Android for zooming. For WebCore,
+        // GestureTap with tap count = 2 is used instead. So we drop GestureDoubleTap here.
+        eventSwallowed = true;
         m_client->didHandleGestureEvent(event, eventCancelled);
         return eventSwallowed;
-    default:
-        break;
     }
 
-    PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event);
-
-    // FIXME: Remove redundant hit tests by pushing the call to EventHandler::targetGestureEvent
-    // up to this point and pass GestureEventWithHitTestResults around.
+    // Hit test across all frames and do touch adjustment as necessary for the event type.
+    GestureEventWithHitTestResults targetedEvent =
+        m_page->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(platformEvent);
 
     // Handle link highlighting outside the main switch to avoid getting lost in the
     // complicated set of cases handled below.
     switch (event.type) {
     case WebInputEvent::GestureShowPress:
         // Queue a highlight animation, then hand off to regular handler.
-        enableTapHighlightAtPoint(platformEvent);
+        enableTapHighlightAtPoint(targetedEvent);
         break;
     case WebInputEvent::GestureTapCancel:
     case WebInputEvent::GestureTap:
@@ -688,7 +675,8 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
     switch (event.type) {
     case WebInputEvent::GestureTap: {
         m_client->cancelScheduledContentIntents();
-        if (detectContentOnTouch(platformEvent.position())) {
+        // FIXME: Use targeted event here and save another hit test.
+        if (detectContentOnTouch(targetedEvent.event().position())) {
             eventSwallowed = true;
             break;
         }
@@ -724,7 +712,7 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
             }
         }
 
-        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
+        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(targetedEvent);
 
         if (m_selectPopup && m_selectPopup == selectPopup) {
             // That tap triggered a select popup which is the same as the one that
@@ -745,43 +733,83 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
         m_client->cancelScheduledContentIntents();
         m_page->contextMenuController().clearContextMenu();
         m_contextMenuAllowed = true;
-        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
+        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(targetedEvent);
         m_contextMenuAllowed = false;
 
         break;
     }
-    case WebInputEvent::GestureShowPress: {
+    case WebInputEvent::GestureShowPress:
         m_client->cancelScheduledContentIntents();
-        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
+    case WebInputEvent::GestureTapDown:
+    case WebInputEvent::GestureTapCancel:
+    case WebInputEvent::GestureTapUnconfirmed: {
+        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(targetedEvent);
         break;
     }
-    case WebInputEvent::GestureDoubleTap:
-        if (m_webSettings->doubleTapToZoomEnabled() && minimumPageScaleFactor() != maximumPageScaleFactor()) {
-            m_client->cancelScheduledContentIntents();
-            animateDoubleTapZoom(platformEvent.position());
-        }
-        // GestureDoubleTap is currently only used by Android for zooming. For WebCore,
-        // GestureTap with tap count = 2 is used instead. So we drop GestureDoubleTap here.
+    default:
+        ASSERT_NOT_REACHED();
+    }
+    m_client->didHandleGestureEvent(event, eventCancelled);
+    return eventSwallowed;
+}
+
+bool WebViewImpl::handleGestureScrollEvent(const WebGestureEvent& event)
+{
+    bool eventSwallowed = false;
+    bool eventCancelled = false; // for disambiguation
+
+    // Special handling for slow-path fling gestures.
+    switch (event.type) {
+    case WebInputEvent::GestureFlingStart: {
+        if (mainFrameImpl()->frame()->eventHandler().isScrollbarHandlingGestures())
+            break;
+        m_client->cancelScheduledContentIntents();
+        m_positionOnFlingStart = WebPoint(event.x / pageScaleFactor(), event.y / pageScaleFactor());
+        m_globalPositionOnFlingStart = WebPoint(event.globalX, event.globalY);
+        m_flingModifier = event.modifiers;
+        m_flingSourceDevice = event.sourceDevice;
+        OwnPtr<WebGestureCurve> flingCurve = adoptPtr(Platform::current()->createFlingAnimationCurve(event.sourceDevice, WebFloatPoint(event.data.flingStart.velocityX, event.data.flingStart.velocityY), WebSize()));
+        ASSERT(flingCurve);
+        m_gestureAnimation = WebActiveGestureAnimation::createAtAnimationStart(flingCurve.release(), this);
+        scheduleAnimation();
         eventSwallowed = true;
+
+        m_client->didHandleGestureEvent(event, eventCancelled);
+        return eventSwallowed;
+    }
+    case WebInputEvent::GestureFlingCancel:
+        if (endActiveFlingAnimation())
+            eventSwallowed = true;
+
+        m_client->didHandleGestureEvent(event, eventCancelled);
+        return eventSwallowed;
+    default:
         break;
+    }
+
+    switch (event.type) {
     case WebInputEvent::GestureScrollBegin:
     case WebInputEvent::GesturePinchBegin:
         m_client->cancelScheduledContentIntents();
-    case WebInputEvent::GestureTapDown:
     case WebInputEvent::GestureScrollEnd:
     case WebInputEvent::GestureScrollUpdate:
     case WebInputEvent::GestureScrollUpdateWithoutPropagation:
-    case WebInputEvent::GestureTapCancel:
-    case WebInputEvent::GestureTapUnconfirmed:
     case WebInputEvent::GesturePinchEnd:
     case WebInputEvent::GesturePinchUpdate:
     case WebInputEvent::GestureFlingStart: {
-        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
+        PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event);
+
+        // Scrolling-related gesture events invoke EventHandler recursively for each frame down
+        // the chain, doing a single-frame hit-test per frame. This matches handleWheelEvent.
+        // Perhaps we could simplify things by rewriting scroll handling to work inner frame
+        // out, and then unify with other gesture events.
+        eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureScrollEvent(platformEvent);
         break;
     }
     default:
         ASSERT_NOT_REACHED();
     }
+
     m_client->didHandleGestureEvent(event, eventCancelled);
     return eventSwallowed;
 }
@@ -1196,17 +1224,14 @@ static bool showsHandCursor(Node* node, LocalFrame* frame)
         || (cursor == CURSOR_AUTO && frame->eventHandler().useHandCursor(node, node->isLink()));
 }
 
-Node* WebViewImpl::bestTapNode(const PlatformGestureEvent& tapEvent)
+Node* WebViewImpl::bestTapNode(const GestureEventWithHitTestResults& targetedTapEvent)
 {
     TRACE_EVENT0("input", "WebViewImpl::bestTapNode");
 
     if (!m_page || !m_page->mainFrame())
         return 0;
 
-    // FIXME: Rely on earlier hit test instead of hit testing again.
-    GestureEventWithHitTestResults targetedEvent =
-        m_page->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(tapEvent, true);
-    Node* bestTouchNode = targetedEvent.hitTestResult().targetNode();
+    Node* bestTouchNode = targetedTapEvent.hitTestResult().targetNode();
 
     // We might hit something like an image map that has no renderer on it
     // Walk up the tree until we have a node with an attached renderer
@@ -1233,9 +1258,9 @@ Node* WebViewImpl::bestTapNode(const PlatformGestureEvent& tapEvent)
     return bestTouchNode;
 }
 
-void WebViewImpl::enableTapHighlightAtPoint(const PlatformGestureEvent& tapEvent)
+void WebViewImpl::enableTapHighlightAtPoint(const GestureEventWithHitTestResults& targetedTapEvent)
 {
-    Node* touchNode = bestTapNode(tapEvent);
+    Node* touchNode = bestTapNode(targetedTapEvent);
 
     WillBeHeapVector<RawPtrWillBeMember<Node> > highlightNodes;
     highlightNodes.append(touchNode);