Fire pointermoves for mousemove events.

Fire pointermoves for mousemove events.

This CL adds pointermove firing for non-synthetic mousemove events.
Synthetic mousemoves, e.g. from gesture taps, doesn't need pointermoves
because PEs have been fired earlier from TEs.

This CL also include a bit of cleanup: EventHandler::dispatchMouseEvent
returned false when input event has been handled. This is counter
intuitive---all callers negated the returned boolean. This CL flips the
return value to make the method in-line with handleTouchEvent(),
handleMouseFocus().

BUG=476571, 548374
Committed: https://crrev.com/d904a344ffe1e2e3f5126df826284b7c3c70fdab
Cr-Commit-Position: refs/heads/master@{#357856}

[mustaq, 2015-10-28 20:18:02]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-28 20:18:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425053002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425053002/1

[mustaq, 2015-10-28 21:02:20]

Description was changed from

==========
Fire pointermoves for mousemove events.

This CL add pointermove firing for non-synthetic mousemove events.

This also include a bit of cleanup: EventHandler::dispatchMouseEvent
returned false when input event has been handled. This is counter
intuitive---all callers negated the returned boolean. This CL flips the
return value to make the method in-line with handleTouchEvent(),
handleMouseFocus().

BUG=476571, 548374
==========

to

==========
Fire pointermoves for mousemove events.

This CL adds pointermove firing for non-synthetic mousemove events.
Synthetic mousemoves, e.g. from gesture taps, doesn't need pointermoves
because PEs have been fired earlier from TEs.

This CL also include a bit of cleanup: EventHandler::dispatchMouseEvent
returned false when input event has been handled. This is counter
intuitive---all callers negated the returned boolean. This CL flips the
return value to make the method in-line with handleTouchEvent(),
handleMouseFocus().

BUG=476571, 548374
==========

[mustaq, 2015-10-28 21:03:10]

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

[mustaq, 2015-10-28 21:03:11]

ptal

[Rick Byers, 2015-10-30 17:21:29]

Just a couple small suggestions

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

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1730: bool
EventHandler::dispatchMouseEvent(const AtomicString& eventType, Node*
targetNode, int clickCount, const PlatformMouseEvent& mouseEvent)
So the pattern you're going for is that all EventHandler methods (both dispatch*
and handle*) methods will return true when the event is canceled ("handled"),
right?  To make that consistent can you also fix dispatchDragSrcEvent (which I
think is the only other backwards method remaining)?

Note that elsewhere in DOM event handling, dispatch* methods all return false if
the event was canceled ("stop dispatch"), matching the web-exposed API pattern:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent.

Since this is a regular source of confusion, perhaps we should just change
EventHandler to use an enum instead of a boolean in the first place?  Could be a
good noogler refactoring bug.

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1736: bool
EventHandler::dispatchPointerAndMouseEvent(const AtomicString& mouseEventType,
Node* targetNode, int clickCount, const PlatformMouseEvent& mouseEvent)
In the spirit of working towards pulling all mouse event handling logic out
(such that it's really just legacy compat code that sits on top of pointer
events), perhaps this logic should be folded into
dispatchPointerEventForMouseEvent?

I.e. dispatching the necessary mouse/touch events can always be thought of as an
implementation detail of dispatching pointer events (or handling gesture
events).

We could probably take a 2nd step here pretty soon too - rather than have
EventHandler dispatch the mouse event at all, move all the mouse event logic to
a separate class which just hooks in as a post-pointer-event dispatch hook
somewhere.  Ideally callers of EventHandler methods would never need to think
about DOM mouse events explicitly (eg. knowing when they are and are not
suppressed by a canceled pointer event).

WDYT?

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:299: int clickCount, const
PlatformMouseEvent&);
PlatformMouseEvent contains a clickCount, can you eliminate the clickCount
argument?

[mustaq, 2015-11-03 20:44:30]

ptal.

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

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1730: bool
EventHandler::dispatchMouseEvent(const AtomicString& eventType, Node*
targetNode, int clickCount, const PlatformMouseEvent& mouseEvent)
On 2015/10/30 17:21:29, Rick Byers wrote:
> So the pattern you're going for is that all EventHandler methods (both
dispatch*
> and handle*) methods will return true when the event is canceled ("handled"),
> right?  To make that consistent can you also fix dispatchDragSrcEvent (which I
> think is the only other backwards method remaining)?
> 
> Note that elsewhere in DOM event handling, dispatch* methods all return false
if
> the event was canceled ("stop dispatch"), matching the web-exposed API
pattern:
> https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent.
> 
> Since this is a regular source of confusion, perhaps we should just change
> EventHandler to use an enum instead of a boolean in the first place?  Could be
a
> good noogler refactoring bug.

