Send node transition events for touch events

Send node transition events for touch events

This CL adds firing of pointerover/enter when touch
starts on an element, and pointerout/leave when touch
ends.

This CL also includes some refactoring around
our PointerEvents code. We isolated:
- PE handling details off from EventHandler,
- PE states & dispatches into PointerEventManager, and
- PE creation & id-management into PointerEventFactory.

BUG=583376
Committed: https://crrev.com/99d107b0e05edcd98f74b8d3a889e5151519e14d
Cr-Commit-Position: refs/heads/master@{#375261}

[Navid Zolghadr, 2016-02-05 15:51:05]

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

[Navid Zolghadr, 2016-02-05 15:51:06]

This is the change for sending pointerover/enter/out/leave for touch events.
Before I go and fix/add the tests for this change that would be great if you
guys can have a quick look and see if this is the right approach to you guys or
not.

[mustaq, 2016-02-08 17:01:34]

The change makes EventHandler's PE handling more modular for sure. However, the
transition event task duplication is bad. As we discussed offline now, consider
moving the function as a util method to somewhere.

Could be a static one in EventHandler. But EventHandler is too big, so putting
util-like methods away would be a good cleanup.

I suggest putting the method in a new EventHandlerUtil class.

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventManager.cpp (right):

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventManager.cpp:193: void
PointerEventManager::sendNodeTransitionEvents(
This duplication looks bad. All you need here is a PE-only version of
EventHandler::sendNodeTransitionEvents(), right? We need a better way to address
this.

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventManager.h (right):

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventManager.h:53: void
remove(const PassRefPtrWillBeRawPtr<PointerEvent>);
We can make it private now since the manager class controls id lifetime, right?

[Navid Zolghadr, 2016-02-10 16:25:29]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org

[Navid Zolghadr, 2016-02-10 16:25:29]

Mustaq, I moved as much pointerevent related logic as I could at this stage from
EventHandler to PointerEventManager class.

Dave, is the changing of the eventToEventResult functions into static members of
EventHandler okay with you or do you want me to create another class or
something for them?

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventManager.cpp (right):

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventManager.cpp:193: void
PointerEventManager::sendNodeTransitionEvents(
On 2016/02/08 17:01:34, mustaq wrote:
> This duplication looks bad. All you need here is a PE-only version of
> EventHandler::sendNodeTransitionEvents(), right? We need a better way to
address
> this.

Done.

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventManager.h (right):

https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventManager.h:53: void
remove(const PassRefPtrWillBeRawPtr<PointerEvent>);
On 2016/02/08 17:01:34, mustaq wrote:
> We can make it private now since the manager class controls id lifetime,
right?

Yup.

[dtapuska, 2016-02-10 16:30:50]

On 2016/02/10 16:25:29, Navid Zolghadr wrote:
> Mustaq, I moved as much pointerevent related logic as I could at this stage
from
> EventHandler to PointerEventManager class.
> 
> Dave, is the changing of the eventToEventResult functions into static members
of
> EventHandler okay with you or do you want me to create another class or
> something for them?
> 
>
https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/PointerEventManager.cpp:193: void
> PointerEventManager::sendNodeTransitionEvents(
> On 2016/02/08 17:01:34, mustaq wrote:
> > This duplication looks bad. All you need here is a PE-only version of
> > EventHandler::sendNodeTransitionEvents(), right? We need a better way to
> address
> > this.
> 
> Done.
> 
>
https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/PointerEventManager.h (right):
> 
>
https://codereview.chromium.org/1670073004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/PointerEventManager.h:53: void
> remove(const PassRefPtrWillBeRawPtr<PointerEvent>);
> On 2016/02/08 17:01:34, mustaq wrote:
> > We can make it private now since the manager class controls id lifetime,
> right?
> 
> Yup.

I'm fine with moving it to a static.

[mustaq, 2016-02-11 16:15:16]

The refactoring makes the code much cleaner and modular, thanks.

Only one major concern here: m_nodeUnderMouse and m_nodeUnderPointer(mouseId)
shouldn't be different...

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html
(right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html:200:
testPEParamsSingleTouch(x, y);
The enter/leave events on outer divs make the output too long, and eventPhase ==
TARGET comparison can't suppress them. Since we are focusing on only the
parameters here and below, let's make (x,y) a point on the outer div ("grey")
and get rid of the phase comparison.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEvent.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEvent.h:28: int pointerId() const {
return m_pointerId; }
Good catch, thanks.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.h:72:
HashMap<IncomingId, int, WTF::PairHash<int, int>,
WTF::PairHashTraits<PointerEventFactory::UnsignedHash,
PointerEventFactory::UnsignedHash>> m_idMapping;
Do you think "PointerEventFactory::" is redundant here?

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:355: WebInputEventResult
responseA, WebInputEventResult responseB)
s/response/result/

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:371:
PassRefPtrWillBeRawPtr<Event> event, bool res)
s/res/result/

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1608:
m_pointerEventManager.sendNodeTransitionEvents(lastNodeUnderMouse.get(),
I think you can pass the RefPtrWillBeMember params directly here w/o switching
to raw pointers here.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3557: touchInfo.consumed =
m_pointerEventManager.sendTouchPointerEvent(
Please split the line into two, long "x = y != z" is hard to read.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3912:
m_pointerEventManager.clearPreventMouseEventForPointerTypeMouse();
PE manager's clearPreventMouseEventForPointerTypeMouse() method sounds
mysterious here since there is no setter method, and it is an internal state of
PE manager. I think moving conditionallyEnableMouseEventForPointerTypeMouse()
into PE manager and hiding the clear...() method is cleaner. Wdyt?

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:82:
WillBeHeapHashMap<int, RefPtrWillBeMember<EventTarget>> m_nodeUnderPointer;
This surely needs some attention. We can easily have:
  m_nodeUnderPointer.get(mouse) != EventHandler::m_nodeUnderMouse
for (legacy) mouse events that don't fire PEs.

What if we add {get,set}NodeUnderMouse() here to get rid of the state in
EventHandler?

[Navid Zolghadr, 2016-02-11 16:34:52]

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html
(right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html:200:
testPEParamsSingleTouch(x, y);
On 2016/02/11 16:15:15, mustaq wrote:
> The enter/leave events on outer divs make the output too long, and eventPhase
==
> TARGET comparison can't suppress them. Since we are focusing on only the
> parameters here and below, let's make (x,y) a point on the outer div ("grey")
> and get rid of the phase comparison.

But I think this is good to test those events here as well. Since we never test
the properties of the pointerenter/leave that we send to the parents of a node
anywhere else. Do you still want me to strip them out?

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.h:72:
HashMap<IncomingId, int, WTF::PairHash<int, int>,
WTF::PairHashTraits<PointerEventFactory::UnsignedHash,
PointerEventFactory::UnsignedHash>> m_idMapping;
On 2016/02/11 16:15:16, mustaq wrote:
> Do you think "PointerEventFactory::" is redundant here?

It definitely works without. I intentionally added that to make it clear that
this enum did belong to this class. If you think it doesn't add that much
clarity I can remove it. Having this do you want me to remove it?

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:355: WebInputEventResult
responseA, WebInputEventResult responseB)
On 2016/02/11 16:15:16, mustaq wrote:
> s/response/result/

Will do.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:371:
PassRefPtrWillBeRawPtr<Event> event, bool res)
On 2016/02/11 16:15:16, mustaq wrote:
> s/res/result/

Will do.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1608:
m_pointerEventManager.sendNodeTransitionEvents(lastNodeUnderMouse.get(),
On 2016/02/11 16:15:16, mustaq wrote:
> I think you can pass the RefPtrWillBeMember params directly here w/o switching
> to raw pointers here.

Yup. I later changed the interface of setNodeTransitionsEvents and forgot to
change this call.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3557: touchInfo.consumed =
m_pointerEventManager.sendTouchPointerEvent(
On 2016/02/11 16:15:16, mustaq wrote:
> Please split the line into two, long "x = y != z" is hard to read.

Sure.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3912:
m_pointerEventManager.clearPreventMouseEventForPointerTypeMouse();
On 2016/02/11 16:15:16, mustaq wrote:
> PE manager's clearPreventMouseEventForPointerTypeMouse() method sounds
> mysterious here since there is no setter method, and it is an internal state
of
> PE manager. I think moving conditionallyEnableMouseEventForPointerTypeMouse()
> into PE manager and hiding the clear...() method is cleaner. Wdyt?

I wanted to totally hid this from EventHandler but I don't think it is possible
at this stage. I still need to keep the
clearPreventMouseEventForPointerTypeMouse visible to EventHandler as there is
one place in this file (in HandleMouseReleaveEvent) that I couldn't move that
logic to PointerEventManager. Do you have any suggestion for that?
I will be able to do that eventually if we clean up all the events here and we
do send all the correct pointercancels then I can move it all to
PointerEventManager.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:82:
WillBeHeapHashMap<int, RefPtrWillBeMember<EventTarget>> m_nodeUnderPointer;
On 2016/02/11 16:15:16, mustaq wrote:
> This surely needs some attention. We can easily have:
>   m_nodeUnderPointer.get(mouse) != EventHandler::m_nodeUnderMouse
> for (legacy) mouse events that don't fire PEs.
> 
> What if we add {get,set}NodeUnderMouse() here to get rid of the state in
> EventHandler?

I had that problem in mind. The catch is that this map is not yet handling mouse
events yet. It is only for touch events and it works for them just fine (i.e.
the setNodeUnderPointer is only called in touch functions). When I add the
tracking of mouse (and mouse like pen events) then I will need to clean up that
piece of code from EventHandler as well.

[mustaq, 2016-02-11 16:51:12]

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html
(right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events.html:200:
testPEParamsSingleTouch(x, y);
On 2016/02/11 16:34:51, Navid Zolghadr wrote:
> On 2016/02/11 16:15:15, mustaq wrote:
> > The enter/leave events on outer divs make the output too long, and
eventPhase
> ==
> > TARGET comparison can't suppress them. Since we are focusing on only the
> > parameters here and below, let's make (x,y) a point on the outer div
("grey")
> > and get rid of the phase comparison.
> 
> But I think this is good to test those events here as well. Since we never
test
> the properties of the pointerenter/leave that we send to the parents of a node
> anywhere else. Do you still want me to strip them out?

Sounds fair. Let's keep them as is.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.h:72:
HashMap<IncomingId, int, WTF::PairHash<int, int>,
WTF::PairHashTraits<PointerEventFactory::UnsignedHash,
PointerEventFactory::UnsignedHash>> m_idMapping;
On 2016/02/11 16:34:51, Navid Zolghadr wrote:
> On 2016/02/11 16:15:16, mustaq wrote:
> > Do you think "PointerEventFactory::" is redundant here?
> 
> It definitely works without. I intentionally added that to make it clear that
> this enum did belong to this class. If you think it doesn't add that much
> clarity I can remove it. Having this do you want me to remove it?

Yes please (I might have thought otherwise for non-private types).

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3912:
m_pointerEventManager.clearPreventMouseEventForPointerTypeMouse();
On 2016/02/11 16:34:52, Navid Zolghadr wrote:
> On 2016/02/11 16:15:16, mustaq wrote:
> > PE manager's clearPreventMouseEventForPointerTypeMouse() method sounds
> > mysterious here since there is no setter method, and it is an internal state
> of
> > PE manager. I think moving
conditionallyEnableMouseEventForPointerTypeMouse()
> > into PE manager and hiding the clear...() method is cleaner. Wdyt?
> 
> I wanted to totally hid this from EventHandler but I don't think it is
possible
> at this stage. I still need to keep the
> clearPreventMouseEventForPointerTypeMouse visible to EventHandler as there is
> one place in this file (in HandleMouseReleaveEvent) that I couldn't move that
> logic to PointerEventManager. Do you have any suggestion for that?
> I will be able to do that eventually if we clean up all the events here and we
> do send all the correct pointercancels then I can move it all to
> PointerEventManager.

Can't you just call
m_pointerEventManager.conditionallyEnableMouseEventForPointerTypeMouse directly?
All it needs is the button-state of the current PlatformMouseEvent.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:82:
WillBeHeapHashMap<int, RefPtrWillBeMember<EventTarget>> m_nodeUnderPointer;
On 2016/02/11 16:34:52, Navid Zolghadr wrote:
> On 2016/02/11 16:15:16, mustaq wrote:
> > This surely needs some attention. We can easily have:
> >   m_nodeUnderPointer.get(mouse) != EventHandler::m_nodeUnderMouse
> > for (legacy) mouse events that don't fire PEs.
> > 
> > What if we add {get,set}NodeUnderMouse() here to get rid of the state in
> > EventHandler?
> 
> I had that problem in mind. The catch is that this map is not yet handling
mouse
> events yet. It is only for touch events and it works for them just fine (i.e.
> the setNodeUnderPointer is only called in touch functions). When I add the
> tracking of mouse (and mouse like pen events) then I will need to clean up
that
> piece of code from EventHandler as well.

Sorry didn't get it: for touch, we are currently sending mouse transition
events, right? Are we not updating m_nodeUnderMouse?

[Navid Zolghadr, 2016-02-11 17:47:49]

I applied the comments. ptal.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.h:72:
HashMap<IncomingId, int, WTF::PairHash<int, int>,
WTF::PairHashTraits<PointerEventFactory::UnsignedHash,
PointerEventFactory::UnsignedHash>> m_idMapping;
On 2016/02/11 16:51:12, mustaq wrote:
> On 2016/02/11 16:34:51, Navid Zolghadr wrote:
> > On 2016/02/11 16:15:16, mustaq wrote:
> > > Do you think "PointerEventFactory::" is redundant here?
> > 
> > It definitely works without. I intentionally added that to make it clear
that
> > this enum did belong to this class. If you think it doesn't add that much
> > clarity I can remove it. Having this do you want me to remove it?
> 
> Yes please (I might have thought otherwise for non-private types).

Done.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:3912:
m_pointerEventManager.clearPreventMouseEventForPointerTypeMouse();
On 2016/02/11 16:51:12, mustaq wrote:
> On 2016/02/11 16:34:52, Navid Zolghadr wrote:
> > On 2016/02/11 16:15:16, mustaq wrote:
> > > PE manager's clearPreventMouseEventForPointerTypeMouse() method sounds
> > > mysterious here since there is no setter method, and it is an internal
state
> > of
> > > PE manager. I think moving
> conditionallyEnableMouseEventForPointerTypeMouse()
> > > into PE manager and hiding the clear...() method is cleaner. Wdyt?
> > 
> > I wanted to totally hid this from EventHandler but I don't think it is
> possible
> > at this stage. I still need to keep the
> > clearPreventMouseEventForPointerTypeMouse visible to EventHandler as there
is
> > one place in this file (in HandleMouseReleaveEvent) that I couldn't move
that
> > logic to PointerEventManager. Do you have any suggestion for that?
> > I will be able to do that eventually if we clean up all the events here and
we
> > do send all the correct pointercancels then I can move it all to
> > PointerEventManager.
> 
> Can't you just call
> m_pointerEventManager.conditionallyEnableMouseEventForPointerTypeMouse
directly?
> All it needs is the button-state of the current PlatformMouseEvent.

Sure. I will move that function to PointerEventManager with a different
parameter to take care of that.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.h (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.h:82:
WillBeHeapHashMap<int, RefPtrWillBeMember<EventTarget>> m_nodeUnderPointer;
On 2016/02/11 16:51:12, mustaq wrote:
> On 2016/02/11 16:34:52, Navid Zolghadr wrote:
> > On 2016/02/11 16:15:16, mustaq wrote:
> > > This surely needs some attention. We can easily have:
> > >   m_nodeUnderPointer.get(mouse) != EventHandler::m_nodeUnderMouse
> > > for (legacy) mouse events that don't fire PEs.
> > > 
> > > What if we add {get,set}NodeUnderMouse() here to get rid of the state in
> > > EventHandler?
> > 
> > I had that problem in mind. The catch is that this map is not yet handling
> mouse
> > events yet. It is only for touch events and it works for them just fine
(i.e.
> > the setNodeUnderPointer is only called in touch functions). When I add the
> > tracking of mouse (and mouse like pen events) then I will need to clean up
> that
> > piece of code from EventHandler as well.
> 
> Sorry didn't get it: for touch, we are currently sending mouse transition
> events, right? Are we not updating m_nodeUnderMouse?

Not through PointerEventManager class. EventHandler takes care of those using
m_nodeUnderMouse. I will add a comment explaining this field for now.

[mustaq, 2016-02-11 19:11:31]

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1325:
PlatformEvent::NoModifiers);
I added it before chorded button support, should have checked mouseEvent.buttons
originally, sorry. Please pass the correct value here value.

Please add a test in mouse-pointer-chorded-buttons.html to catch the original
bug:
L-down-preventDefault, then M-down, then L-up, then M-up should start firing
compat mouse events after step 4, not 3.

[Navid Zolghadr, 2016-02-11 19:17:24]

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1325:
PlatformEvent::NoModifiers);
On 2016/02/11 19:11:31, mustaq wrote:
> I added it before chorded button support, should have checked
mouseEvent.buttons
> originally, sorry. Please pass the correct value here value.
> 
> Please add a test in mouse-pointer-chorded-buttons.html to catch the original
> bug:
> L-down-preventDefault, then M-down, then L-up, then M-up should start firing
> compat mouse events after step 4, not 3.

I will file a bug for that one and fix that in another CL as this CL doesn't
change the behavior of this at all.

[mustaq, 2016-02-11 20:37:29]

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

[mustaq, 2016-02-11 20:37:29]

lgtm with nit.

Adding Ian for owner's approval.

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1608:
m_pointerEventManager.sendNodeTransitionEvents(lastNodeUnderMouse.get(),
On 2016/02/11 16:34:51, Navid Zolghadr wrote:
> On 2016/02/11 16:15:16, mustaq wrote:
> > I think you can pass the RefPtrWillBeMember params directly here w/o
switching
> > to raw pointers here.
> 
> Yup. I later changed the interface of setNodeTransitionsEvents and forgot to
> change this call.

Same applies for enteredNode.

[Ian Vollick, 2016-02-12 15:01:17]

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events-expected.txt
(right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events-expected.txt:42:
FAIL receivedPEsAtTarget[2].buttons should be 1. Threw exception TypeError:
Cannot read property 'buttons' of undefined
If this test is only valid in LayoutTests/virtual/pointerevents/, then you can
put the test itself there rather than having a failing expectation here.

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEvent.h (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEvent.h:28: int pointerId() const {
return m_pointerId; }
Did we change this so that it could be handled by the hash fn?

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:5: #include
"core/events/PointerEventFactory.h"
Although the net result seems clearer, this patch is made a little confusing by
moving PointerEventManager to PointerEventFactory as well as introducing a new
class named PointerEventManager which does different stuff.

Is this a correct interpretation of what's happening here:
 * Since the old PointerEventManager was behaving mostly as a factory, you've
made it official and added some more factory stuff.
 * You've refactored some event management stuff out of EventHandler into a new
PointerEventManager class that it aggregates?

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:137:
PassRefPtrWillBeRawPtr<PointerEvent> pointerevent,
nit: camel case.

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:1: // Copyright 2015
The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:20: will have a
unique id amongst all pointer events as per pointer events'
nit: I'd use // to be consistent with the license comments above. here and
elsewhere.

[mustaq, 2016-02-12 15:17:40]

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:5: #include
"core/events/PointerEventFactory.h"
On 2016/02/12 15:01:17, vollick wrote:
> Although the net result seems clearer, this patch is made a little confusing
by
> moving PointerEventManager to PointerEventFactory as well as introducing a new
> class named PointerEventManager which does different stuff.
> 
> Is this a correct interpretation of what's happening here:
>  * Since the old PointerEventManager was behaving mostly as a factory, you've
> made it official and added some more factory stuff.
>  * You've refactored some event management stuff out of EventHandler into a
new
> PointerEventManager class that it aggregates?

Yes, mainly to isolate:
  (a) PointerEvent states & dispatches, and
  (b) PointerEvent creation & id-management
from each other and also from EventHandler.

Navid, please mention this in the CL description.

[Navid Zolghadr, 2016-02-12 15:29:49]

Description was changed from

==========
Send node transition events for touch events

Send pointerover/enter when touch starts on an element
and send pointerout/leave when touch ends.

BUG=583376
==========

to

==========
Send node transition events for touch events

Send pointerover/enter when touch starts on an element
and send pointerout/leave when touch ends.

Also refactoring the code to isolate:
  (a) PointerEvent states & dispatches into PointerEventManager
  (b) PointerEvent creation & id-management into PointerEventFactory
from each other and also from EventHandler.


BUG=583376
==========

[Navid Zolghadr, 2016-02-12 16:30:48]

ptal

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1670073004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1608:
m_pointerEventManager.sendNodeTransitionEvents(lastNodeUnderMouse.get(),
On 2016/02/11 20:37:29, mustaq wrote:
> On 2016/02/11 16:34:51, Navid Zolghadr wrote:
> > On 2016/02/11 16:15:16, mustaq wrote:
> > > I think you can pass the RefPtrWillBeMember params directly here w/o
> switching
> > > to raw pointers here.
> > 
> > Yup. I later changed the interface of setNodeTransitionsEvents and forgot to
> > change this call.
> 
> Same applies for enteredNode.

Done.

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events-expected.txt
(right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events-expected.txt:42:
FAIL receivedPEsAtTarget[2].buttons should be 1. Threw exception TypeError:
Cannot read property 'buttons' of undefined
On 2016/02/12 15:01:17, vollick wrote:
> If this test is only valid in LayoutTests/virtual/pointerevents/, then you can
> put the test itself there rather than having a failing expectation here.

Done.

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEvent.h (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEvent.h:28: int pointerId() const {
return m_pointerId; }
On 2016/02/12 15:01:17, vollick wrote:
> Did we change this so that it could be handled by the hash fn?

No. The spec is indicating the range for this field is int. It should have been
int from the beginning. Long type was incorrect.

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:5: #include
"core/events/PointerEventFactory.h"
On 2016/02/12 15:01:17, vollick wrote:
> Although the net result seems clearer, this patch is made a little confusing
by
> moving PointerEventManager to PointerEventFactory as well as introducing a new
> class named PointerEventManager which does different stuff.
> 
> Is this a correct interpretation of what's happening here:
>  * Since the old PointerEventManager was behaving mostly as a factory, you've
> made it official and added some more factory stuff.
>  * You've refactored some event management stuff out of EventHandler into a
new
> PointerEventManager class that it aggregates?

Done

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:137:
PassRefPtrWillBeRawPtr<PointerEvent> pointerevent,
On 2016/02/12 15:01:17, vollick wrote:
> nit: camel case.

Done

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/02/12 15:01:17, vollick wrote:
> 2016

Done.

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:20: will have a
unique id amongst all pointer events as per pointer events'
On 2016/02/12 15:01:17, vollick wrote:
> nit: I'd use // to be consistent with the license comments above. here and
> elsewhere.

Done.

[Ian Vollick, 2016-02-12 16:41:32]

Thanks.

lgtm w/ nit.

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:56: */
nit: //.

[Navid Zolghadr, 2016-02-12 16:43:55]

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events-expected.txt
(right):

https://codereview.chromium.org/1670073004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointer-events-expected.txt:42:
FAIL receivedPEsAtTarget[2].buttons should be 1. Threw exception TypeError:
Cannot read property 'buttons' of undefined
On 2016/02/12 16:30:47, Navid Zolghadr wrote:
> On 2016/02/12 15:01:17, vollick wrote:
> > If this test is only valid in LayoutTests/virtual/pointerevents/, then you
can
> > put the test itself there rather than having a failing expectation here.
> 
> Done.

One quick question regarding this. I found some examples of this that they moved
the .html file to virtual folder. However, now that I did that it expects
-expected.png file as well and it fails the bots. Do you know how to get that
fixed?

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:56: */
On 2016/02/12 16:41:32, vollick wrote:
> nit: //.

Will fix it.

[Ian Vollick, 2016-02-12 17:01:22]

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-pointer-events.html
(right):

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-pointer-events.html:2:
<script src="../../../resources/js-test.js"></script>
I suspect that the need for a png is that this relative path is now wrong.

[mustaq, 2016-02-12 17:08:40]

Description was changed from

==========
Send node transition events for touch events

Send pointerover/enter when touch starts on an element
and send pointerout/leave when touch ends.

Also refactoring the code to isolate:
  (a) PointerEvent states & dispatches into PointerEventManager
  (b) PointerEvent creation & id-management into PointerEventFactory
from each other and also from EventHandler.


BUG=583376
==========

to

==========
Send node transition events for touch events

This CL adds firing of pointerover/enter when touch
starts on an element, and pointerout/leave when touch
ends.

This CL also includes some refactoring around
our PointerEvents code. We isolated:
- PE handling details off from EventHandler,
- PE states & dispatches into PointerEventManager, and
- PE creation & id-management into PointerEventFactory.

BUG=583376
==========

[Navid Zolghadr, 2016-02-12 18:41:18]

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-pointer-events.html
(right):

https://codereview.chromium.org/1670073004/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/touch-pointer-events.html:2:
<script src="../../../resources/js-test.js"></script>
On 2016/02/12 17:01:21, vollick wrote:
> I suspect that the need for a png is that this relative path is now wrong.

Yup. That did the trick. Thanks for the catch.

[Navid Zolghadr, 2016-02-12 20:07:58]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-02-12 20:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/1670073004/#ps160001
(title: "Fixing the comment")

[commit-bot: I haz the power, 2016-02-12 20:10:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1670073004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1670073004/160001

[commit-bot: I haz the power, 2016-02-12 21:43:21]

Description was changed from

==========
Send node transition events for touch events

This CL adds firing of pointerover/enter when touch
starts on an element, and pointerout/leave when touch
ends.

This CL also includes some refactoring around
our PointerEvents code. We isolated:
- PE handling details off from EventHandler,
- PE states & dispatches into PointerEventManager, and
- PE creation & id-management into PointerEventFactory.

BUG=583376
==========

to

==========
Send node transition events for touch events

This CL adds firing of pointerover/enter when touch
starts on an element, and pointerout/leave when touch
ends.

This CL also includes some refactoring around
our PointerEvents code. We isolated:
- PE handling details off from EventHandler,
- PE states & dispatches into PointerEventManager, and
- PE creation & id-management into PointerEventFactory.

BUG=583376
==========

[commit-bot: I haz the power, 2016-02-12 21:43:22]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-16 22:45:17]

Description was changed from

==========
Send node transition events for touch events

This CL adds firing of pointerover/enter when touch
starts on an element, and pointerout/leave when touch
ends.

This CL also includes some refactoring around
our PointerEvents code. We isolated:
- PE handling details off from EventHandler,
- PE states & dispatches into PointerEventManager, and
- PE creation & id-management into PointerEventFactory.

BUG=583376
==========

to

==========
Send node transition events for touch events

This CL adds firing of pointerover/enter when touch
starts on an element, and pointerout/leave when touch
ends.

This CL also includes some refactoring around
our PointerEvents code. We isolated:
- PE handling details off from EventHandler,
- PE states & dispatches into PointerEventManager, and
- PE creation & id-management into PointerEventFactory.

BUG=583376

Committed: https://crrev.com/99d107b0e05edcd98f74b8d3a889e5151519e14d
Cr-Commit-Position: refs/heads/master@{#375261}
==========

[commit-bot: I haz the power, 2016-02-16 22:45:18]

Patchset 9 (id:??) landed as
https://crrev.com/99d107b0e05edcd98f74b8d3a889e5151519e14d
Cr-Commit-Position: refs/heads/master@{#375261}