Added PointerEvent firing on touch events.

Added PointerEvent firing on touch events.

This CL adds PointerEvent firing logic on touches. TouchEvent
firing is made conditional on all PointerEvents being unconsumed.
The PointerEvent with the first pointerdown is marked as primary.

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

[mustaq, 2015-05-25 16:53:11]

Patchset #1 (id:1) has been deleted

[mustaq, 2015-05-25 16:58:59]

mustaq@chromium.org changed reviewers:
+ rbyers@chromium.org

[mustaq, 2015-05-25 17:01:11]

PTAL at my first attempt.

The fired PEs are not visible in JS (e.g. through jsbin.com/rilate). This needs
some investigation.

[Rick Byers, 2015-05-25 18:24:26]

This all seems reasonable for a start.  A few suggestions...

https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2119: if (event->isPointerEvent())
This code won't be reached.  Your pointer events satisfy isMouseEvent too.  It
looks like WheelEvent overrides isMouseEvent to return it to 'false', you should
probably do the same for PointerEvent.

https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2120: return
dispatchPointerEvent(static_pointer_cast<PointerEvent>(event));
Any idea why we need this special case at all?  Presumably there's some reason
mouse (and so pointer) events need it.  But would be nice to understand why /
try to avoid the extra logic if possible (lots of complex events, eg. keyboard,
presumably go down the generic path below).

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Even...
Source/core/events/EventTypeNames.in:169: pointerover
why add over/out but not enter/leave?  They're pretty related.  All 4 can be
left for a future CL if that's simplest...

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Touc...
File Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Touc...
Source/core/events/TouchEvent.cpp:137: fprintf(stderr, "mDebug %s:%d\n",
__FILE__, __LINE__);
remove this of course...

https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:4066: // to the targets as required.
I think you'll want to skip all this if your RuntimeEnabledFeature is disabled
(just to make sure we're not adding any overhead in that case).

https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:4076: TouchList* touchesForTarget =
touchesByTarget.get(pointerEventTarget);
I think this would be simpler if you didn't use the indexes eg.
(targetsForState), they're needed only for creating the complicated touchlists
specific to touch events.  In fact, looking to the future (where TE support is
an optional component on top of PE) we'll want to avoid creating them in the
first place unless some pointer event goes unhandled and there is a touch event.
 So perhaps this dispatching pointer event logic could be moved up above line
3947 and then iterate over the points directly (since there's a 1:1
correspondence between the input points and the generated pointer events). 
You'll then need to track some "pointer event was consumed" bit per point.

Maybe it would even make sense to change any point which had a consumed pointer
event to the 'stationary' state?  I.e. if the events are consumed for one finger
but not another, then we'd fire touchmove with only a single changedTouch?  This
will ultimately need to be part of whatever touch/pointer interoperability doc
we produce in the TECG, but seems like a reasonable place to start to me.  I
don't think the current behavior (where eg. consuming only pointermove could
suppress generation of touchstart/touchend) is likely to be where we want to
land.

[mustaq, 2015-05-26 15:00:09]

https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2119: if (event->isPointerEvent())
On 2015/05/25 18:24:26, Rick Byers wrote:
> This code won't be reached.  Your pointer events satisfy isMouseEvent too.  It
> looks like WheelEvent overrides isMouseEvent to return it to 'false', you
should
> probably do the same for PointerEvent.

Done.

https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2120: return
dispatchPointerEvent(static_pointer_cast<PointerEvent>(event));
On 2015/05/25 18:24:26, Rick Byers wrote:
> Any idea why we need this special case at all?  Presumably there's some reason
> mouse (and so pointer) events need it.  But would be nice to understand why /
> try to avoid the extra logic if possible (lots of complex events, eg.
keyboard,
> presumably go down the generic path below).

It seems none of the 57 callers to dispatchEvent() pass mouse/touch events. So
the type-specific handling should be nuked IMO. I have started a thread with
hayato@ to double-check.

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Even...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Even...
Source/core/events/EventTypeNames.in:169: pointerover
On 2015/05/25 18:24:26, Rick Byers wrote:
> why add over/out but not enter/leave?  They're pretty related.  All 4 can be
> left for a future CL if that's simplest...

Added all, including the ones for capture changes.

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Touc...
File Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Touc...
Source/core/events/TouchEvent.cpp:137: fprintf(stderr, "mDebug %s:%d\n",
__FILE__, __LINE__);
On 2015/05/25 18:24:26, Rick Byers wrote:
> remove this of course...

Done.

https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:4066: // to the targets as required.
On 2015/05/25 18:24:26, Rick Byers wrote:
> I think you'll want to skip all this if your RuntimeEnabledFeature is disabled
> (just to make sure we're not adding any overhead in that case).

Done. See my next comment on cleanup.

https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:4076: TouchList* touchesForTarget =
touchesByTarget.get(pointerEventTarget);
On 2015/05/25 18:24:26, Rick Byers wrote:
> I think this would be simpler if you didn't use the indexes eg.
> (targetsForState), they're needed only for creating the complicated touchlists
> specific to touch events.  In fact, looking to the future (where TE support is
> an optional component on top of PE) we'll want to avoid creating them in the
> first place unless some pointer event goes unhandled and there is a touch
event.
>  So perhaps this dispatching pointer event logic could be moved up above line
> 3947 and then iterate over the points directly (since there's a 1:1
> correspondence between the input points and the generated pointer events). 
> You'll then need to track some "pointer event was consumed" bit per point.
> 
> Maybe it would even make sense to change any point which had a consumed
pointer
> event to the 'stationary' state?  I.e. if the events are consumed for one
finger
> but not another, then we'd fire touchmove with only a single changedTouch? 
This
> will ultimately need to be part of whatever touch/pointer interoperability doc
> we produce in the TECG, but seems like a reasonable place to start to me.  I
> don't think the current behavior (where eg. consuming only pointermove could
> suppress generation of touchstart/touchend) is likely to be where we want to
> land.