Done. You are right: dispatchDragSrcEvent is the only remaining dispatch*() that
needs to be fixed. I originally thought the handle*() methods are mixed up but
it seems they are not.

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1736: bool
EventHandler::dispatchPointerAndMouseEvent(const AtomicString& mouseEventType,
Node* targetNode, int clickCount, const PlatformMouseEvent& mouseEvent)
On 2015/10/30 17:21:29, Rick Byers wrote:
> In the spirit of working towards pulling all mouse event handling logic out
> (such that it's really just legacy compat code that sits on top of pointer
> events), perhaps this logic should be folded into
> dispatchPointerEventForMouseEvent?
> 
> I.e. dispatching the necessary mouse/touch events can always be thought of as
an
> implementation detail of dispatching pointer events (or handling gesture
> events).
> 
> We could probably take a 2nd step here pretty soon too - rather than have
> EventHandler dispatch the mouse event at all, move all the mouse event logic
to
> a separate class which just hooks in as a post-pointer-event dispatch hook
> somewhere.  Ideally callers of EventHandler methods would never need to think
> about DOM mouse events explicitly (eg. knowing when they are and are not
> suppressed by a canceled pointer event).
> 
> WDYT?

I think the confusion is caused by the similar looking names of the two methods.
This method updates mouse target before dispatching events, while
dispatchPointerEventForMouseEvent is a straightforward "state-ignoring" dispatch
(has direct callers for mouse/pointer entry/exit events).

Hope the new names address your concerns.

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:299: int clickCount, const
PlatformMouseEvent&);
On 2015/10/30 17:21:29, Rick Byers wrote:
> PlatformMouseEvent contains a clickCount, can you eliminate the clickCount
> argument?

I think PlatformMouse event doesn't maintain the clickcount needed by UI
MouseEvent, specially on mouse release. EventHandler caches the last count in
m_clickCount, I guess this is the only reason.

[Rick Byers, 2015-11-03 20:59:56]

LGTM with nit

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

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1736: bool
EventHandler::dispatchPointerAndMouseEvent(const AtomicString& mouseEventType,
Node* targetNode, int clickCount, const PlatformMouseEvent& mouseEvent)
On 2015/11/03 20:44:30, mustaq wrote:
> On 2015/10/30 17:21:29, Rick Byers wrote:
> > In the spirit of working towards pulling all mouse event handling logic out
> > (such that it's really just legacy compat code that sits on top of pointer
> > events), perhaps this logic should be folded into
> > dispatchPointerEventForMouseEvent?
> > 
> > I.e. dispatching the necessary mouse/touch events can always be thought of
as
> an
> > implementation detail of dispatching pointer events (or handling gesture
> > events).
> > 
> > We could probably take a 2nd step here pretty soon too - rather than have
> > EventHandler dispatch the mouse event at all, move all the mouse event logic
> to
> > a separate class which just hooks in as a post-pointer-event dispatch hook
> > somewhere.  Ideally callers of EventHandler methods would never need to
think
> > about DOM mouse events explicitly (eg. knowing when they are and are not
> > suppressed by a canceled pointer event).
> > 
> > WDYT?
> 
> I think the confusion is caused by the similar looking names of the two
methods.
> This method updates mouse target before dispatching events, while
> dispatchPointerEventForMouseEvent is a straightforward "state-ignoring"
dispatch
> (has direct callers for mouse/pointer entry/exit events).
> 
> Hope the new names address your concerns.

I'd still like to untangle the mouse event support so that it's ultimately built
on top of pointer events (as opposed to something EventHandler code has to
reason about explicitly).  As a really simple example, ultimately there should
be no need for "pointerEventNameForMouseEventName" because everything goes the
other direction (instead we'd need mouseEventNameForPointerEventName).

But more of this can come later as the PE support matures.  This is fine for
now, thanks for the clarifications.

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:299: int clickCount, const
PlatformMouseEvent&);
On 2015/11/03 20:44:30, mustaq wrote:
> On 2015/10/30 17:21:29, Rick Byers wrote:
> > PlatformMouseEvent contains a clickCount, can you eliminate the clickCount
> > argument?
> 
> I think PlatformMouse event doesn't maintain the clickcount needed by UI
> MouseEvent, specially on mouse release. EventHandler caches the last count in
> m_clickCount, I guess this is the only reason.

