Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
490783003:
Reduce hit test on ShowPress by moving event targeting to WebViewImpl (Closed)
|
Created:
6 years, 4 months ago by Zeeshan Qureshi Modified:
6 years, 3 months ago CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, blink-reviews, blink-reviews-events_chromium.org, caseq+blink_chromium.org, devtools-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, eustas+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, paulirish+reviews_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vsevik+blink_chromium.org, yurys+blink_chromium.org, bokan, wjmaclean, mustaq Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionReduce 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=182510
Patch Set 1 #Patch Set 2 : Rebase ToT #Patch Set 3 : Only hit test in WebViewImpl if not scroll event #Patch Set 4 : Update hit test counts #
Total comments: 8
Patch Set 5 : Add separate path for scroll events #Patch Set 6 : Rebase ToT #Patch Set 7 : Remove unnecessary headers #
Total comments: 10
Patch Set 8 : Rebase after other hit test patches #
Total comments: 4
Patch Set 9 : Rebase ToT #Patch Set 10 : Update hover/active after link highlights have been set #Patch Set 11 : Tweak test to only consider scrolling condition #Patch Set 12 : Switch back to using platform event position for content intents #
Messages
Total messages: 40 (5 generated)
Rick, still WIP but can you have a look.
This looks good overall, thanks! Just a couple additional opportunities to clean things up. https://codereview.chromium.org/490783003/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/490783003/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorOverlay.cpp:374: toLocalFrame(overlayPage()->mainFrame())->eventHandler().targetGestureEvent(event); Perhaps you should keep a trivial version of EventHandler::handleGestureEvent which just does these two steps. Then you can avoid changing all these callsites, but WebViewImpl can still separate the two steps. https://codereview.chromium.org/490783003/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/490783003/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:660: HitTestResult emptyResult; As discussed, having this "sometimes targetted but sometimes not" event is error-prone. You can split the code paths up further into methods that take a fully targetted event and those that don't. https://codereview.chromium.org/490783003/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:686: if (detectContentOnTouch(targetedEvent.event().position())) { Add FIXME to use the targeted event here. https://codereview.chromium.org/490783003/diff/60001/Source/web/tests/LinkHig... File Source/web/tests/LinkHighlightTest.cpp (right): https://codereview.chromium.org/490783003/diff/60001/Source/web/tests/LinkHig... Source/web/tests/LinkHighlightTest.cpp:82: webViewImpl->page()->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(platformEvent, true); Factor these couple lines into a little helper function to avoid having to repeat them.
https://codereview.chromium.org/490783003/diff/60001/Source/core/inspector/In... File Source/core/inspector/InspectorOverlay.cpp (right): https://codereview.chromium.org/490783003/diff/60001/Source/core/inspector/In... Source/core/inspector/InspectorOverlay.cpp:374: toLocalFrame(overlayPage()->mainFrame())->eventHandler().targetGestureEvent(event); On 2014/08/20 23:59:32, Rick Byers wrote: > Perhaps you should keep a trivial version of EventHandler::handleGestureEvent > which just does these two steps. Then you can avoid changing all these > callsites, but WebViewImpl can still separate the two steps. Done. https://codereview.chromium.org/490783003/diff/60001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/490783003/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:660: HitTestResult emptyResult; On 2014/08/20 23:59:33, Rick Byers wrote: > As discussed, having this "sometimes targetted but sometimes not" event is > error-prone. You can split the code paths up further into methods that take a > fully targetted event and those that don't. Done. https://codereview.chromium.org/490783003/diff/60001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:686: if (detectContentOnTouch(targetedEvent.event().position())) { On 2014/08/20 23:59:32, Rick Byers wrote: > Add FIXME to use the targeted event here. Done. https://codereview.chromium.org/490783003/diff/60001/Source/web/tests/LinkHig... File Source/web/tests/LinkHighlightTest.cpp (right): https://codereview.chromium.org/490783003/diff/60001/Source/web/tests/LinkHig... Source/web/tests/LinkHighlightTest.cpp:82: webViewImpl->page()->deprecatedLocalMainFrame()->eventHandler().targetGestureEvent(platformEvent, true); On 2014/08/20 23:59:33, Rick Byers wrote: > Factor these couple lines into a little helper function to avoid having to > repeat them. Done.
The CQ bit was checked by zeeshanq@chromium.org
The CQ bit was unchecked by zeeshanq@chromium.org
FYI: if you plan to reduce the number of hit tests even further, make sure that inspectorOverlay makes its own hit test, since it happens in another page (special overlay page).
I'm in the middle of trying to merge a bunch of fixes back to M38 around code like this (hopefully https://codereview.chromium.org/499433003/ is the last one). Is it OK if we hold off landing this for a little bit just to avoid merge headaches?
Sure.
This is looking really good! I have a couple more minor suggestions for cleaning it up a bit further, then we can ask aelias for a web/ OWNERS review. https://codereview.chromium.org/490783003/diff/120001/LayoutTests/fast/events... File LayoutTests/fast/events/hit-test-counts-expected.txt (right): https://codereview.chromium.org/490783003/diff/120001/LayoutTests/fast/events... LayoutTests/fast/events/hit-test-counts-expected.txt:20: GestureShowPress: 1 woot! Thank you! Sorry I need to add one back (https://codereview.chromium.org/499433003/), but we'll get it down again... https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2065: TRACE_EVENT0("input", "EventHandler::handleGestureEvent"); How about we remove this trace (since it's normally redundant with the one in handleGestureScrollEvent/handleGestureEvent), and add one to targetGestureEvent? This is just a little wrapper helper that's not always used, so probably doesn't need it's own trace. But I expect more and more places to call targetGestureEvent and it has some non-trivial work (like applying touch adjustment) so we really should trace it directly... https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2073: handleGestureScrollEvent(gestureEvent); you still need the return here, right? https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (left): https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:637: // Special handling for slow-path fling gestures. 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... https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:641: // Special handling for double tap as we don't want to hit test for it. 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. https://codereview.chromium.org/490783003/diff/120001/Source/web/tests/LinkHi... File Source/web/tests/LinkHighlightTest.cpp (right): https://codereview.chromium.org/490783003/diff/120001/Source/web/tests/LinkHi... Source/web/tests/LinkHighlightTest.cpp:59: GestureEventWithHitTestResults getTargetedEvent(WebViewImpl* webViewImpl, WebGestureEvent& touchEvent) nit: s/touch/gesture/
https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2065: TRACE_EVENT0("input", "EventHandler::handleGestureEvent"); On 2014/08/22 14:49:57, Rick Byers wrote: > How about we remove this trace (since it's normally redundant with the one in > handleGestureScrollEvent/handleGestureEvent), and add one to targetGestureEvent? > This is just a little wrapper helper that's not always used, so probably > doesn't need it's own trace. But I expect more and more places to call > targetGestureEvent and it has some non-trivial work (like applying touch > adjustment) so we really should trace it directly... Agreed, I only added here because I was thinking about the targeting cost, but tracing that directly is better. https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2073: handleGestureScrollEvent(gestureEvent); On 2014/08/22 14:49:57, Rick Byers wrote: > you still need the return here, right? Oh yes, and all the tests passed! https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (left): https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:637: // Special handling for slow-path fling gestures. Having a single source of truth is definitely better. https://codereview.chromium.org/490783003/diff/120001/Source/web/tests/LinkHi... File Source/web/tests/LinkHighlightTest.cpp (right): https://codereview.chromium.org/490783003/diff/120001/Source/web/tests/LinkHi... Source/web/tests/LinkHighlightTest.cpp:59: GestureEventWithHitTestResults getTargetedEvent(WebViewImpl* webViewImpl, WebGestureEvent& touchEvent) On 2014/08/22 14:49:57, Rick Byers wrote: > nit: s/touch/gesture/ Only used this because the whole test was using touchEvent even when they're specifically creating a gesture event.
On 2014/08/22 15:30:50, Zeeshan Qureshi wrote: > https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... > Source/core/page/EventHandler.cpp:2065: TRACE_EVENT0("input", > "EventHandler::handleGestureEvent"); > On 2014/08/22 14:49:57, Rick Byers wrote: > > How about we remove this trace (since it's normally redundant with the one in > > handleGestureScrollEvent/handleGestureEvent), and add one to > targetGestureEvent? > > This is just a little wrapper helper that's not always used, so probably > > doesn't need it's own trace. But I expect more and more places to call > > targetGestureEvent and it has some non-trivial work (like applying touch > > adjustment) so we really should trace it directly... > > Agreed, I only added here because I was thinking about the targeting cost, but > tracing that directly is better. > > https://codereview.chromium.org/490783003/diff/120001/Source/core/page/EventH... > Source/core/page/EventHandler.cpp:2073: handleGestureScrollEvent(gestureEvent); > On 2014/08/22 14:49:57, Rick Byers wrote: > > you still need the return here, right? > > Oh yes, and all the tests passed! Yeah, scary! I guess most things don't use this handleGestureEvent convenience wrapper anymore (eg. the hit-test-count test doesn't). So the things that do (Devtools overlay etc.) must not have tests that simulate scroll gestures, otherwise we would have hit the ASSERT(!targetedEvent.event().isScrollEvent()) in EventHandler::handleGestureEventInFrame. If you see an easy way to add a test that would hit this that would be great, but not necessary since it's a pre-existing test hole. > https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (left): > > https://codereview.chromium.org/490783003/diff/120001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:637: // Special handling for slow-path fling > gestures. > > Having a single source of truth is definitely better. > > https://codereview.chromium.org/490783003/diff/120001/Source/web/tests/LinkHi... > File Source/web/tests/LinkHighlightTest.cpp (right): > > https://codereview.chromium.org/490783003/diff/120001/Source/web/tests/LinkHi... > Source/web/tests/LinkHighlightTest.cpp:59: GestureEventWithHitTestResults > getTargetedEvent(WebViewImpl* webViewImpl, WebGestureEvent& touchEvent) > On 2014/08/22 14:49:57, Rick Byers wrote: > > nit: s/touch/gesture/ > > Only used this because the whole test was using touchEvent even when they're > specifically creating a gesture event. Yeah - either way, I don't really care.
Rick, updated after your changes landed. Added a FIXME for the scroll events missing test for now.
rbyers@chromium.org changed reviewers: + aelias@chromium.org
LGTM (with minor nit). +aelias for Source/web OWNERS https://codereview.chromium.org/490783003/diff/140001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/490783003/diff/140001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2070: // FIXME: Add a test that traverses this path, e.g. for devtools overlay. Personally I wouldn't use a code comment for this purpose - we have lots of untested code paths, I don't think adding FIXME's for them will help make them tested or otherwise improve understandability of the code. If you want to track adding such a test feel free to file a bug.
https://codereview.chromium.org/490783003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/490783003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:670: case WebInputEvent::GestureDoubleTap: It seems strange to treat double-tap specially here and I don't follow what makes it fundamentally different from the other tap-related events. (Double-tap also effectively hit-tests and then figures out a containing node.) Could you explain or stop special-casing it?
https://codereview.chromium.org/490783003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/490783003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:670: case WebInputEvent::GestureDoubleTap: On 2014/08/27 03:26:34, aelias wrote: > It seems strange to treat double-tap specially here and I don't follow what > makes it fundamentally different from the other tap-related events. (Double-tap > also effectively hit-tests and then figures out a containing node.) Could you > explain or stop special-casing it? The way I see it is that DoubleTap is handled directly in WebViewImpl whereas most of the others go down to blink::core and get processed there. Looking at animateDoubleTapZoom, it might be doing a hit test but it's not quite like what we do in the event targeting for the others. It was already special cased in the previous switch, we're just moving it up a bit to spare the unnecessary hit test we would have done. Maybe we should start tracking how many hit tests are triggered for a double tap and look into unifying it if that's an issue.
https://codereview.chromium.org/490783003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/490783003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:670: case WebInputEvent::GestureDoubleTap: On 2014/08/27 19:37:56, Zeeshan Qureshi wrote: > On 2014/08/27 03:26:34, aelias wrote: > > It seems strange to treat double-tap specially here and I don't follow what > > makes it fundamentally different from the other tap-related events. > (Double-tap > > also effectively hit-tests and then figures out a containing node.) Could you > > explain or stop special-casing it? > > The way I see it is that DoubleTap is handled directly in WebViewImpl whereas > most of the others go down to blink::core and get processed there. Looking at > animateDoubleTapZoom, it might be doing a hit test but it's not quite like what > we do in the event targeting for the others. It was already special cased in the > previous switch, we're just moving it up a bit to spare the unnecessary hit test > we would have done. > > Maybe we should start tracking how many hit tests are triggered for a double tap > and look into unifying it if that's an issue. I agree that this CL doesn't change the special-handling for double tap at all - just moves it up above the blink core hit test we're doing. But I agree that this comment is misleading - we do want to hit test for double tap (looks like it happens in WebViewImpl::computeBlockBounds). Perhaps we should just add a FIXME saying to unify double-tap hit testing with targetGestureEvent? That would have the benefit of increasing the chance of getting a hit on the (to be written) hit-test cache, and probably wouldn't be hard to implement (eg. change animateDoubleTapZoom to take a GestureEventWithHitTestResults). Bug again I don't think that needs to happen in this CL (although it's simple enough that I'm not opposed to lumping it in here too).
OK, it sounds like further improvement is possible in double-tap, but lgtm for this incremental improvement anyway. For what it's worth, I care more about code cleanliness/unification than perf for double-tap. I don't think too many hit tests is likely a significant issue for double-tap (going to the main thread at all for them is the problem, so I think the Blink scheduler is where we'll get the real wins). That said, code cleanliness and perf are hopefully well-aligned goals in this case.
The CQ bit was checked by zeeshanq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/490783003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 181009
Message was sent while issue was closed.
A revert of this CL (patchset #8) has been created in https://codereview.chromium.org/509173004/ by mlamouri@chromium.org. The reason for reverting is: This is breaking content shell instrumentation tests (Chromium Android): http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg... 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).
Message was sent while issue was closed.
I'm trying to repro these on my device and they pass. Maybe it was a one-off thing. Will try re-landing.
The CQ bit was checked by zeeshanq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zeeshanq@chromium.org/490783003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 181427
Rick/Alexander PTAL This patch set fixes the issue with compositing/gestures/gesture-tapHighlight-on-promoted-o verflow-div-scrolled.html What seemed to be happening was that updateHoverActiveState() was being called before the link highlights had been updated, therefore it wasn't marking the appropriate sections of the tree as dirty for layout. This in turn caused the right graphics layer to not be invalidated and the link highlight got stuck at the original position.
Patchset #10 (id:180001) has been deleted
Patchset #10 (id:200001) has been deleted
On 2014/09/16 05:56:32, Zeeshan Qureshi wrote: > Rick/Alexander PTAL > > This patch set fixes the issue with > compositing/gestures/gesture-tapHighlight-on-promoted-o > verflow-div-scrolled.html > > What seemed to be happening was that updateHoverActiveState() was being called > before the link highlights had been updated, therefore it wasn't marking the > appropriate sections of the tree as dirty for layout. This in turn caused the > right graphics layer to not be invalidated and the link highlight got stuck at > the original position. Does this change affect the behavior of cases like this where the :active state causes the size of the highlight to change: http://jsbin.com/naguc/2/edit? Today that seems to work reasonably, with the highlight covering the new (active) size. I'm concerned this may be a symptom of a deeper issue with link highlighting. Eg. if it doesn't behave correctly when invoked with dirty layout, then there could be races that get us into the same state. I don't see any good reason why we shouldn't be able to do updateHoverActiveState just before computing link highlights. In fact I think that's exactly what we want since it's not crazy that the active state would have a different highlight area than the non-active state. Otherwise, at best, we're doing work to commpute a highlight we're just going to throw away and recompute after the active state has been applied. Can you try to provoke a similar issue with a LayoutTest that dirties the dom (i.e. applies a style which behaves just like your :active case) immediately before sending a GestureShowPress? I see WebViewImpl::enableTapHighlight invokes WebViewImpl::layout(). I would have thought that sufficient to deal with such cases. +bokan,wjmaclean,mustaq for their expertise on link highlighting. We may find we can split this into a separate highlighting bug which this bug becomes blocked on.
I agree, this is only a temporary fix. We should be updating link highlighting to work independently of when hover/active states are updated. I'll investigate a bit more and try to point out the exact problem.
I've filed a bug for the link highlighting issue crbug.com/415702 and updated the test to better target the scrolling scenario.
The CQ bit was checked by zeeshanq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/490783003/240001
On 2014/09/18 21:15:23, Zeeshan Qureshi wrote: > I've filed a bug for the link highlighting issue crbug.com/415702 and updated > the test to better target the scrolling scenario. Thanks, I agree this is the best way forward here. Still LGTM.
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as 182281
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:240001) has been created in https://codereview.chromium.org/587543004/ by zeeshanq@chromium.org. The reason for reverting is: Failing ContentShellTest on blink roll. http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes....
The CQ bit was checked by zeeshanq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/490783003/260001
Message was sent while issue was closed.
Committed patchset #12 (id:260001) as 182510 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
