Correctly handle touch events that contain touches not previously reported to blink

Handle touch events containing touches not previously reported to blink

Chromium has a couple optimizations which suppress sending touch events
to the renderer (for example, when they're at a place or time known not
to contain touch event handlers).  Our security fix in
http://crbug.com/148567 assumed we'd always see every touchstart event,
so if we got an event which had a point we'd never seen we'd
effectively ignore the event completely.

Touch events do their hit-test on touchstart, and every TouchEvent
contains a touches array for all active touches.  So if we're going
to skip sending some touchstart events, we need to define how those
active touch points show up in the events for other points.  This
problem is similar to how touches should show up for fingers that
are down on iframes outside the bounds of the document receiving
touch events.  I think it's important that we include such touches
(otherwise an app could mis-recognize a two-finger gesture as a
single finger one), but their 'target' should be irrelevant since
we know it wasn't capable of receiving the touch events.  So here
I just set the target to the document.  This also updates the
cross-iframe case, we were previously hiding such points completely.

I've split the main loop across points into two stages (each with
their own loop).  First we do any necessary hit tests, including
establishing the active touch sequence document if it wasn't
previously determined.  Then we create all the Touch objects,
where we may rely on the active touch sequence document determined
by some later point.

This CL also contains a number of overdue cleanups to account for the
fact that we now only need to hit-test on touchstart, and that there's
only ever a single document receiving touch events at any time.
Also rather than use a funky ID+1 key for our touch target HashMap,
just use the ID but ensure HashMap supports 0-value keys by using
UnsignedWithZeroKeyHashTraits.

BUG=363321
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173245

[Rick Byers, 2014-04-29 21:14:16]

Hey Zeeshan,
Can you please do an initial code review for me before I ask Adam to take a
look?

Thanks,
   Rick

[Zeeshan Qureshi, 2014-04-30 18:09:05]

LGTM

An added thought, after some discussion with Tim it seems that the target frame
for a gesture event should be the same as the frame for the touch events it is
synthesized from. Since we already do the calculation here, we should be able to
cache and re-use it in handleGestureEvent() and other places too.

[Rick Byers, 2014-05-01 02:59:02]

On 2014/04/30 18:09:05, Zeeshan Qureshi wrote:
> LGTM

Thanks.  Adam, please take a look.

> An added thought, after some discussion with Tim it seems that the target
frame
> for a gesture event should be the same as the frame for the touch events it is
> synthesized from. Since we already do the calculation here, we should be able
to
> cache and re-use it in handleGestureEvent() and other places too.

Perhaps.  But remember that we often won't send the touch events to blink at all
(when we know there is no handler), so we'd have to be prepared to do the hit
test on the gesture event if necessary.  I'd be a little worried about potential
observable behavior differences here (adding an empty touch handler shouldn't,
ideally, cause any behavior change expect performance).  Perhaps this is part of
the larger issue around repeating hit-tests for gesture events when we should be
re-using the result of a previous hit-test (http://crbug.com/226842 assigned to
you)?

[Zeeshan Qureshi, 2014-05-01 03:07:20]

> Perhaps.  But remember that we often won't send the touch events to blink at
all
> (when we know there is no handler), so we'd have to be prepared to do the hit
> test on the gesture event if necessary.  I'd be a little worried about
potential
> observable behavior differences here (adding an empty touch handler shouldn't,
> ideally, cause any behavior change expect performance).  Perhaps this is part
of
> the larger issue around repeating hit-tests for gesture events when we should
be
> re-using the result of a previous hit-test (http://crbug.com/226842 assigned
to
> you)?

Oh yes, I should have clarified, I was mentioning it in the context of that bug.
Yes,
I didn't realize that the GR would short circuit and only send gesture events to
blink
in the case of no touch handlers. I'll keep in mind to consider the touch events
case
too when resuming work on that CL.

[abarth-chromium, 2014-05-01 06:36:10]

I'm a bit of a noob here.  I'm happy to rubber-stamp lgtm this CL given that
you've already gotten approval and you're the expert here.

[Rick Byers, 2014-05-01 12:59:28]

Thanks.  +rjkroege just for another set of eyes from someone who has worked in
this code before.

[rjkroege, 2014-05-02 20:33:49]

lgtm

is it possible that handleTouchEvent should be refactored? It seems
long/convoluted which suggests a refactor someday?

https://codereview.chromium.org/259413003/diff/1/Source/core/events/EventPath...
File Source/core/events/EventPath.cpp (right):

https://codereview.chromium.org/259413003/diff/1/Source/core/events/EventPath...
Source/core/events/EventPath.cpp:343: 
do the layout tests still pass if you comment them out? :-)

and if you're going to add this, maybe a tracker bug?

https://codereview.chromium.org/259413003/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/259413003/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:3474: // Ideally we'd ASSERT
!m_touchSequenceDocument here since we should have cleared the
nit: you could line-break the comment at 80?

[Rick Byers, 2014-05-02 21:24:15]

Thanks.

> is it possible that handleTouchEvent should be refactored? It seems
> long/convoluted which suggests a refactor someday?

Yeah I think it could be split apart a bit.  But there's definitely a lot of
state that needs to be shared across the phases.

https://codereview.chromium.org/259413003/diff/1/Source/core/events/EventPath...
File Source/core/events/EventPath.cpp (right):

https://codereview.chromium.org/259413003/diff/1/Source/core/events/EventPath...
Source/core/events/EventPath.cpp:343: 
On 2014/05/02 20:33:50, rjkroege wrote:
> do the layout tests still pass if you comment them out? :-)
> 
> and if you're going to add this, maybe a tracker bug?

Ah, it looks like I misunderstood the oilpannery here.  We're keeping pointers
to the TouchLists from the TouchEventContext and then mutating those lists. 
Thanks for challenging me on this.

https://codereview.chromium.org/259413003/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/259413003/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:3474: // Ideally we'd ASSERT
!m_touchSequenceDocument here since we should have cleared the
On 2014/05/02 20:33:50, rjkroege wrote:
> nit: you could line-break the comment at 80?

Done.  Also updated comments below.

[Rick Byers, 2014-05-02 21:24:25]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:24:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/259413003/20001

[commit-bot: I haz the power, 2014-05-02 23:01:49]

Change committed as 173245