touch: Fix touch-event hit-testing for multiple touch points.

touch: Fix touch-event hit-testing for multiple touch points.

If the first touch press happens on a touch-region, but then moves out of that
region, and the second press happens again on the touch-region, then the hit-test
for the second finger would fail because the code used to only look at the position
of the first touch point. Fix that to make sure the position of the new touch
point is used for hit-testing.

BUG=321457
R=jamesr@chromium.org, rbyers@chromium.org
TBR=jochen@chromium.org for content_tests.gypi

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238289

[piman, 2013-11-19 20:37:27]

->jamesr who understands this code better than I do.

[jamesr, 2013-11-19 20:41:31]

I believe that the first-touch-is-the-only-one-that-matters choice was
deliberate, but my memory is vague. Alex - do you know what's expected here?

[jamesr, 2013-11-19 20:42:05]

Why is there no bug?  Have you tested this behavior on different platforms?

[Rick Byers, 2013-11-19 22:38:07]

The intention for our design was that compositor touch hit testing should not
change behavior other than performance.  It looks like the previous behavior
violated that (eg. a touchstart handler could be missed as a result of hit
testing), and this CL will fix that.  So this seems like the right thing for me.
 +yusuf who wrote this initially.

It should be possible to have a simple demo for this that works (for example) on
mobile safari but fails on Chrome Android.  But yes please file a bug to track
and we can discuss demos and compare platforms there.

[sadrul, 2013-11-20 05:51:23]

Filed a bug with a small demo that shows the bug.

[aelias_OOO_until_Jul13, 2013-11-20 07:50:04]

I don't know offhand what's expected -- we should aim to match mobile Safari.

[Rick Byers, 2013-11-20 14:53:16]

Thanks for the test case.

So on Android a new TouchPressed is always at index 0 in the event (and we tend
not to get multiple TouchPressed in the same event), and so the bug never
manifested itself there, right?  I wonder if there are similar assumptions
around the significance of touch point index elsewhere...

LGTM

https://codereview.chromium.org/76283003/diff/80001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/76283003/diff/80001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_proxy_unittest.cc:1143:
touch.touches[2].position = WebPoint(-10, 10);
I was initially confused by this point.  I assume the intention is to verify
that HaveTouchEventHandlersAt isn't called for it (since a previous call already
returned true), right?  Probably worth commenting.

[Rick Byers, 2013-11-20 14:55:21]

On 2013/11/20 07:50:04, aelias - OOO until Dec 9 wrote:
> I don't know offhand what's expected -- we should aim to match mobile Safari.

Yes, this change fixes ChromeOS/Windows to match mobile safari behavior.  And it
looks like Chrome Android happened to avoid the bug too, so this also makes Aura
consistent with Android (and may fix edge cases in Android - eg. if it's
possible to get a TouchPressed for two fingers in the same event).

[sadrul, 2013-11-20 20:11:37]

https://codereview.chromium.org/76283003/diff/80001/content/renderer/gpu/inpu...
File content/renderer/gpu/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/76283003/diff/80001/content/renderer/gpu/inpu...
content/renderer/gpu/input_handler_proxy_unittest.cc:1143:
touch.touches[2].position = WebPoint(-10, 10);
On 2013/11/20 14:53:16, Rick Byers wrote:
> I was initially confused by this point.  I assume the intention is to verify
> that HaveTouchEventHandlersAt isn't called for it (since a previous call
already
> returned true), right?  Probably worth commenting.

Yep. I added a comment above in line 1126-1127.

[jamesr, 2013-11-20 20:49:49]

Do we understand where the difference in behavior between Android and ChromeOS
is?  This isn't the only place in the code that process touch events so patching
only this function seems unlikely to the right fix.

[sadrul, 2013-11-20 20:57:20]

On 2013/11/20 20:49:49, jamesr wrote:
> Do we understand where the difference in behavior between Android and ChromeOS
> is?  This isn't the only place in the code that process touch events so
patching
> only this function seems unlikely to the right fix.

The test does fail on android if multiple fingers are pressed at the same time.
The issue here is that we need to test the location of each pressed finger,
instead of testing only the first finger (which may or may not be the
touch-point that was pressed).

[sadrul, 2013-11-20 21:10:51]

On 2013/11/20 20:49:49, jamesr wrote:
> Do we understand where the difference in behavior between Android and ChromeOS
> is?  This isn't the only place in the code that process touch events so
patching
> only this function seems unlikely to the right fix.

I found another instance in RenderWidget. I will look for other such cases, and
ping back with the final patch. Thanks.

