Fixed missing pointerTypes for touch events.

Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817
Committed: https://crrev.com/cc6220fa81c73d8be7ac0b032cb7fecd2b5b4b6a
Cr-Commit-Position: refs/heads/master@{#380238}

[mustaq, 2016-03-03 17:52:23]

Description was changed from

==========
Fixed a few missing pointerTypes.

This CL fixed all missing pointerTypes that takes uninitilalized
default value ("unknown") while coming from touch-events.

BUG=557817
==========

to

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.

BUG=557817
==========

[mustaq, 2016-03-03 17:52:24]

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

[Navid Zolghadr, 2016-03-03 19:11:20]

https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:629: point.pointerType
= WebPointerProperties::PointerType::Touch;
Do we by any chance pass this point while we are handling pen events on Android?

[mustaq, 2016-03-03 19:18:37]

https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebInputEventConversion.cpp:629: point.pointerType
= WebPointerProperties::PointerType::Touch;
On 2016/03/03 19:11:20, Navid Zolghadr wrote:
> Do we by any chance pass this point while we are handling pen events on
Android?
> 

No, this reverse conversion is used for plugins.

For Android, we should check the conversion from MotionEvent to WebMouseEvent,
which seems okay after a quick look. I will check it later on.

This CL is not a complete fix for the bug, as it only covers touches. I am
looking at WebMouseEvents now.

[mustaq, 2016-03-03 19:22:34]

On 2016/03/03 19:18:37, mustaq wrote:
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):
> 
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/WebInputEventConversion.cpp:629:
point.pointerType
> = WebPointerProperties::PointerType::Touch;
> On 2016/03/03 19:11:20, Navid Zolghadr wrote:
> > Do we by any chance pass this point while we are handling pen events on
> Android?
> > 
> 
> No, this reverse conversion is used for plugins.
> 
> For Android, we should check the conversion from MotionEvent to WebMouseEvent,
> which seems okay after a quick look. I will check it later on.
> 
> This CL is not a complete fix for the bug, as it only covers touches. I am
> looking at WebMouseEvents now.

Correction: I meant for Android the conversion from MotionEvent to
*WebTouchPoint*.

[Navid Zolghadr, 2016-03-03 22:29:09]

On 2016/03/03 19:22:34, mustaq wrote:
> On 2016/03/03 19:18:37, mustaq wrote:
> >
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/WebInputEventConversion.cpp:629:
> point.pointerType
> > = WebPointerProperties::PointerType::Touch;
> > On 2016/03/03 19:11:20, Navid Zolghadr wrote:
> > > Do we by any chance pass this point while we are handling pen events on
> > Android?
> > > 
> > 
> > No, this reverse conversion is used for plugins.
> > 
> > For Android, we should check the conversion from MotionEvent to
WebMouseEvent,
> > which seems okay after a quick look. I will check it later on.
> > 
> > This CL is not a complete fix for the bug, as it only covers touches. I am
> > looking at WebMouseEvents now.
> 
> Correction: I meant for Android the conversion from MotionEvent to
> *WebTouchPoint*.

LGTM

[Navid Zolghadr, 2016-03-03 22:29:12]

On 2016/03/03 19:22:34, mustaq wrote:
> On 2016/03/03 19:18:37, mustaq wrote:
> >
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/WebInputEventConversion.cpp:629:
> point.pointerType
> > = WebPointerProperties::PointerType::Touch;
> > On 2016/03/03 19:11:20, Navid Zolghadr wrote:
> > > Do we by any chance pass this point while we are handling pen events on
> > Android?
> > > 
> > 
> > No, this reverse conversion is used for plugins.
> > 
> > For Android, we should check the conversion from MotionEvent to
WebMouseEvent,
> > which seems okay after a quick look. I will check it later on.
> > 
> > This CL is not a complete fix for the bug, as it only covers touches. I am
> > looking at WebMouseEvents now.
> 
> Correction: I meant for Android the conversion from MotionEvent to
> *WebTouchPoint*.

LGTM

[mustaq, 2016-03-07 15:22:53]

Description was changed from

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.

BUG=557817
==========

to

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817
==========

[mustaq, 2016-03-07 16:51:29]

