Bug 75506 - Improve touch handling performance by reusing the hitTest result
Summary: Improve touch handling performance by reusing the hitTest result
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-03 17:10 PST by Min Qin
Modified: 2012-03-22 13:30 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.20 KB, patch)
2012-01-03 17:18 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (4.28 KB, patch)
2012-01-18 13:32 PST, Min Qin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Min Qin 2012-01-03 17:10:16 PST
Currently we are doing a hit test for every touch events, this is unnecessary.
For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.
Even worse, performing a hitTest on a TouchMove could give us a new containing frame as finger moves. This could totally mess up the touch handling.
One good example for this is scrolling http://www.nme.com/reviews/young-knives/11931 on android browser with galaxy nexus. Touch the first paragraph and start scrolling upwards. When finger reaches the video area, flickering will appear.

This change reuses the hit test results from TouchPressed for further touch events.
Comment 1 Min Qin 2012-01-03 17:18:05 PST
Created attachment 121025 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-01-04 13:27:45 PST
> For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.

Is it really the case? Page content may move, so another element will be at the touch position. The original element may even be removed already.
Comment 3 Min Qin 2012-01-04 13:52:03 PST
Yes, this is the case. TouchTarget should not change before finger is released. In the current EventHandler.cc, for example, the touchTarget  for TouchRelease and TouchMove is always the node given by TouchPressed.
Yes, the touchTarget could be removed during touch moves. But since it is ref counted, so as long as eventHandler maintains a pointer to it, it will not be GCed. And subsequent events send to it should do nothing. 

(In reply to comment #2)
> > For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.
> 
> Is it really the case? Page content may move, so another element will be at the touch position. The original element may even be removed already.
Comment 4 Min Qin 2012-01-04 14:14:48 PST
From the spec: https://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html
5.6 The touchmove event
The target of this event must be the same Element on which the touch point started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of the target element.

(In reply to comment #3)
> Yes, this is the case. TouchTarget should not change before finger is released. In the current EventHandler.cc, for example, the touchTarget  for TouchRelease and TouchMove is always the node given by TouchPressed.
> Yes, the touchTarget could be removed during touch moves. But since it is ref counted, so as long as eventHandler maintains a pointer to it, it will not be GCed. And subsequent events send to it should do nothing. 
> 
> (In reply to comment #2)
> > > For TouchRelease and TouchMove events, the touchTarget is always the same as the TouchPressed event.
> > 
> > Is it really the case? Page content may move, so another element will be at the touch position. The original element may even be removed already.
Comment 5 Adam Barth 2012-01-18 12:54:45 PST
Comment on attachment 121025 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121025&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

We won't be able to land this patch with this line in the ChangeLog.  In cases like this, we usually replace this line with a sentence saying that that patch is a performance optimization only and shouldn't have any behavior changes.
Comment 6 Min Qin 2012-01-18 13:32:17 PST
Created attachment 122981 [details]
Patch
Comment 7 Min Qin 2012-01-19 10:45:18 PST
ChangeLog fixed
Comment 8 Adam Barth 2012-01-19 13:48:22 PST
Comment on attachment 122981 [details]
Patch

Thanks.
Comment 9 WebKit Review Bot 2012-01-19 15:07:26 PST
Comment on attachment 122981 [details]
Patch

Clearing flags on attachment: 122981

Committed r105453: <http://trac.webkit.org/changeset/105453>
Comment 10 WebKit Review Bot 2012-01-19 15:07:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Allan Sandfeld Jensen 2012-01-23 06:41:45 PST
We already have code that avoids moving the frame on hittests that are read-only. Was this really necessary in trunk to fix this bug?

While this patch is still an optimization. It makes it impossible to improve the :active handling to for instance stop showing :active if the finger moves off the target.

:active is used to show the user what is activated, but once the finger ends off the target this link will no longer be activated, and thus the :active feedback to user will be wrong.
Comment 12 Min Qin 2012-01-23 07:43:44 PST
Why is it hard to show :active? Since the touchTarget won't change, the last hittest results could be reused as finger moves off the target. So your :active link won't change during a touch down->touch move->touch release

On some pages, doing a hittest on each touchMove can be really expensive. the espn blog page is a good example. It invokes a layout each time when a move happens. each layout could take more than 500 msec to do.

And this change also avoids the problem that finger could move to another element after touchDown. One good example for this is scrolling http://www.nme.com/reviews/young-knives/11931 on android browser. Touch the first paragraph and start scrolling upwards. When finger reaches the video area, flickering will appear as a new hittest will return a different pagePoint as the condition of if statement becomes true.

if (m_frame != doc->frame()) {		
            // pagePoint should always be relative to the target elements contai
ning frame.		
            pagePoint = documentPointForWindowPoint(doc->frame(), point.pos());		
}



(In reply to comment #11)
> We already have code that avoids moving the frame on hittests that are read-only. Was this really necessary in trunk to fix this bug?
> 
> While this patch is still an optimization. It makes it impossible to improve the :active handling to for instance stop showing :active if the finger moves off the target.
> 
> :active is used to show the user what is activated, but once the finger ends off the target this link will no longer be activated, and thus the :active feedback to user will be wrong.
Comment 13 Min Qin 2012-01-23 07:58:51 PST
And one more thing on finger moves off the target:

From the spec: https://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html:
5.6 The touchmove event
The target of this event must be the same Element on which the touch point started when it was first placed on the surface, even if the touch point has since moved outside the interactive area of the target element.

Even if the finger is off the target, the :active should still remain on the touch down target. Until touch is released. So user should still see the original touch target is activated.

(In reply to comment #11)
> We already have code that avoids moving the frame on hittests that are read-only. Was this really necessary in trunk to fix this bug?
> 
> While this patch is still an optimization. It makes it impossible to improve the :active handling to for instance stop showing :active if the finger moves off the target.
> 
> :active is used to show the user what is activated, but once the finger ends off the target this link will no longer be activated, and thus the :active feedback to user will be wrong.
Comment 14 Allan Sandfeld Jensen 2012-01-23 08:03:11 PST
(In reply to comment #12)
> Why is it hard to show :active? Since the touchTarget won't change, the last hittest results could be reused as finger moves off the target. So your :active link won't change during a touch down->touch move->touch release
> 
The point was to cancel :active and :hover if the activated object no longer is going to be activated by the current gesture. You make a good case for optimizing touch move, but are activated and hovered elements updated on TouchReleased or TouchCancelled?

I believe maintaining the active- and hovered elements is usually done by the hitTest. If hitTest is not performed on touch-release the active or hovered elements will stick to the last hitTested result. Actually hover already stick, but I was going to fix that, but will need a hit-test on touch-release/cancel to do so.
Comment 15 Yong Li 2012-03-22 10:37:48 PDT
 	        Document* doc = node->document(); 
3264	 	        if (!doc) 
3265	 	            continue; 

This check is moved to if (pointState == PlatformTouchPoint::TouchPressed) block. And we no longer check it for the other two cases.

I know the node is locked by a RefPtr, however that doesn't prevent it from being evicted from the document. Will this cause potential crash?
Comment 16 Yong Li 2012-03-22 13:30:22 PDT
Bug 81958 is created for the regression.