I agree on your first point. I will isolate the indices from PE code soon, still
trying to discover the missing PEs.

Re firing TEs for "partially consumed" PEs, I think this would lead to
unnecessary complications for developers. The simplest model for the developers
could be: "handle either PE or TE, not both". If we allow complex PE+TE handling
(e.g. "can consume touchstart, yet can see pointermove"), this would leave too
many corner cases open for developers, and encourage out-of-spec web-pages by
naive ones which is bad for the long-term health of the web. Am I missing
something here?

[Rick Byers, 2015-05-27 23:58:07]

Looking good.  Need to add tests of course.

https://codereview.chromium.org/1144313003/diff/40001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (left):

https://codereview.chromium.org/1144313003/diff/40001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2117: return
dispatchTouchEvent(static_pointer_cast<TouchEvent>(event));
I agree with hayato that we should at least ASSERT (probably better to
RELEASE_ASSERT) here that we're not given touch/mouse/pointer to ensure no new
calls don't get added accidentally.  Alternately I'm fine keeping the existing
code now that we understand it (although delegating to the dispatchMouseEvent
method is probably better than repeating the logic).  Up to you.

Also add a commenting saying "these event types have a 'relatedTarget' property
which must be updated during dispatch.

https://codereview.chromium.org/1144313003/diff/40001/Source/core/events/Poin...
File Source/core/events/PointerEvent.cpp (right):

https://codereview.chromium.org/1144313003/diff/40001/Source/core/events/Poin...
Source/core/events/PointerEvent.cpp:84: return dispatcher.dispatch();
I think you need at least the logic from
MouseEventDispatchMediator::dispatchEvent which adjusts the relatedTarget. 
Hopefully there's a test you can add that covers that behavior (maybe try
breaking it for mouse and see what test fails, then make a similar test for
pointer events).

[Rick Byers, 2015-05-28 00:05:19]

On 2015/05/26 15:00:09, mustaq wrote:
> https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cp...
> Source/core/dom/Node.cpp:2119: if (event->isPointerEvent())
> On 2015/05/25 18:24:26, Rick Byers wrote:
> > This code won't be reached.  Your pointer events satisfy isMouseEvent too. 
It
> > looks like WheelEvent overrides isMouseEvent to return it to 'false', you
> should
> > probably do the same for PointerEvent.
> 
> Done.
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/dom/Node.cp...
> Source/core/dom/Node.cpp:2120: return
> dispatchPointerEvent(static_pointer_cast<PointerEvent>(event));
> On 2015/05/25 18:24:26, Rick Byers wrote:
> > Any idea why we need this special case at all?  Presumably there's some
reason
> > mouse (and so pointer) events need it.  But would be nice to understand why
/
> > try to avoid the extra logic if possible (lots of complex events, eg.
> keyboard,
> > presumably go down the generic path below).
> 
> It seems none of the 57 callers to dispatchEvent() pass mouse/touch events. So
> the type-specific handling should be nuked IMO. I have started a thread with
> hayato@ to double-check.
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Even...
> File Source/core/events/EventTypeNames.in (right):
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Even...
> Source/core/events/EventTypeNames.in:169: pointerover
> On 2015/05/25 18:24:26, Rick Byers wrote:
> > why add over/out but not enter/leave?  They're pretty related.  All 4 can be
> > left for a future CL if that's simplest...
> 
> Added all, including the ones for capture changes.
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Touc...
> File Source/core/events/TouchEvent.cpp (right):
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/events/Touc...
> Source/core/events/TouchEvent.cpp:137: fprintf(stderr, "mDebug %s:%d\n",
> __FILE__, __LINE__);
> On 2015/05/25 18:24:26, Rick Byers wrote:
> > remove this of course...
> 
> Done.
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
> Source/core/page/EventHandler.cpp:4066: // to the targets as required.
> On 2015/05/25 18:24:26, Rick Byers wrote:
> > I think you'll want to skip all this if your RuntimeEnabledFeature is
disabled
> > (just to make sure we're not adding any overhead in that case).
> 
> Done. See my next comment on cleanup.
> 
>
https://codereview.chromium.org/1144313003/diff/20001/Source/core/page/EventH...
> Source/core/page/EventHandler.cpp:4076: TouchList* touchesForTarget =
> touchesByTarget.get(pointerEventTarget);
> On 2015/05/25 18:24:26, Rick Byers wrote:
> > I think this would be simpler if you didn't use the indexes eg.
> > (targetsForState), they're needed only for creating the complicated
touchlists
> > specific to touch events.  In fact, looking to the future (where TE support
is
> > an optional component on top of PE) we'll want to avoid creating them in the
> > first place unless some pointer event goes unhandled and there is a touch
> event.
> >  So perhaps this dispatching pointer event logic could be moved up above
line
> > 3947 and then iterate over the points directly (since there's a 1:1
> > correspondence between the input points and the generated pointer events). 
> > You'll then need to track some "pointer event was consumed" bit per point.
> > 
> > Maybe it would even make sense to change any point which had a consumed
> pointer
> > event to the 'stationary' state?  I.e. if the events are consumed for one
> finger
> > but not another, then we'd fire touchmove with only a single changedTouch? 
> This
> > will ultimately need to be part of whatever touch/pointer interoperability
doc
> > we produce in the TECG, but seems like a reasonable place to start to me.  I
> > don't think the current behavior (where eg. consuming only pointermove could
> > suppress generation of touchstart/touchend) is likely to be where we want to
> > land.
> 
> I agree on your first point. I will isolate the indices from PE code soon,
still
> trying to discover the missing PEs.
> 
> Re firing TEs for "partially consumed" PEs, I think this would lead to
> unnecessary complications for developers. The simplest model for the
developers
> could be: "handle either PE or TE, not both". If we allow complex PE+TE
handling
> (e.g. "can consume touchstart, yet can see pointermove"), this would leave too
> many corner cases open for developers, and encourage out-of-spec web-pages by
> naive ones which is bad for the long-term health of the web. Am I missing
> something here?

