Set width=height=1 of mouse like pointer events

Set width=height=1 of mouse like pointer events

Right now these devices don't report any geometry
and there is no plumbing to set these values in
the lower levels. So for now we just set them in
PointerEventFactory.

BUG=608459
Committed: https://crrev.com/69ca22269406e17f2e67caaa5e01167736ccc86b
Cr-Commit-Position: refs/heads/master@{#393829}

[Navid Zolghadr, 2016-05-11 19:13:08]

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

[mustaq, 2016-05-11 20:42:58]

lgtm modulo nits.

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (left):

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:40:
RETURN_CORRESPONDING_PE_NAME(enter);
Let's leave them as is. There's no harm in keeping this function complete, and
we don't want to close the way to easily create "pointerleave" from "mouseleave"
(e.g. for stylus-off-the-digitizer case).

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

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:105: } else { //
Only when pointerEventName == EventTypeNames::pointermove
The comment is not 100% correct: we are actually hacking
mosueEventType="mousedown" to ultimately create transition events, right?

Let's remove the comment.

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:111: // Set
width/height to 1 because we don't get any information from lower
"...because it matches Edge (and supported by the spec)."

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1725:
m_pointerEventManager.sendMouseAndPossiblyPointerBoundaryEvents(
Why is this? "Transition events" more accurately reflects enter/leave/over/out
behaviour because these events can fire even w/o the pointer crossing the
boundary.

We can get rid of the "Node" from "NodeTransitionEvent" though.

[Navid Zolghadr, 2016-05-11 20:50:20]

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (left):

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:40:
RETURN_CORRESPONDING_PE_NAME(enter);
On 2016/05/11 20:42:57, mustaq wrote:
> Let's leave them as is. There's no harm in keeping this function complete, and
> we don't want to close the way to easily create "pointerleave" from
"mouseleave"
> (e.g. for stylus-off-the-digitizer case).

Sure. I will add a DCHECK separately in the create function to only allow
move/up/down. Is that okay?

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

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:105: } else { //
Only when pointerEventName == EventTypeNames::pointermove
On 2016/05/11 20:42:57, mustaq wrote:
> The comment is not 100% correct: we are actually hacking
> mosueEventType="mousedown" to ultimately create transition events, right?
> 
> Let's remove the comment.

No. The boundary events use another create function which clone the pointerevent
properties and only change the type and target.

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:111: // Set
width/height to 1 because we don't get any information from lower
On 2016/05/11 20:42:57, mustaq wrote:
> "...because it matches Edge (and supported by the spec)."

Sure.

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1725:
m_pointerEventManager.sendMouseAndPossiblyPointerBoundaryEvents(
On 2016/05/11 20:42:57, mustaq wrote:
> Why is this? "Transition events" more accurately reflects enter/leave/over/out
> behaviour because these events can fire even w/o the pointer crossing the
> boundary.
> 
> We can get rid of the "Node" from "NodeTransitionEvent" though.

I was trying to follow Rick's recent email to the pewg. It seems that there is
already a meaning for transition events.
https://drafts.csswg.org/css-transitions/#transition-events

Having this in mind, do you want me to keep it as transition events?

[mustaq, 2016-05-12 14:03:09]

Still LGTM.

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

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:105: } else { //
Only when pointerEventName == EventTypeNames::pointermove
On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> On 2016/05/11 20:42:57, mustaq wrote:
> > The comment is not 100% correct: we are actually hacking
> > mosueEventType="mousedown" to ultimately create transition events, right?
> > 
> > Let's remove the comment.
> 
> No. The boundary events use another create function which clone the
pointerevent
> properties and only change the type and target.

The create() call in sendMouseAndPossiblyPointerNodeTransitionEvents looks
confusing then. Looks like there is no easy way to avoid the create() call
there, right? If I am right, please rename the created event there to "dummy..."
at least.

A related thing: please update the doc for processPendingPointerCapture() to
somehow say that it doesn't care about the button state. It looks confusing from
spec perspective that the dummy events created here with button=none may not
release capture.

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1725:
m_pointerEventManager.sendMouseAndPossiblyPointerBoundaryEvents(
On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> On 2016/05/11 20:42:57, mustaq wrote:
> > Why is this? "Transition events" more accurately reflects
enter/leave/over/out
> > behaviour because these events can fire even w/o the pointer crossing the
> > boundary.
> > 
> > We can get rid of the "Node" from "NodeTransitionEvent" though.
> 
> I was trying to follow Rick's recent email to the pewg. It seems that there is
> already a meaning for transition events.
> https://drafts.csswg.org/css-transitions/#transition-events
> 
> Having this in mind, do you want me to keep it as transition events?

Missed the github comment, sorry. Your renaming looks good then.

[Navid Zolghadr, 2016-05-12 14:36:43]

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

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:105: } else { //
Only when pointerEventName == EventTypeNames::pointermove
On 2016/05/12 14:03:09, mustaq wrote:
> On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> > On 2016/05/11 20:42:57, mustaq wrote:
> > > The comment is not 100% correct: we are actually hacking
> > > mosueEventType="mousedown" to ultimately create transition events, right?
> > > 
> > > Let's remove the comment.
> > 
> > No. The boundary events use another create function which clone the
> pointerevent
> > properties and only change the type and target.
> 
> The create() call in sendMouseAndPossiblyPointerNodeTransitionEvents looks
> confusing then. Looks like there is no easy way to avoid the create() call
> there, right? If I am right, please rename the created event there to
"dummy..."
> at least.
> 
> A related thing: please update the doc for processPendingPointerCapture() to
> somehow say that it doesn't care about the button state. It looks confusing
from
> spec perspective that the dummy events created here with button=none may not
> release capture.

I'm not quite sure about the source of confusion here. Can you explain a little
bit more?

Sure. I will rename the variable to "dummyPointerEvent".

Regarding processPendingPointerCapture, it cares about neither button nor
buttons. The processPendingPointerCapture is defined in the spec here

https://w3c.github.io/pointerevents/#process-pending-pointer-capture

and this method was trying to implement exactly that. Whether a release will
happen or not depending on the explicit release call or buttons value that is
something else and when it happens it only sets the "pending pointer capture
target" variable. On the other hand, when processPendingPointerCapture is called
it is processing the assignment of "pointer capture target" from "pending
pointer capture target" and sends the got/lostpointercapture. Is this okay?
Maybe I can put a link to the spec section as comment before that function. Do
you want me to do that or will it be too tight to the spec?

[mustaq, 2016-05-12 15:26:59]

On 2016/05/12 14:36:43, Navid Zolghadr wrote:
>
https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> 
>
https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/PointerEventFactory.cpp:105: } else { //
> Only when pointerEventName == EventTypeNames::pointermove
> On 2016/05/12 14:03:09, mustaq wrote:
> > On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> > > On 2016/05/11 20:42:57, mustaq wrote:
> > > > The comment is not 100% correct: we are actually hacking
> > > > mosueEventType="mousedown" to ultimately create transition events,
right?
> > > > 
> > > > Let's remove the comment.
> > > 
> > > No. The boundary events use another create function which clone the
> > pointerevent
> > > properties and only change the type and target.
> > 
> > The create() call in sendMouseAndPossiblyPointerNodeTransitionEvents looks
> > confusing then. Looks like there is no easy way to avoid the create() call
> > there, right? If I am right, please rename the created event there to
> "dummy..."
> > at least.
> > 
> > A related thing: please update the doc for processPendingPointerCapture() to
> > somehow say that it doesn't care about the button state. It looks confusing
> from
> > spec perspective that the dummy events created here with button=none may not
> > release capture.
> 
> I'm not quite sure about the source of confusion here. Can you explain a
little
> bit more?
> 
> Sure. I will rename the variable to "dummyPointerEvent".
> 
> Regarding processPendingPointerCapture, it cares about neither button nor
> buttons. The processPendingPointerCapture is defined in the spec here
> 
> https://w3c.github.io/pointerevents/#process-pending-pointer-capture
> 
> and this method was trying to implement exactly that. Whether a release will
> happen or not depending on the explicit release call or buttons value that is
> something else and when it happens it only sets the "pending pointer capture
> target" variable. On the other hand, when processPendingPointerCapture is
called
> it is processing the assignment of "pointer capture target" from "pending
> pointer capture target" and sends the got/lostpointercapture. Is this okay?
> Maybe I can put a link to the spec section as comment before that function. Do
> you want me to do that or will it be too tight to the spec?

Looks like I forgot the 1:1 mapping of this function to the spec section. To
avoid
this in future, just add what you mentioned above, perhaps in short like this:

Processes the assignment of "pointer capture target" from "pending pointer
capture
target" and sends the got/lostpointercapture events, as per the spec:
  [link].
Returns whether the pointer capture is changed. When pointer capture is changed,
this function will take care of boundary events.

[Navid Zolghadr, 2016-05-12 20:13:19]

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

[Navid Zolghadr, 2016-05-12 20:13:21]

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (left):

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:40:
RETURN_CORRESPONDING_PE_NAME(enter);
On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> On 2016/05/11 20:42:57, mustaq wrote:
> > Let's leave them as is. There's no harm in keeping this function complete,
and
> > we don't want to close the way to easily create "pointerleave" from
> "mouseleave"
> > (e.g. for stylus-off-the-digitizer case).
> 
> Sure. I will add a DCHECK separately in the create function to only allow
> move/up/down. Is that okay?

Done.

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

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:105: } else { //
Only when pointerEventName == EventTypeNames::pointermove
On 2016/05/12 14:36:43, Navid Zolghadr wrote:
> On 2016/05/12 14:03:09, mustaq wrote:
> > On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> > > On 2016/05/11 20:42:57, mustaq wrote:
> > > > The comment is not 100% correct: we are actually hacking
> > > > mosueEventType="mousedown" to ultimately create transition events,
right?
> > > > 
> > > > Let's remove the comment.
> > > 
> > > No. The boundary events use another create function which clone the
> > pointerevent
> > > properties and only change the type and target.
> > 
> > The create() call in sendMouseAndPossiblyPointerNodeTransitionEvents looks
> > confusing then. Looks like there is no easy way to avoid the create() call
> > there, right? If I am right, please rename the created event there to
> "dummy..."
> > at least.
> > 
> > A related thing: please update the doc for processPendingPointerCapture() to
> > somehow say that it doesn't care about the button state. It looks confusing
> from
> > spec perspective that the dummy events created here with button=none may not
> > release capture.
> 
> I'm not quite sure about the source of confusion here. Can you explain a
little
> bit more?
> 
> Sure. I will rename the variable to "dummyPointerEvent".
> 
> Regarding processPendingPointerCapture, it cares about neither button nor
> buttons. The processPendingPointerCapture is defined in the spec here
> 
> https://w3c.github.io/pointerevents/#process-pending-pointer-capture
> 
> and this method was trying to implement exactly that. Whether a release will
> happen or not depending on the explicit release call or buttons value that is
> something else and when it happens it only sets the "pending pointer capture
> target" variable. On the other hand, when processPendingPointerCapture is
called
> it is processing the assignment of "pointer capture target" from "pending
> pointer capture target" and sends the got/lostpointercapture. Is this okay?
> Maybe I can put a link to the spec section as comment before that function. Do
> you want me to do that or will it be too tight to the spec?

Done.

https://codereview.chromium.org/1968653005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:111: // Set
width/height to 1 because we don't get any information from lower
On 2016/05/11 20:50:20, Navid Zolghadr wrote:
> On 2016/05/11 20:42:57, mustaq wrote:
> > "...because it matches Edge (and supported by the spec)."
> 
> Sure.

Done.

[Rick Byers, 2016-05-13 18:38:50]

LGTM

[Navid Zolghadr, 2016-05-16 13:00:37]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-05-16 13:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/1968653005/#ps40001
(title: "Rebased")

[commit-bot: I haz the power, 2016-05-16 13:00:48]

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

[commit-bot: I haz the power, 2016-05-16 14:37:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-16 14:38:36]

Description was changed from

==========
Set width=height=1 of mouse like pointer events

Right now these devices don't report any geometry
and there is no plumbing to set these values in
the lower levels. So for now we just set them in
PointerEventFactory.

BUG=608459
==========

to

==========
Set width=height=1 of mouse like pointer events

Right now these devices don't report any geometry
and there is no plumbing to set these values in
the lower levels. So for now we just set them in
PointerEventFactory.

BUG=608459

Committed: https://crrev.com/69ca22269406e17f2e67caaa5e01167736ccc86b
Cr-Commit-Position: refs/heads/master@{#393829}
==========

[commit-bot: I haz the power, 2016-05-16 14:38:37]

Patchset 3 (id:??) landed as
https://crrev.com/69ca22269406e17f2e67caaa5e01167736ccc86b
Cr-Commit-Position: refs/heads/master@{#393829}