Skip to content

Commit

Permalink
Revert of Reduce hit test on ShowPress by moving event targeting to W…
Browse files Browse the repository at this point in the history 
…ebViewImpl (patchset #8 of https://codereview.chromium.org/490783003/)

Reason for revert:
This is breaking content shell instrumentation tests (Chromium Android):
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/21185/steps/contentshell_instrumentation_tests/logs/stdio

You can reproduce that by building a Chromium Android build and run:
build/android/test_runner.py instrumentation --test-apk ContentShellTest --verbose --test_data content:content/test/data/android/device_files -f AddressDetectionTest#testAddressLimits
(assuming you have content_shell_apk and content_shell_test_apk build)

Original issue's description:
> Reduce hit test on ShowPress by moving hit testing to WebViewImpl.
> 
> Previously there was a hit test for link highlighting in WebViewImpl and then a                                          
> separate one in EventHandler::handleGestureEvent. This CL makes it so that a                                             
> single hit test in WebViewImpl is reused by the EventHandler. Scroll events                                              
> still need to hit test in a separate fashion though and their handling has been                                          
> refactored into a separate code path.
> 
> BUG=381728
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181009

[email protected],[email protected],[email protected],[email protected]
NOTREECHECKS=true
NOTRY=true
BUG=381728

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

git-svn-id: svn://svn.chromium.org/blink/trunk@181030 bbb929c8-8fbe-4397-9dbb-9b2b20218538
  • Loading branch information
@mounirlamouri
mounirlamouri committed Aug 28, 2014
1 parent 2cda1a2 commit 4e9841f
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 92 deletions.
6 changes: 3 additions & 3 deletions LayoutTests/fast/events/hit-test-counts-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ TouchStart: 1
TouchMove: 0
TouchEnd: 0
GestureTapDown: 2
GestureShowPress: 2
GestureShowPress: 4
GestureTap: 7
GestureScrollBegin: 1
GestureTapCancel: 1
Expand All @@ -35,7 +35,7 @@ TouchStart: 1 1 1
TouchMove: 0 0 0
TouchEnd: 0 0 0
GestureTapDown: 1 1 2
GestureShowPress: 1 1 2
GestureShowPress: 2 2 4
GestureTap: 3 3 7
GestureScrollBegin: 1 1 1
GestureTapCancel: 1 1 1
Expand All @@ -53,7 +53,7 @@ TouchStart: 1 1 0
TouchMove: 0 0 0
TouchEnd: 0 0 0
GestureTapDown: 1 4 16
GestureShowPress: 1 4 16
GestureShowPress: 2 8 32
GestureTap: 4 9 32
GestureScrollBegin: 1 1 0
GestureTapCancel: 1 1 0
Expand Down
27 changes: 8 additions & 19 deletions Source/core/page/EventHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
#include "core/page/DragController.h"
#include "core/page/DragState.h"
#include "core/page/EditorClient.h"
#include "core/page/EventWithHitTestResults.h"
#include "core/page/FocusController.h"
#include "core/page/FrameTree.h"
#include "core/page/Page.h"
Expand Down Expand Up @@ -2062,32 +2063,24 @@ bool EventHandler::handleGestureShowPress()

bool EventHandler::handleGestureEvent(const PlatformGestureEvent& gestureEvent)
{
TRACE_EVENT0("input", "EventHandler::handleGestureEvent");

// Propagation to inner frames is handled below this function.
ASSERT(m_frame == m_frame->localFrameRoot());

// Scrolling-related gesture events invoke EventHandler recursively for each frame down
// the chain, doing a single-frame hit-test per frame. This matches handleWheelEvent.
// FIXME: Add a test that traverses this path, e.g. for devtools overlay.
// Perhaps we could simplify things by rewriting scroll handling to work inner frame
// out, and then unify with other gesture events.
if (gestureEvent.isScrollEvent())
return handleGestureScrollEvent(gestureEvent);

// Non-scrolling related gesture events instead do a single cross-frame hit-test and
// jump directly to the inner most frame. This matches handleMousePressEvent etc.

// Hit test across all frames and do touch adjustment as necessary for the event type.
GestureEventWithHitTestResults targetedEvent = targetGestureEvent(gestureEvent);

return handleGestureEvent(targetedEvent);
}

bool EventHandler::handleGestureEvent(const GestureEventWithHitTestResults& targetedEvent)
{
TRACE_EVENT0("input", "EventHandler::handleGestureEvent");

// Propagation to inner frames is handled below this function.
ASSERT(m_frame == m_frame->localFrameRoot());

// Non-scrolling related gesture events do a single cross-frame hit-test and jump
// directly to the inner most frame. This matches handleMousePressEvent etc.
ASSERT(!targetedEvent.event().isScrollEvent());

// Route to the correct frame.
if (LocalFrame* innerFrame = targetedEvent.hitTestResult().innerNodeFrame())
return innerFrame->eventHandler().handleGestureEventInFrame(targetedEvent);
Expand Down Expand Up @@ -2142,8 +2135,6 @@ bool EventHandler::handleGestureEventInFrame(const GestureEventWithHitTestResult

bool EventHandler::handleGestureScrollEvent(const PlatformGestureEvent& gestureEvent)
{
TRACE_EVENT0("input", "EventHandler::handleGestureScrollEvent");

RefPtrWillBeRawPtr<Node> eventTarget = nullptr;
RefPtr<Scrollbar> scrollbar;
if (gestureEvent.type() != PlatformEvent::GestureScrollBegin) {
Expand Down Expand Up @@ -2565,8 +2556,6 @@ bool EventHandler::bestZoomableAreaForTouchPoint(const IntPoint& touchCenter, co

GestureEventWithHitTestResults EventHandler::targetGestureEvent(const PlatformGestureEvent& gestureEvent, bool readOnly)
{
TRACE_EVENT0("input", "EventHandler::targetGestureEvent");

ASSERT(m_frame == m_frame->localFrameRoot());
// Scrolling events get hit tested per frame (like wheel events do).
ASSERT(!gestureEvent.isScrollEvent());
Expand Down
5 changes: 3 additions & 2 deletions Source/core/page/EventHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "core/editing/TextGranularity.h"
#include "core/events/TextEventInputType.h"
#include "core/page/DragActions.h"
#include "core/page/EventWithHitTestResults.h"
#include "core/page/FocusType.h"
#include "core/rendering/HitTestRequest.h"
#include "core/rendering/style/RenderStyleConstants.h"
Expand Down Expand Up @@ -82,6 +81,9 @@ class VisibleSelection;
class WheelEvent;
class Widget;

typedef EventWithHitTestResults<PlatformGestureEvent> GestureEventWithHitTestResults;
typedef EventWithHitTestResults<PlatformMouseEvent> MouseEventWithHitTestResults;

enum AppendTrailingWhitespace { ShouldAppendTrailingWhitespace, DontAppendTrailingWhitespace };
enum CheckDragHysteresis { ShouldCheckDragHysteresis, DontCheckDragHysteresis };

Expand Down Expand Up @@ -146,7 +148,6 @@ class EventHandler : public NoBaseWillBeGarbageCollectedFinalized<EventHandler>

// Called on the local root frame exactly once per gesture event.
bool handleGestureEvent(const PlatformGestureEvent&);
bool handleGestureEvent(const GestureEventWithHitTestResults&);

// Hit-test the provided (non-scroll) gesture event, applying touch-adjustment and updating
// hover/active state across all frames if necessary. This should be called at most once
Expand Down
86 changes: 38 additions & 48 deletions Source/web/WebViewImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
#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"
Expand Down Expand Up @@ -664,49 +665,15 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)

PlatformGestureEventBuilder platformEvent(mainFrameImpl()->frameView(), event);

// Special handling for double tap and scroll events as we don't want to
// hit test for them.
switch (event.type) {
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.
eventSwallowed = true;
m_client->didHandleGestureEvent(event, eventCancelled);
return eventSwallowed;
case WebInputEvent::GestureScrollBegin:
case WebInputEvent::GesturePinchBegin:
m_client->cancelScheduledContentIntents();
case WebInputEvent::GestureScrollEnd:
case WebInputEvent::GestureScrollUpdate:
case WebInputEvent::GestureScrollUpdateWithoutPropagation:
case WebInputEvent::GesturePinchEnd:
case WebInputEvent::GesturePinchUpdate:
case WebInputEvent::GestureFlingStart:
// 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);
m_client->didHandleGestureEvent(event, eventCancelled);
return eventSwallowed;
default:
break;
}

// Hit test across all frames and do touch adjustment as necessary for the event type.
GestureEventWithHitTestResults targetedEvent =
m_page->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(platformEvent);
// FIXME: Remove redundant hit tests by pushing the call to EventHandler::targetGestureEvent
// up to this point and pass GestureEventWithHitTestResults around.

// 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(targetedEvent);
enableTapHighlightAtPoint(platformEvent);
break;
case WebInputEvent::GestureTapCancel:
case WebInputEvent::GestureTap:
Expand All @@ -721,8 +688,7 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
switch (event.type) {
case WebInputEvent::GestureTap: {
m_client->cancelScheduledContentIntents();
// FIXME: Use targeted event here and save another hit test.
if (detectContentOnTouch(targetedEvent.event().position())) {
if (detectContentOnTouch(platformEvent.position())) {
eventSwallowed = true;
break;
}
Expand Down Expand Up @@ -758,7 +724,7 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
}
}

eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(targetedEvent);
eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);

if (m_selectPopup && m_selectPopup == selectPopup) {
// That tap triggered a select popup which is the same as the one that
Expand All @@ -779,17 +745,38 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
m_client->cancelScheduledContentIntents();
m_page->contextMenuController().clearContextMenu();
m_contextMenuAllowed = true;
eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(targetedEvent);
eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
m_contextMenuAllowed = false;

break;
}
case WebInputEvent::GestureShowPress:
case WebInputEvent::GestureShowPress: {
m_client->cancelScheduledContentIntents();
eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
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.
eventSwallowed = true;
break;
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: {
eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(targetedEvent);
case WebInputEvent::GestureTapUnconfirmed:
case WebInputEvent::GesturePinchEnd:
case WebInputEvent::GesturePinchUpdate:
case WebInputEvent::GestureFlingStart: {
eventSwallowed = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(platformEvent);
break;
}
default:
Expand Down Expand Up @@ -1209,14 +1196,17 @@ static bool showsHandCursor(Node* node, LocalFrame* frame)
|| (cursor == CURSOR_AUTO && frame->eventHandler().useHandCursor(node, node->isLink()));
}

Node* WebViewImpl::bestTapNode(const GestureEventWithHitTestResults& targetedTapEvent)
Node* WebViewImpl::bestTapNode(const PlatformGestureEvent& tapEvent)
{
TRACE_EVENT0("input", "WebViewImpl::bestTapNode");

if (!m_page || !m_page->mainFrame())
return 0;

Node* bestTouchNode = targetedTapEvent.hitTestResult().targetNode();
// 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();

// 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
Expand All @@ -1243,9 +1233,9 @@ Node* WebViewImpl::bestTapNode(const GestureEventWithHitTestResults& targetedTap
return bestTouchNode;
}

void WebViewImpl::enableTapHighlightAtPoint(const GestureEventWithHitTestResults& targetedTapEvent)
void WebViewImpl::enableTapHighlightAtPoint(const PlatformGestureEvent& tapEvent)
{
Node* touchNode = bestTapNode(targetedTapEvent);
Node* touchNode = bestTapNode(tapEvent);

WillBeHeapVector<RawPtrWillBeMember<Node> > highlightNodes;
highlightNodes.append(touchNode);
Expand Down
5 changes: 2 additions & 3 deletions Source/web/WebViewImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#ifndef WebViewImpl_h
#define WebViewImpl_h

#include "core/page/EventWithHitTestResults.h"
#include "core/page/PagePopupDriver.h"
#include "platform/geometry/IntPoint.h"
#include "platform/geometry/IntRect.h"
Expand Down Expand Up @@ -432,8 +431,8 @@ class WebViewImpl FINAL : public WebView
void fullFramePluginZoomLevelChanged(double zoomLevel);

void computeScaleAndScrollForBlockRect(const WebPoint& hitPoint, const WebRect& blockRect, float padding, float defaultScaleWhenAlreadyLegible, float& scale, WebPoint& scroll);
Node* bestTapNode(const GestureEventWithHitTestResults& targetedTapEvent);
void enableTapHighlightAtPoint(const GestureEventWithHitTestResults& targetedTapEvent);
Node* bestTapNode(const PlatformGestureEvent& tapEvent);
void enableTapHighlightAtPoint(const PlatformGestureEvent& tapEvent);
void enableTapHighlights(WillBeHeapVector<RawPtrWillBeMember<Node> >&);
void computeScaleAndScrollForFocusedNode(Node* focusedNode, float& scale, IntPoint& scroll, bool& needAnimation);

Expand Down
48 changes: 31 additions & 17 deletions Source/web/tests/LinkHighlightTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
#include "bindings/core/v8/ExceptionStatePlaceholder.h"
#include "core/dom/Node.h"
#include "core/frame/FrameView.h"
#include "core/page/EventHandler.h"
#include "core/page/Page.h"
#include "core/page/TouchDisambiguation.h"
#include "core/testing/URLTestHelpers.h"
#include "platform/geometry/IntRect.h"
Expand All @@ -54,12 +52,6 @@ using namespace blink;

namespace {

GestureEventWithHitTestResults getTargetedEvent(WebViewImpl* webViewImpl, WebGestureEvent& touchEvent)
{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
return webViewImpl->page()->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(platformEvent, true);
}

TEST(LinkHighlightTest, verifyWebViewImplIntegration)
{
const std::string baseURL("http://www.test.com/");
Expand All @@ -80,31 +72,53 @@ TEST(LinkHighlightTest, verifyWebViewImplIntegration)
touchEvent.x = 20;
touchEvent.y = 20;

ASSERT_TRUE(webViewImpl->bestTapNode(getTargetedEvent(webViewImpl, touchEvent)));
{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
Node* touchNode = webViewImpl->bestTapNode(platformEvent);
ASSERT_TRUE(touchNode);
}

touchEvent.y = 40;
EXPECT_FALSE(webViewImpl->bestTapNode(getTargetedEvent(webViewImpl, touchEvent)));
{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
EXPECT_FALSE(webViewImpl->bestTapNode(platformEvent));
}

touchEvent.y = 20;
// Shouldn't crash.
webViewImpl->enableTapHighlightAtPoint(getTargetedEvent(webViewImpl, touchEvent));

{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
webViewImpl->enableTapHighlightAtPoint(platformEvent);
}

EXPECT_TRUE(webViewImpl->linkHighlight(0));
EXPECT_TRUE(webViewImpl->linkHighlight(0)->contentLayer());
EXPECT_TRUE(webViewImpl->linkHighlight(0)->clipLayer());

// Find a target inside a scrollable div

touchEvent.y = 100;
webViewImpl->enableTapHighlightAtPoint(getTargetedEvent(webViewImpl, touchEvent));
{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
webViewImpl->enableTapHighlightAtPoint(platformEvent);
}

ASSERT_TRUE(webViewImpl->linkHighlight(0));

// Don't highlight if no "hand cursor"
touchEvent.y = 220; // An A-link with cross-hair cursor.
webViewImpl->enableTapHighlightAtPoint(getTargetedEvent(webViewImpl, touchEvent));
{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
webViewImpl->enableTapHighlightAtPoint(platformEvent);
}
ASSERT_EQ(0U, webViewImpl->numLinkHighlights());

touchEvent.y = 260; // A text input box.
webViewImpl->enableTapHighlightAtPoint(getTargetedEvent(webViewImpl, touchEvent));
{
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
webViewImpl->enableTapHighlightAtPoint(platformEvent);
}
ASSERT_EQ(0U, webViewImpl->numLinkHighlights());

Platform::current()->unitTestSupport()->unregisterAllMockedURLs();
Expand Down Expand Up @@ -144,11 +158,11 @@ TEST(LinkHighlightTest, resetDuringNodeRemoval)
touchEvent.x = 20;
touchEvent.y = 20;

GestureEventWithHitTestResults targetedEvent = getTargetedEvent(webViewImpl, touchEvent);
Node* touchNode = webViewImpl->bestTapNode(targetedEvent);
PlatformGestureEventBuilder platformEvent(webViewImpl->mainFrameImpl()->frameView(), touchEvent);
Node* touchNode = webViewImpl->bestTapNode(platformEvent);
ASSERT_TRUE(touchNode);

webViewImpl->enableTapHighlightAtPoint(targetedEvent);
webViewImpl->enableTapHighlightAtPoint(platformEvent);
ASSERT_TRUE(webViewImpl->linkHighlight(0));

GraphicsLayer* highlightLayer = webViewImpl->linkHighlight(0)->currentGraphicsLayerForTesting();
Expand Down

0 comments on commit 4e9841f

Please sign in to comment.