The case to be worried about isn't one piece of code inconsistently handling
both PE and TE - I agree developers should never do that.  It's composition of
different libraries in the same page.  Eg. my site which uses PE embedds a
google maps widget which uses TE, or vice versa.  What happens if one of those
components consumes isPrimary PointerEvents only?  Should the other component
still get touch events for the other finger?

Eg. imagine a piano keyboard application that uses a volume slider component
from some widget framework.  When both the piano and volume slider are using
touch events, each can have it's own touchmove handler and call preventDefault
on the touchmoves sent to it.  But when the volume slider component gets updated
to use pointer events, now the piano keys won't register hits anymore when one
finger is dragging the volume slider.

If instead you still sent touchmove events for the touch points which did not
have pointer events consumed, then it would just continue to work fine.

Multi-touch is fairly rare outside of simple gestures like pinch, so I'm not
sure how important this is.  But the code may actually be simpler than what
you've got now so maybe it's a reasonable place to start?  It might also be
worth discussing this now on public-touchevents.

[mustaq, 2015-06-09 15:31:27]

ptal if you see any red flag. This is closer to the final state of the CL except
two things:
- isPrimary field calculation still needs work.
- fires double TEs when PE is disabled.

https://codereview.chromium.org/1144313003/diff/40001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (left):

https://codereview.chromium.org/1144313003/diff/40001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:2117: return
dispatchTouchEvent(static_pointer_cast<TouchEvent>(event));
On 2015/05/27 23:58:07, Rick Byers wrote:
> I agree with hayato that we should at least ASSERT (probably better to
> RELEASE_ASSERT) here that we're not given touch/mouse/pointer to ensure no new
> calls don't get added accidentally.  Alternately I'm fine keeping the existing
> code now that we understand it (although delegating to the dispatchMouseEvent
> method is probably better than repeating the logic).  Up to you.
> 
> Also add a commenting saying "these event types have a 'relatedTarget'
property
> which must be updated during dispatch.  

Done.

https://codereview.chromium.org/1144313003/diff/40001/Source/core/events/Poin...
File Source/core/events/PointerEvent.cpp (right):

https://codereview.chromium.org/1144313003/diff/40001/Source/core/events/Poin...
Source/core/events/PointerEvent.cpp:84: return dispatcher.dispatch();
On 2015/05/27 23:58:07, Rick Byers wrote:
> I think you need at least the logic from
> MouseEventDispatchMediator::dispatchEvent which adjusts the relatedTarget. 
> Hopefully there's a test you can add that covers that behavior (maybe try
> breaking it for mouse and see what test fails, then make a similar test for
> pointer events).

Done.

[USE eero AT chromium.org, 2015-06-09 18:51:01]

e.hakkinen@samsung.com changed reviewers:
+ e.hakkinen@samsung.com

[USE eero AT chromium.org, 2015-06-09 18:51:02]

https://codereview.chromium.org/1144313003/diff/60001/Source/core/frame/Event...
File Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1144313003/diff/60001/Source/core/frame/Event...
Source/core/frame/EventHandlerRegistry.h:75: static bool eventTypeToClass(const
AtomicString& eventType, EventHandlerClass* result);
You have not extended the eventTypeToClass function to handle also touch like
pointer events (such as pointerdown, pointermove, pointerup and pointercancel).

Due to that, if a document has PE handlers but not TE handlers, the renderer
thinks erroneously that touch events are not needed and does not pass them to
blink::EventHandler::handleTouchEvent, resulting in pointer events not to be
fired.

A simple fix is to modify the eventTypeToClass function to classify also touch
like pointer event types as TouchEvent types. After that, pointer event handlers
works.

A better fix might be to modify the eventTypeToClass function to classify touch
like pointer event types separately and to modify the EventHandlerRegistry class
and all other classes calling
EventHandlerRegistry::hasEventHandlers(EventHandlerRegistry::TouchEvent) to
consider a new TouchLikePointerEvent event handler class, too.
That would allow blink::EventHandler::handleTouchEvent to skip creation and
dispatching of TouchEvent or PointerEvent objects if there are no handlers for
touch events or touch like pointer events, respectively.

[mustaq, 2015-06-10 14:55:12]

On 2015/06/09 18:51:02, e_hakkinen wrote:
>
https://codereview.chromium.org/1144313003/diff/60001/Source/core/frame/Event...
> File Source/core/frame/EventHandlerRegistry.h (right):
> 
>
https://codereview.chromium.org/1144313003/diff/60001/Source/core/frame/Event...
> Source/core/frame/EventHandlerRegistry.h:75: static bool
eventTypeToClass(const
> AtomicString& eventType, EventHandlerClass* result);
> You have not extended the eventTypeToClass function to handle also touch like
> pointer events (such as pointerdown, pointermove, pointerup and
pointercancel).
> 
> Due to that, if a document has PE handlers but not TE handlers, the renderer
> thinks erroneously that touch events are not needed and does not pass them to
> blink::EventHandler::handleTouchEvent, resulting in pointer events not to be
> fired.
> 
> A simple fix is to modify the eventTypeToClass function to classify also touch
> like pointer event types as TouchEvent types. After that, pointer event
handlers
> works.
> 
> A better fix might be to modify the eventTypeToClass function to classify
touch
> like pointer event types separately and to modify the EventHandlerRegistry
class
> and all other classes calling
> EventHandlerRegistry::hasEventHandlers(EventHandlerRegistry::TouchEvent) to
> consider a new TouchLikePointerEvent event handler class, too.
> That would allow blink::EventHandler::handleTouchEvent to skip creation and
> dispatching of TouchEvent or PointerEvent objects if there are no handlers for
> touch events or touch like pointer events, respectively.