[sadrul, 2013-11-20 23:20:28]

On 2013/11/20 21:10:51, sadrul wrote:
> On 2013/11/20 20:49:49, jamesr wrote:
> > Do we understand where the difference in behavior between Android and
ChromeOS
> > is?  This isn't the only place in the code that process touch events so
> patching
> > only this function seems unlikely to the right fix.
> 
> I found another instance in RenderWidget. I will look for other such cases,
and
> ping back with the final patch. Thanks.

Re comment in https://code.google.com/p/chromium/issues/detail?id=321457#c6, I
believe this is ready for review. PTAL.

[Rick Byers, 2013-11-20 23:57:35]

On 2013/11/20 23:20:28, sadrul wrote:
> On 2013/11/20 21:10:51, sadrul wrote:
> > On 2013/11/20 20:49:49, jamesr wrote:
> > > Do we understand where the difference in behavior between Android and
> ChromeOS
> > > is?  This isn't the only place in the code that process touch events so
> > patching
> > > only this function seems unlikely to the right fix.
> > 
> > I found another instance in RenderWidget. I will look for other such cases,
> and
> > ping back with the final patch. Thanks.
> 
> Re comment in https://code.google.com/p/chromium/issues/detail?id=321457#c6, I
> believe this is ready for review. PTAL.

Thanks Sadrul.  LGTM.

[jamesr, 2013-11-21 00:21:05]

Where's the test for the render_widget.cc change?  Ideally we'd have one test
that can run against both codepaths and verifies that they do the same thing as
each other.

[sadrul, 2013-12-02 07:18:20]

On 2013/11/21 00:21:05, jamesr wrote:
> Where's the test for the render_widget.cc change?  Ideally we'd have one test
> that can run against both codepaths and verifies that they do the same thing
as
> each other.

Added a test for render_widget.cc changes (I couldn't find a good way to add a
common set of tests that would work for both RenderWidget and
InputHandlerProxy). PTAL

[jamesr, 2013-12-03 00:44:37]

lgtm. unit tests are useful, but I was thinking that an integration level test
that spawned a web page with touch handlers in various places and then fires
events at it would be a good way to ensure the high-level behavior is
consistent.  it's hard to verify that the two unit tests are in sync with each
other as the code is refactored/etc. This is fine test coverage to land with,
though.

https://codereview.chromium.org/76283003/diff/460006/content/renderer/gpu/inp...
File content/renderer/gpu/input_handler_proxy.cc (right):

https://codereview.chromium.org/76283003/diff/460006/content/renderer/gpu/inp...
content/renderer/gpu/input_handler_proxy.cc:231: for (unsigned i = 0; i <
touch_event.touchesLength; ++i) {
can you use size_t for indexing? unsigned works given how touches[] works, but
it's slightly less idiomatic for a loop counter indexing into an array

https://codereview.chromium.org/76283003/diff/460006/content/renderer/render_...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/76283003/diff/460006/content/renderer/render_...
content/renderer/render_widget_unittest.cc:26: render_process_.reset(new
MockRenderProcess());
nit: if these will be unconditionally default allocated, why put them in
scoped_ptr<>s? why not just have MockRenderProcess / MockRenderThread members
directly?

[sadrul, 2013-12-03 03:20:00]

Thanks. I will look into adding an intergration test for this case.

+jochen@ for content_tests.gypi

https://codereview.chromium.org/76283003/diff/460006/content/renderer/gpu/inp...
File content/renderer/gpu/input_handler_proxy.cc (right):

https://codereview.chromium.org/76283003/diff/460006/content/renderer/gpu/inp...
content/renderer/gpu/input_handler_proxy.cc:231: for (unsigned i = 0; i <
touch_event.touchesLength; ++i) {
On 2013/12/03 00:44:38, jamesr wrote:
> can you use size_t for indexing? unsigned works given how touches[] works, but
> it's slightly less idiomatic for a loop counter indexing into an array

Done.

https://codereview.chromium.org/76283003/diff/460006/content/renderer/render_...
File content/renderer/render_widget_unittest.cc (right):

https://codereview.chromium.org/76283003/diff/460006/content/renderer/render_...
content/renderer/render_widget_unittest.cc:26: render_process_.reset(new
MockRenderProcess());
On 2013/12/03 00:44:38, jamesr wrote:
> nit: if these will be unconditionally default allocated, why put them in
> scoped_ptr<>s? why not just have MockRenderProcess / MockRenderThread members
> directly?

Good point. Done.

[sadrul, 2013-12-03 03:27:30]

Committed patchset #9 manually as r238289 (presubmit successful).