On 2016/03/03 22:29:12, Navid Zolghadr wrote:
> On 2016/03/03 19:22:34, mustaq wrote:
> > On 2016/03/03 19:18:37, mustaq wrote:
> > >
> >
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> > > File third_party/WebKit/Source/web/WebInputEventConversion.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1760183002/diff/1/third_party/WebKit/Source/w...
> > > third_party/WebKit/Source/web/WebInputEventConversion.cpp:629:
> > point.pointerType
> > > = WebPointerProperties::PointerType::Touch;
> > > On 2016/03/03 19:11:20, Navid Zolghadr wrote:
> > > > Do we by any chance pass this point while we are handling pen events on
> > > Android?
> > > > 
> > > 
> > > No, this reverse conversion is used for plugins.
> > > 
> > > For Android, we should check the conversion from MotionEvent to
> WebMouseEvent,
> > > which seems okay after a quick look. I will check it later on.
> > > 
> > > This CL is not a complete fix for the bug, as it only covers touches. I am
> > > looking at WebMouseEvents now.
> > 
> > Correction: I meant for Android the conversion from MotionEvent to
> > *WebTouchPoint*.
> 
> LGTM

ptal, added most of the missing mouse event type.

[Rick Byers, 2016-03-07 17:52:20]

LGTM with nits