Done with the simple fix. We will revisit the better solution while we will work
on firing PEs on mouse events.

[mustaq, 2015-06-10 14:58:14]

ptal, this should be ready to go now. Completed the 2 missing pieces:
- done with isPrimary determination
- fixed the double event issue in the test.

[Rick Byers, 2015-06-11 03:39:35]

On 2015/06/09 18:51:02, e_hakkinen wrote:
>
https://codereview.chromium.org/1144313003/diff/60001/Source/core/frame/Event...
> File Source/core/frame/EventHandlerRegistry.h (right):
> 
>
https://codereview.chromium.org/1144313003/diff/60001/Source/core/frame/Event...
> Source/core/frame/EventHandlerRegistry.h:75: static bool
eventTypeToClass(const
> AtomicString& eventType, EventHandlerClass* result);
> You have not extended the eventTypeToClass function to handle also touch like
> pointer events (such as pointerdown, pointermove, pointerup and
pointercancel).
> 
> Due to that, if a document has PE handlers but not TE handlers, the renderer
> thinks erroneously that touch events are not needed and does not pass them to
> blink::EventHandler::handleTouchEvent, resulting in pointer events not to be
> fired.
> 
> A simple fix is to modify the eventTypeToClass function to classify also touch
> like pointer event types as TouchEvent types. After that, pointer event
handlers
> works.
> 
> A better fix might be to modify the eventTypeToClass function to classify
touch
> like pointer event types separately and to modify the EventHandlerRegistry
class
> and all other classes calling
> EventHandlerRegistry::hasEventHandlers(EventHandlerRegistry::TouchEvent) to
> consider a new TouchLikePointerEvent event handler class, too.
> That would allow blink::EventHandler::handleTouchEvent to skip creation and
> dispatching of TouchEvent or PointerEvent objects if there are no handlers for
> touch events or touch like pointer events, respectively.