Ok.  Seems like a wart to me that might not be too hard to clean up somehow. 
Care to add a TODO?  At the very least the different semantics should be
documented in comments / argument names (maybe there's a distinction between the
pre-event-click-count and the post-event-click-count?).

[mustaq, 2015-11-03 21:37:34]

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

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1736: bool
EventHandler::dispatchPointerAndMouseEvent(const AtomicString& mouseEventType,
Node* targetNode, int clickCount, const PlatformMouseEvent& mouseEvent)
On 2015/11/03 20:59:56, Rick Byers wrote:
> On 2015/11/03 20:44:30, mustaq wrote:
> > On 2015/10/30 17:21:29, Rick Byers wrote:
> > > In the spirit of working towards pulling all mouse event handling logic
out
> > > (such that it's really just legacy compat code that sits on top of pointer
> > > events), perhaps this logic should be folded into
> > > dispatchPointerEventForMouseEvent?
> > > 
> > > I.e. dispatching the necessary mouse/touch events can always be thought of
> as
> > an
> > > implementation detail of dispatching pointer events (or handling gesture
> > > events).
> > > 
> > > We could probably take a 2nd step here pretty soon too - rather than have
> > > EventHandler dispatch the mouse event at all, move all the mouse event
logic
> > to
> > > a separate class which just hooks in as a post-pointer-event dispatch hook
> > > somewhere.  Ideally callers of EventHandler methods would never need to
> think
> > > about DOM mouse events explicitly (eg. knowing when they are and are not
> > > suppressed by a canceled pointer event).
> > > 
> > > WDYT?
> > 
> > I think the confusion is caused by the similar looking names of the two
> methods.
> > This method updates mouse target before dispatching events, while
> > dispatchPointerEventForMouseEvent is a straightforward "state-ignoring"
> dispatch
> > (has direct callers for mouse/pointer entry/exit events).
> > 
> > Hope the new names address your concerns.
> 
> I'd still like to untangle the mouse event support so that it's ultimately
built
> on top of pointer events (as opposed to something EventHandler code has to
> reason about explicitly).  As a really simple example, ultimately there should
> be no need for "pointerEventNameForMouseEventName" because everything goes the
> other direction (instead we'd need mouseEventNameForPointerEventName).
> 
> But more of this can come later as the PE support matures.  This is fine for
> now, thanks for the clarifications. 

Yes, your PE-driving-ME-dispatch suggestion made perfect sense to me except that
I held it back as the next change here. The main bottleneck in this direction
would be pointer/mouse state updates through synthetic mouse events.

Added a TODO for now.

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1425053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.h:299: int clickCount, const
PlatformMouseEvent&);
On 2015/11/03 20:59:56, Rick Byers wrote:
> On 2015/11/03 20:44:30, mustaq wrote:
> > On 2015/10/30 17:21:29, Rick Byers wrote:
> > > PlatformMouseEvent contains a clickCount, can you eliminate the clickCount
> > > argument?
> > 
> > I think PlatformMouse event doesn't maintain the clickcount needed by UI
> > MouseEvent, specially on mouse release. EventHandler caches the last count
in
> > m_clickCount, I guess this is the only reason.
> 
> Ok.  Seems like a wart to me that might not be too hard to clean up somehow. 
> Care to add a TODO?  At the very least the different semantics should be
> documented in comments / argument names (maybe there's a distinction between
the
> pre-event-click-count and the post-event-click-count?).

Added a TODO.

[mustaq, 2015-11-03 21:38:08]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2015-11-03 21:38:08]

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/1425053002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-11-03 21:40:56]

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

[commit-bot: I haz the power, 2015-11-03 22:18:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-03 22:18:11]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mustaq, 2015-11-04 15:27:58]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2015-11-04 15:28:39]

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

[commit-bot: I haz the power, 2015-11-04 17:34:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 17:34:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mustaq, 2015-11-04 17:44:32]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2015-11-04 17:44:32]

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/1425053002/#ps80001
(title: "TestExpectations")

[commit-bot: I haz the power, 2015-11-04 17:44:59]

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

[mustaq, 2015-11-04 17:51:09]

On 2015/11/04 17:34:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

The win_chromium_rel_ng bug hit again, this time the main test suite! Updated
TestExpectations.

[commit-bot: I haz the power, 2015-11-04 18:46:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-04 18:47:16]

Patchset 5 (id:??) landed as
https://crrev.com/d904a344ffe1e2e3f5126df826284b7c3c70fdab
Cr-Commit-Position: refs/heads/master@{#357856}