https://codereview.chromium.org/1760183002/diff/40001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:156:
WebPointerProperties::PointerType ToWebPointerType(int tool_type) {
I think there's an existing process for sharing enums between C++ and Java code,
not sure if hard-coded constants like this are frowned upon (at the least I
assume we'd want a comment saying what the real Java type definition is).  But
I'm fine leaving this to whoever does the OWNERS review for
content/browser/android (aelias maybe?).

https://codereview.chromium.org/1760183002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/event_conversion.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/content/renderer/pepper...
content/renderer/pepper/event_conversion.cc:277: // TODO(crrev.com/93902)
nit: this link is wrong, you meant crbug.com/93902

https://codereview.chromium.org/1760183002/diff/40001/mojo/converters/blink/b...
File mojo/converters/blink/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/mojo/converters/blink/b...
mojo/converters/blink/blink_input_events_type_converters.cc:77: // TODO(mustaq):
Set web_event->pointerType from event->pointer_data->kind
nit: bug tracking this?

[Rick Byers, 2016-03-07 17:55:43]

These are mostly edge cases that are probably not worth the effort to test
independently, but you do have some basic tests for the pointerType in common
scenarios, right?  Did you go through each of these cases asking yourself
whether any of them were easy enough / valuable enough to be worth updating a
test case to cover?  In general I'm worried about the interaction of all the
special sources of mouse/touch events with pointer events - kinda expect a bug
tail here.  So it's at least worth thinking about the appropriate testing
strategy (though I DON'T think it makes sense to try to be exhaustive - covering
every possible scenario / bug).

[mustaq, 2016-03-07 21:50:43]

Tweaked existing layoutTests to catch default types. End-to-end tests for
plugins, Android etc would be good in general, but it doesn't worth the effort
for this tiny change.

https://codereview.chromium.org/1760183002/diff/40001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:156:
WebPointerProperties::PointerType ToWebPointerType(int tool_type) {
On 2016/03/07 17:52:20, Rick Byers wrote:
> I think there's an existing process for sharing enums between C++ and Java
code,
> not sure if hard-coded constants like this are frowned upon (at the least I
> assume we'd want a comment saying what the real Java type definition is).  But
> I'm fine leaving this to whoever does the OWNERS review for
> content/browser/android (aelias maybe?).

Reused the method in blink_event_util here.

https://codereview.chromium.org/1760183002/diff/40001/content/renderer/pepper...
File content/renderer/pepper/event_conversion.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/content/renderer/pepper...
content/renderer/pepper/event_conversion.cc:277: // TODO(crrev.com/93902)
On 2016/03/07 17:52:20, Rick Byers wrote:
> nit: this link is wrong, you meant crbug.com/93902

Done.

https://codereview.chromium.org/1760183002/diff/40001/mojo/converters/blink/b...
File mojo/converters/blink/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/mojo/converters/blink/b...
mojo/converters/blink/blink_input_events_type_converters.cc:77: // TODO(mustaq):
Set web_event->pointerType from event->pointer_data->kind
On 2016/03/07 17:52:20, Rick Byers wrote:
> nit: bug tracking this?

I think I will use the correct mojo field here, should be easy but I don't know
how to test it. I will ask a mojo expert about it.

[mustaq, 2016-03-07 21:56:53]

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

[mustaq, 2016-03-07 21:56:54]

aelias@chromium.org: Please review changes in 
- content/browser/renderer_host/input
- ui/events/blink

[aelias_OOO_until_Jul13, 2016-03-08 01:45:27]

lgtm

[mustaq, 2016-03-08 14:43:11]

mustaq@chromium.org changed reviewers:
+ pfeldman@chromium.org, sky@chromium.org

[mustaq, 2016-03-08 14:43:12]

sky@chromium.org: Please review changes in:
- mojo/converters/blink/blink_input_events_type_converters.cc

pfeldman@chromium.org: Please review changes in:
- content/browser/android/content_view_core_impl.*
- content/browser/devtools/protocol/input_handler.cc
-
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
- content/renderer/pepper/event_conversion.cc

[Rick Byers, 2016-03-08 15:43:38]

> Tweaked existing layoutTests to catch default types. End-to-end tests for
> plugins, Android etc would be good in general, but it doesn't worth the effort
> for this tiny change.

Agreed.  Thanks for the test updates.  Still LGTM

https://codereview.chromium.org/1760183002/diff/40001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/content/browser/android...
content/browser/android/content_view_core_impl.cc:156:
WebPointerProperties::PointerType ToWebPointerType(int tool_type) {
On 2016/03/07 21:50:42, mustaq wrote:
> On 2016/03/07 17:52:20, Rick Byers wrote:
> > I think there's an existing process for sharing enums between C++ and Java
> code,
> > not sure if hard-coded constants like this are frowned upon (at the least I
> > assume we'd want a comment saying what the real Java type definition is). 
But
> > I'm fine leaving this to whoever does the OWNERS review for
> > content/browser/android (aelias maybe?).
> 
> Reused the method in blink_event_util here.

Ah, even better - thanks!

https://codereview.chromium.org/1760183002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-event-properties.html
(left):

https://codereview.chromium.org/1760183002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-event-properties.html:72:
// TODO(crbug.com/557817): linux_chromium_rel_ng fails here, sets to empty
string.
excellent!

[sky, 2016-03-08 17:45:03]

LGTM

[mustaq, 2016-03-09 17:53:27]

https://codereview.chromium.org/1760183002/diff/40001/mojo/converters/blink/b...
File mojo/converters/blink/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/1760183002/diff/40001/mojo/converters/blink/b...
mojo/converters/blink/blink_input_events_type_converters.cc:77: // TODO(mustaq):
Set web_event->pointerType from event->pointer_data->kind
On 2016/03/07 21:50:43, mustaq wrote:
> On 2016/03/07 17:52:20, Rick Byers wrote:
> > nit: bug tracking this?
> 
> I think I will use the correct mojo field here, should be easy but I don't
know
> how to test it. I will ask a mojo expert about it.

Filed a bug for now.

[pfeldman, 2016-03-09 18:22:00]

devtools lgtm

[mustaq, 2016-03-09 18:48:34]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2016-03-09 18:48:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
nzolghadr@chromium.org, rbyers@chromium.org, aelias@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1760183002/#ps80001
(title: "Fixed line-lengths, added a bug ref.")

[commit-bot: I haz the power, 2016-03-09 18:48:53]

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

[commit-bot: I haz the power, 2016-03-09 22:08:03]

Description was changed from

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817
==========

to

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817
==========

[commit-bot: I haz the power, 2016-03-09 22:08:04]

Committed patchset #5 (id:80001)

[mustaq, 2016-03-09 22:10:53]

Description was changed from

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817
==========

to

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817,591473
==========

[commit-bot: I haz the power, 2016-03-09 22:47:47]

Description was changed from

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817,591473
==========

to

==========
Fix missing pointerTypes for touch.

This CL fixes uninitialized pointerTypes for WebTouchPoint.
Also fixes a few missing pointerType=mouse.

BUG=557817

Committed: https://crrev.com/cc6220fa81c73d8be7ac0b032cb7fecd2b5b4b6a
Cr-Commit-Position: refs/heads/master@{#380238}
==========

[commit-bot: I haz the power, 2016-03-09 22:47:49]

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