Thanks for raising this!  Mustaq and I had talked a bit about this.  Ultimately
we're going to have to plumb some extra information back to chromium.  When
there are pointer handlers but NO touch handlers, we'll want to send the touch
events asynchronously (like when we're scrolling except without the throttle). 
So I initially suggested we should just put this off for a future CL and rely on
a dummy touch handler to defeat the optimization for now but I like your first
suggestion better: just treat the pointer handlers as if they are touch handlers
for now.  Thanks!

[Rick Byers, 2015-06-11 04:14:56]

This is looking close!

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/touch/touch-pointer-events.html (right):

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:1: <!DOCTYPE HTML>
Ultimately we're going to want to try to bring tests like this into some W3C
test suite.  Perhaps at a minimum we should put all our new pointer events tests
into one place (fast/events/pointer/* maybe)?

I guess it's harder to write a testharness.js test in this virtual test suite
setup (since you have two different modes), right?  Ideally we'd be using W3C
test style as much as possible from the start to make the process easier later. 
But we can always convert them later if necessary...

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:30: return
event.eventPhase == 0 ? "none" :
nit: rather than hard-coding the constants you should use, eg.
Event.CAPTURING_PHASE (then we can see at a glance that the values match the
strings).

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:64:
eventSender.touchEnd();
I think you need to pass the ID (0) here to get this to send a touchend. 
touchend is missing from your test output.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/dom/Node.c...
Source/core/dom/Node.cpp:2112: // during dispatch. This generic dispatcher can't
handle this.
Doesn't the Javascript EventTarget.dispatchEvent case hit this path?  I'd expect
this to get triggered by at least a few tests.  If I'm right then you probably
do need the original pattern.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/frame/Even...
File Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/frame/Even...
Source/core/frame/EventHandlerRegistry.h:33: PointerEvent,
It's kind of misleading to add this and then not use it.  Just leave it out for
now?

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3815: // TouchStationary state is not
converted to touch events, so fall through to assert.
remove incorrect (and otherwise fairly useless) comment

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3944: using TouchInfo = struct {
curious: how is this syntax different from the simpler 'struct TouchInfo {
...}'?

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4005: FloatPoint adjustedPagePoint =
pagePoint.scaledBy(scaleFactor);
kind of silly to create these locals just to assign below.  Remove the
temporaries.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4017: if
(RuntimeEnabledFeatures::pointerEventEnabled()) {
could some of this be factored out to another function without too much pain? 
EventHandler::handleTouchEvent is already a bit of a monster!

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4019: unsigned primaryIdForTouch =
hasPrimaryIdForTouch? m_primaryIdByPointerType.get(PointerTypeTouch) : 0;
nit: space before ?

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4051: pointerEventInit.setClientY(0);
These are wrong - it should have client co-ordinates (same as in the touch
events).  Please add to test.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4055: pointerEventInit.setButtons(0);
This is wrong, should be 1
http://w3c.github.io/pointerevents/#button-states
Please test this too.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4072:
m_primaryIdByPointerType.remove(PointerTypeTouch);
This logic deserves a test (at first I thought it was wrong, but I see it's
right) - eg.  Eg. pointerdown ID=1, pointerdown ID=2, pointerup ID=1.  Now
verify that a new pointerdown isn't considered primary (and 2 didn't suddenly
become primary).

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.h:404: WillBeHeapHashMap<String, unsigned>
m_primaryIdByPointerType;
Since you're not holding garbage collected types here, I think you just want
WTF::HashMap (no need for Oilpan).

[USE eero AT chromium.org, 2015-06-11 11:12:42]

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4027: const AtomicString&
eventName(pointerEventNameForTouchPointState(pointState));
This does not handle correctly touch points whose state is
PlatformTouchPoint::TouchStationary. Those cases seem to even cause some kind of
corruption resulting in an assert failure from line 4161.

Those touch points should probably be ignored by adding a conditional continue
statement.

[mustaq, 2015-06-12 16:05:24]

ptal, I have addressed all comments so far.

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/touch/touch-pointer-events.html (right):

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:1: <!DOCTYPE HTML>
On 2015/06/11 04:14:56, Rick Byers wrote:
> Ultimately we're going to want to try to bring tests like this into some W3C
> test suite.  Perhaps at a minimum we should put all our new pointer events
tests
> into one place (fast/events/pointer/* maybe)?

Done: moved to "pointerevents", at least preserving the w3c folder name for now
:)

> I guess it's harder to write a testharness.js test in this virtual test suite
> setup (since you have two different modes), right?  Ideally we'd be using W3C
> test style as much as possible from the start to make the process easier
later. 
> But we can always convert them later if necessary...

The bottleneck is the eventSender.touch*() here. We already have two mode
pointer-event tests from w3c: see
imported/web-platform-tests/pointerevents/pointerevent_constructor.html

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:30: return
event.eventPhase == 0 ? "none" :
On 2015/06/11 04:14:56, Rick Byers wrote:
> nit: rather than hard-coding the constants you should use, eg.
> Event.CAPTURING_PHASE (then we can see at a glance that the values match the
> strings).

Done.

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:64:
eventSender.touchEnd();
On 2015/06/11 04:14:56, Rick Byers wrote:
> I think you need to pass the ID (0) here to get this to send a touchend. 
> touchend is missing from your test output.

Done, missed the "release" calls.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/dom/Node.c...
Source/core/dom/Node.cpp:2112: // during dispatch. This generic dispatcher can't
handle this.
On 2015/06/11 04:14:56, Rick Byers wrote:
> Doesn't the Javascript EventTarget.dispatchEvent case hit this path?  I'd
expect
> this to get triggered by at least a few tests.  If I'm right then you probably
> do need the original pattern.

Good catch! Back to my first patch here.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/frame/Even...
File Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/frame/Even...
Source/core/frame/EventHandlerRegistry.h:33: PointerEvent,
On 2015/06/11 04:14:56, Rick Byers wrote:
> It's kind of misleading to add this and then not use it.  Just leave it out
for
> now?

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3815: // TouchStationary state is not
converted to touch events, so fall through to assert.
On 2015/06/11 04:14:56, Rick Byers wrote:
> remove incorrect (and otherwise fairly useless) comment

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3944: using TouchInfo = struct {
On 2015/06/11 04:14:56, Rick Byers wrote:
> curious: how is this syntax different from the simpler 'struct TouchInfo {
> ...}'?

I need to use 'typedef struct {...} TouchInfo' here because I am reusing the
definition later on. To me, typedef is slightly less intuitive here than it's
C++11 counterpart 'using'.

http://stackoverflow.com/questions/10747810/what-is-the-difference-between-ty...

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4005: FloatPoint adjustedPagePoint =
pagePoint.scaledBy(scaleFactor);
On 2015/06/11 04:14:56, Rick Byers wrote:
> kind of silly to create these locals just to assign below.  Remove the
> temporaries.

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4017: if
(RuntimeEnabledFeatures::pointerEventEnabled()) {
On 2015/06/11 04:14:56, Rick Byers wrote:
> could some of this be factored out to another function without too much pain? 
> EventHandler::handleTouchEvent is already a bit of a monster!

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4019: unsigned primaryIdForTouch =
hasPrimaryIdForTouch? m_primaryIdByPointerType.get(PointerTypeTouch) : 0;
On 2015/06/11 04:14:56, Rick Byers wrote:
> nit: space before ?

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4027: const AtomicString&
eventName(pointerEventNameForTouchPointState(pointState));
On 2015/06/11 11:12:42, e_hakkinen wrote:
> This does not handle correctly touch points whose state is
> PlatformTouchPoint::TouchStationary. Those cases seem to even cause some kind
of
> corruption resulting in an assert failure from line 4161.
> 
> Those touch points should probably be ignored by adding a conditional continue
> statement.

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4051: pointerEventInit.setClientY(0);
On 2015/06/11 04:14:56, Rick Byers wrote:
> These are wrong - it should have client co-ordinates (same as in the touch
> events).  Please add to test.

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4055: pointerEventInit.setButtons(0);
On 2015/06/11 04:14:56, Rick Byers wrote:
> This is wrong, should be 1
> http://w3c.github.io/pointerevents/#button-states
> Please test this too.

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4072:
m_primaryIdByPointerType.remove(PointerTypeTouch);
On 2015/06/11 04:14:56, Rick Byers wrote:
> This logic deserves a test (at first I thought it was wrong, but I see it's
> right) - eg.  Eg. pointerdown ID=1, pointerdown ID=2, pointerup ID=1.  Now
> verify that a new pointerdown isn't considered primary (and 2 didn't suddenly
> become primary).

Done.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.h:404: WillBeHeapHashMap<String, unsigned>
m_primaryIdByPointerType;
On 2015/06/11 04:14:56, Rick Byers wrote:
> Since you're not holding garbage collected types here, I think you just want
> WTF::HashMap (no need for Oilpan).

Done.

[mustaq, 2015-06-12 20:37:18]

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4072:
m_primaryIdByPointerType.remove(PointerTypeTouch);
On 2015/06/12 16:05:23, mustaq wrote:
> On 2015/06/11 04:14:56, Rick Byers wrote:
> > This logic deserves a test (at first I thought it was wrong, but I see it's
> > right) - eg.  Eg. pointerdown ID=1, pointerdown ID=2, pointerup ID=1.  Now
> > verify that a new pointerdown isn't considered primary (and 2 didn't
suddenly
> > become primary).
> 
> Done.

Just discovered that Chrome reuses 'retired' pointerIds, and this is supported
by the spec! The current logic fails in the following reuse case:
- Finger1 touches the screen, id=0, primary.
- Finger2 touches the screen, id=1, not primary.
- Finger1 goes off the screen.
- Another finger (Finger3/Finger1) touches the screen, Chrome reuses id=0, which
is wrongly considered primary.

Solution:
- Track the set of current IDs.
- The first id that enters an empty set is primary.
- Store the id only as long the id is not touch-released.

I will work on a fix now.

[Rick Byers, 2015-06-12 21:05:54]

On 2015/06/12 20:37:18, mustaq wrote:
>
https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
> Source/core/page/EventHandler.cpp:4072:
> m_primaryIdByPointerType.remove(PointerTypeTouch);
> On 2015/06/12 16:05:23, mustaq wrote:
> > On 2015/06/11 04:14:56, Rick Byers wrote:
> > > This logic deserves a test (at first I thought it was wrong, but I see
it's
> > > right) - eg.  Eg. pointerdown ID=1, pointerdown ID=2, pointerup ID=1.  Now
> > > verify that a new pointerdown isn't considered primary (and 2 didn't
> suddenly
> > > become primary).
> > 
> > Done.
> 
> Just discovered that Chrome reuses 'retired' pointerIds, and this is supported
> by the spec! The current logic fails in the following reuse case:
> - Finger1 touches the screen, id=0, primary.
> - Finger2 touches the screen, id=1, not primary.
> - Finger1 goes off the screen.
> - Another finger (Finger3/Finger1) touches the screen, Chrome reuses id=0,
which
> is wrongly considered primary.
> 
> Solution:
> - Track the set of current IDs.
> - The first id that enters an empty set is primary.
> - Store the id only as long the id is not touch-released.
> 
> I will work on a fix now.

Ah, of course - makes sense!  Sorry I didn't think of this.

[mustaq, 2015-06-15 18:04:06]

On 2015/06/12 21:05:54, Rick Byers (Slow until 6-22) wrote:
> On 2015/06/12 20:37:18, mustaq wrote:
> >
>
https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
> > File Source/core/page/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
> > Source/core/page/EventHandler.cpp:4072:
> > m_primaryIdByPointerType.remove(PointerTypeTouch);
> > On 2015/06/12 16:05:23, mustaq wrote:
> > > On 2015/06/11 04:14:56, Rick Byers wrote:
> > > > This logic deserves a test (at first I thought it was wrong, but I see
> it's
> > > > right) - eg.  Eg. pointerdown ID=1, pointerdown ID=2, pointerup ID=1. 
Now
> > > > verify that a new pointerdown isn't considered primary (and 2 didn't
> > suddenly
> > > > become primary).
> > > 
> > > Done.
> > 
> > Just discovered that Chrome reuses 'retired' pointerIds, and this is
supported
> > by the spec! The current logic fails in the following reuse case:
> > - Finger1 touches the screen, id=0, primary.
> > - Finger2 touches the screen, id=1, not primary.
> > - Finger1 goes off the screen.
> > - Another finger (Finger3/Finger1) touches the screen, Chrome reuses id=0,
> which
> > is wrongly considered primary.
> > 
> > Solution:
> > - Track the set of current IDs.
> > - The first id that enters an empty set is primary.
> > - Store the id only as long the id is not touch-released.
> > 
> > I will work on a fix now.
> 
> Ah, of course - makes sense!  Sorry I didn't think of this.

Done, ptal.

[Rick Byers, 2015-06-16 17:25:39]

Great!  Just a couple minor nits now

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/touch/touch-pointer-events.html (right):

https://codereview.chromium.org/1144313003/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/touch/touch-pointer-events.html:1: <!DOCTYPE HTML>
On 2015/06/12 16:05:23, mustaq wrote:
> On 2015/06/11 04:14:56, Rick Byers wrote:
> > Ultimately we're going to want to try to bring tests like this into some W3C
> > test suite.  Perhaps at a minimum we should put all our new pointer events
> tests
> > into one place (fast/events/pointer/* maybe)?
> 
> Done: moved to "pointerevents", at least preserving the w3c folder name for
now
> :)
> 
> > I guess it's harder to write a testharness.js test in this virtual test
suite
> > setup (since you have two different modes), right?  Ideally we'd be using
W3C
> > test style as much as possible from the start to make the process easier
> later. 
> > But we can always convert them later if necessary...
> 
> The bottleneck is the eventSender.touch*() here. We already have two mode
> pointer-event tests from w3c: see
> imported/web-platform-tests/pointerevents/pointerevent_constructor.html

I'm fine considering the eventSender question as orthogonal (i.e. feel free to
right testharness.js tests using eventSender).  Eventually we should have an
input simulation API at the W3C and we can replace our uses of eventSender with
that MUCH easier than we can rewrite all the tests to use testharness.js style.

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/120001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3944: using TouchInfo = struct {
On 2015/06/12 16:05:24, mustaq wrote:
> On 2015/06/11 04:14:56, Rick Byers wrote:
> > curious: how is this syntax different from the simpler 'struct TouchInfo {
> > ...}'?
> 
> I need to use 'typedef struct {...} TouchInfo' here because I am reusing the
> definition later on. To me, typedef is slightly less intuitive here than it's
> C++11 counterpart 'using'.
> 
>
http://stackoverflow.com/questions/10747810/what-is-the-difference-between-ty...

Ah, cool - thanks!

https://codereview.chromium.org/1144313003/diff/180001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.h (right):

https://codereview.chromium.org/1144313003/diff/180001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.h:5: #ifndef PointerEventUtils_h
Do you have plans to add other "utils" here?  Usually our filenames match the
class name (I know some editor workflows expect that).  Maybe just call this
PointerIdManager.* for now?

https://codereview.chromium.org/1144313003/diff/200001/LayoutTests/fast/event...
File LayoutTests/fast/events/pointerevents/touch-pointer-events.html (right):

https://codereview.chromium.org/1144313003/diff/200001/LayoutTests/fast/event...
LayoutTests/fast/events/pointerevents/touch-pointer-events.html:148: 
we should probably also verify that (after releasing the last pointer) a new
touchstart will have isPrimary=true.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.cpp:31: m_hasPrimaryId[type] = true;
ASSERT(!m_hasPrimaryId[type]) ?

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.cpp:39: m_hasPrimaryId[type] = false;
ASSERT(m_hasPrimaryId[type]) ?

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.h (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.h:18: // w/o changing the corresponding
type in PointerEvent.idl. Investigate.
Note there is a difference.  At the PointerEvent API level, the "type" is a
string because JS can add arbitrary values.  But here we're not interested in
tracking the primary pointer ID for arbitrary pointer types, only for the
specific pointer types blink supports natively.  So it makes some sense that
there are two different definitions.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3843: TouchInfo& touchInfo = touchInfos[i];
would it be a little cleaner to stick the PlatformTouchPoint into the TouchInfo
(then you're really just iterating across a single list)?

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3859: bool isBubbling = !isEnterOrLeave;
nit isCancelable and isBubbling local variables only seem to decrease
readability to me - just stick the value right into the pointerEventInit?

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4062: handlePointerEventsFromTouchEvent(event,
points, touchInfos);
nit: dispatchPointerEventsForTouchEvent?  We're not really "handle"ing pointer
events here (in the event handler sense).

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4065: 
now that we have this TouchInfo, perhaps we should also move the firing of touch
events out into it's own function too to try to make this function more
readable?

[mustaq, 2015-06-16 18:21:17]

Patchset #8 (id:160001) has been deleted

[mustaq, 2015-06-16 20:18:43]

ptal.

https://codereview.chromium.org/1144313003/diff/180001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.h (right):

https://codereview.chromium.org/1144313003/diff/180001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.h:5: #ifndef PointerEventUtils_h
On 2015/06/16 17:25:38, Rick Byers (Slow until 6-22) wrote:
> Do you have plans to add other "utils" here?  Usually our filenames match the
> class name (I know some editor workflows expect that).  Maybe just call this
> PointerIdManager.* for now?

Done.

https://codereview.chromium.org/1144313003/diff/200001/LayoutTests/fast/event...
File LayoutTests/fast/events/pointerevents/touch-pointer-events.html (right):

https://codereview.chromium.org/1144313003/diff/200001/LayoutTests/fast/event...
LayoutTests/fast/events/pointerevents/touch-pointer-events.html:148: 
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> we should probably also verify that (after releasing the last pointer) a new
> touchstart will have isPrimary=true.

Done.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.cpp:31: m_hasPrimaryId[type] = true;
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> ASSERT(!m_hasPrimaryId[type]) ?

I relied on deduping-through-set here instead of an explicit assert because
apparently we have some tests violating unique id assumptions.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.cpp:39: m_hasPrimaryId[type] = false;
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> ASSERT(m_hasPrimaryId[type]) ?

Ditto.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.h (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.h:18: // w/o changing the corresponding
type in PointerEvent.idl. Investigate.
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> Note there is a difference.  At the PointerEvent API level, the "type" is a
> string because JS can add arbitrary values.  But here we're not interested in
> tracking the primary pointer ID for arbitrary pointer types, only for the
> specific pointer types blink supports natively.  So it makes some sense that
> there are two different definitions.

What about adding a new method pointerTypeEnum() in PointerEvent.h, in addition
to existing pointerType()? We would be able to avoid, at least in our codebase,
the use of strings for what is otherwise an enum.

It's not clear to me why the PE spec uses free-form strings, could be an "enum
of strings" instead, as in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3843: TouchInfo& touchInfo = touchInfos[i];
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> would it be a little cleaner to stick the PlatformTouchPoint into the
TouchInfo
> (then you're really just iterating across a single list)?

Done.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:3859: bool isBubbling = !isEnterOrLeave;
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> nit isCancelable and isBubbling local variables only seem to decrease
> readability to me - just stick the value right into the pointerEventInit?

Done.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4062: handlePointerEventsFromTouchEvent(event,
points, touchInfos);
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> nit: dispatchPointerEventsForTouchEvent?  We're not really "handle"ing pointer
> events here (in the event handler sense).

Done.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4065: 
On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> now that we have this TouchInfo, perhaps we should also move the firing of
touch
> events out into it's own function too to try to make this function more
> readable?

Done, thanks for observing this.

A quick thought: we are returning true when any TE is consumed, regardless of
the PE case. Is it really right?

[Rick Byers, 2015-06-18 17:40:08]

LGTM

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.cpp:31: m_hasPrimaryId[type] = true;
On 2015/06/16 20:18:42, mustaq wrote:
> On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> > ASSERT(!m_hasPrimaryId[type]) ?
> 
> I relied on deduping-through-set here instead of an explicit assert because
> apparently we have some tests violating unique id assumptions.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Oh right, clusterfuzz can definitely trigger this so we can't assert.  Ok, this
is fine - thanks.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.h (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.h:18: // w/o changing the corresponding
type in PointerEvent.idl. Investigate.
On 2015/06/16 20:18:43, mustaq wrote:
> On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> > Note there is a difference.  At the PointerEvent API level, the "type" is a
> > string because JS can add arbitrary values.  But here we're not interested
in
> > tracking the primary pointer ID for arbitrary pointer types, only for the
> > specific pointer types blink supports natively.  So it makes some sense that
> > there are two different definitions.
>  
> It's not clear to me why the PE spec uses free-form strings, could be an "enum
> of strings" instead, as in:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It's because we want script to be able to fire different types of pointer
events, effectively making pointer events extensible.  Eg. ideally leap motion
could create a library that generates PointerEvent objects with
pointertype="depthcamera" (maybe built on top of some low-level USB API or
something).

That said, I think there would be value in changing the spec to combine a fixed
enum with a custom string as a union type.  Then the pre-defined values are
still concreted specified by the IDL (and could perhaps be optimized in a
type-safe language) while still permitting extensibility.

>
> What about adding a new method pointerTypeEnum() in PointerEvent.h, in
addition
> to existing pointerType()? We would be able to avoid, at least in our
codebase,
> the use of strings for what is otherwise an enum.

Sure, that's fine with me.  You could even keep an enum value internally and
only fill in the string in the unusual case that script sets it to a
non-standard value.  Anyway feel free to leave this TODO here and make such a
change in a future CL.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4065: 
On 2015/06/16 20:18:43, mustaq wrote:
> On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> > now that we have this TouchInfo, perhaps we should also move the firing of
> touch
> > events out into it's own function too to try to make this function more
> > readable?
> 
> Done, thanks for observing this.
> 
> A quick thought: we are returning true when any TE is consumed, regardless of
> the PE case. Is it really right?

Yes, I think it's right but ideally would get some test coverage (a follow-up CL
is fine with me though).  In particular if I do a single touch drag and call
preventDefault on the pointer events, that should suppress the generation of
touch events but NOT suppress scrolling (since with pointer events scrolling is
only suppressed via touch-action).  So the return value here is really "was the
touch event consumed, disabling gesture generation".

This probably deserves a comment here.  Eg. "The disposition of any pointer
events affects only the generation of touch events.  If pointer events were
handled (and hence no touch events generated) that is still equivalent to the
touch events going unhandled because pointer event handler don't block scroll
gesture generation."

This is actually a little more subtle than this for GestureTap, but we can worry
about that later IMHO (the exact right behavior isn't necessarily obvious -
needs to get added to the spec: https://github.com/w3c/pointerevents/issues/7

[mustaq, 2015-06-18 18:20:53]

Thanks, will CQ now.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.cpp:31: m_hasPrimaryId[type] = true;
On 2015/06/18 17:40:07, Rick Byers (Slow until 6-22) wrote:
> On 2015/06/16 20:18:42, mustaq wrote:
> > On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> > > ASSERT(!m_hasPrimaryId[type]) ?
> > 
> > I relied on deduping-through-set here instead of an explicit assert because
> > apparently we have some tests violating unique id assumptions.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Oh right, clusterfuzz can definitely trigger this so we can't assert.  Ok,
this
> is fine - thanks.

Acknowledged.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
File Source/core/events/PointerEventUtils.h (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/events/Poi...
Source/core/events/PointerEventUtils.h:18: // w/o changing the corresponding
type in PointerEvent.idl. Investigate.
On 2015/06/18 17:40:07, Rick Byers (Slow until 6-22) wrote:
> On 2015/06/16 20:18:43, mustaq wrote:
> > On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> > > Note there is a difference.  At the PointerEvent API level, the "type" is
a
> > > string because JS can add arbitrary values.  But here we're not interested
> in
> > > tracking the primary pointer ID for arbitrary pointer types, only for the
> > > specific pointer types blink supports natively.  So it makes some sense
that
> > > there are two different definitions.
> >  
> > It's not clear to me why the PE spec uses free-form strings, could be an
"enum
> > of strings" instead, as in:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> It's because we want script to be able to fire different types of pointer
> events, effectively making pointer events extensible.  Eg. ideally leap motion
> could create a library that generates PointerEvent objects with
> pointertype="depthcamera" (maybe built on top of some low-level USB API or
> something).
> 
> That said, I think there would be value in changing the spec to combine a
fixed
> enum with a custom string as a union type.  Then the pre-defined values are
> still concreted specified by the IDL (and could perhaps be optimized in a
> type-safe language) while still permitting extensibility.
> 
> >
> > What about adding a new method pointerTypeEnum() in PointerEvent.h, in
> addition
> > to existing pointerType()? We would be able to avoid, at least in our
> codebase,
> > the use of strings for what is otherwise an enum.
> 
> Sure, that's fine with me.  You could even keep an enum value internally and
> only fill in the string in the unusual case that script sets it to a
> non-standard value.  Anyway feel free to leave this TODO here and make such a
> change in a future CL.

Acknowledged.

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1144313003/diff/200001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:4065: 
On 2015/06/18 17:40:07, Rick Byers (Slow until 6-22) wrote:
> On 2015/06/16 20:18:43, mustaq wrote:
> > On 2015/06/16 17:25:39, Rick Byers (Slow until 6-22) wrote:
> > > now that we have this TouchInfo, perhaps we should also move the firing of
> > touch
> > > events out into it's own function too to try to make this function more
> > > readable?
> > 
> > Done, thanks for observing this.
> > 
> > A quick thought: we are returning true when any TE is consumed, regardless
of
> > the PE case. Is it really right?
> 
> Yes, I think it's right but ideally would get some test coverage (a follow-up
CL
> is fine with me though).  In particular if I do a single touch drag and call
> preventDefault on the pointer events, that should suppress the generation of
> touch events but NOT suppress scrolling (since with pointer events scrolling
is
> only suppressed via touch-action).  So the return value here is really "was
the
> touch event consumed, disabling gesture generation".
> 
> This probably deserves a comment here.  Eg. "The disposition of any pointer
> events affects only the generation of touch events.  If pointer events were
> handled (and hence no touch events generated) that is still equivalent to the
> touch events going unhandled because pointer event handler don't block scroll
> gesture generation."
> 
> This is actually a little more subtle than this for GestureTap, but we can
worry
> about that later IMHO (the exact right behavior isn't necessarily obvious -
> needs to get added to the spec: https://github.com/w3c/pointerevents/issues/7

Done.

[mustaq, 2015-06-18 18:23:17]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2015-06-18 18:23:17]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1144313003/#ps280001
(title: "Rebased. TODO fixes.")

[commit-bot: I haz the power, 2015-06-18 18:24:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1144313003/280001

[commit-bot: I haz the power, 2015-06-18 19:47:54]

Committed patchset #13 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197390