Impelement InputDevice.firesTouchEvents

Impelement InputDevice.firesTouchEvents

Long term InputDevice API design sketch:
https://docs.google.com/a/chromium.org/document/d/1WLadG2dn4vlCewOmUtUEoRsThiptC7Ox28CRmYUn8Uw/edit

This CL implements 'firesTouchEvents' property in InputDevice API.

The Intent-to-Implement: UIEvent.sourceDevice &
InputDevice.firesTouchEvents link:
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/inputdevice/blink-dev/wjtZl9jpCpI/xC0JTRKgc9kJ

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

[lanwei, 2015-05-26 22:31:42]

lanwei@chromium.org changed reviewers:
+ rbyers@chromium.org, tdresser@chromium.org

[tdresser, 2015-05-27 12:38:47]

We should probably put this behind a RuntimeEnabledFeature:
https://www.chromium.org/blink/runtime-enabled-features

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
File Source/core/events/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
Source/core/events/InputDevice.h:43: // touch event handlers.
"Whether this device dispatches touch events for movement."

It isn't just for movement, it's just whether the devices dispatches touch
events, isn't it?

"This is used to detect mouse events which represent only an action..."
Maybe "This lets developers avoid handling both touch and mouse events
dispatched for a single user action"

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
File Source/core/events/InputDevice.idl (right):

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
Source/core/events/InputDevice.idl:5: // Represents an input device (or group of
related devices).
Can we expand on what we mean by "related"? Is our only guarantee that devices
with different properties will be separate InputDevices?

[Rick Byers, 2015-05-28 00:21:57]

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
File Source/core/events/InputDevice.idl (right):

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
Source/core/events/InputDevice.idl:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
as discussed offline, maybe this is a good time to create Source/core/input? 
EventHandler.* could go there (but that can be a separate CL).  Technically
InputDevice isn't strictly coupled to events (we hope to some day have some
InputDevice.getAllDevices API).

[lanwei, 2015-05-28 14:56:18]

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
File Source/core/events/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
Source/core/events/InputDevice.h:43: // touch event handlers.
On 2015/05/27 12:38:47, tdresser wrote:
> "Whether this device dispatches touch events for movement."
> 
> It isn't just for movement, it's just whether the devices dispatches touch
> events, isn't it?
> 
> "This is used to detect mouse events which represent only an action..."
> Maybe "This lets developers avoid handling both touch and mouse events
> dispatched for a single user action"

Done.

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
File Source/core/events/InputDevice.idl (right):

https://codereview.chromium.org/1157173003/diff/1/Source/core/events/InputDev...
Source/core/events/InputDevice.idl:5: // Represents an input device (or group of
related devices).
On 2015/05/27 12:38:47, tdresser wrote:
> Can we expand on what we mean by "related"? Is our only guarantee that devices
> with different properties will be separate InputDevices?

Done.

[tdresser, 2015-05-28 15:03:21]

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
File Source/core/events/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
Source/core/events/InputDevice.h:11: #include "platform/heap/Handle.h"
Is this required?

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
File Source/core/events/InputDevice.idl (right):

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
Source/core/events/InputDevice.idl:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2015/05/28 00:21:57, Rick Byers wrote:
> as discussed offline, maybe this is a good time to create Source/core/input? 
> EventHandler.* could go there (but that can be a separate CL).  Technically
> InputDevice isn't strictly coupled to events (we hope to some day have some
> InputDevice.getAllDevices API).

+1

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
Source/core/events/InputDevice.idl:10: // To ease implementation, developers
cannot rely on comparing two InputDevice
I'd move the "To ease implementation..." section above the description of how we
only have two InputDevices, and then get rid of the "Eg. two mice with the
same..."

Something like:

Represents an input device (or group of related devices).
To ease implementation, developers cannot rely on comparing 
two InputDevice instances for equality. We only have two InputDevice instances
right now:
one represents all the devices which send touch events, the other one covers
all other devices which do not send touch events, because currently all we care
about is if the input device fires touch events or not.

[lanwei, 2015-05-28 18:14:43]

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
File Source/core/events/InputDevice.idl (right):

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
Source/core/events/InputDevice.idl:1: // Copyright 2015 The Chromium Authors.
All rights reserved.
On 2015/05/28 15:03:21, tdresser wrote:
> On 2015/05/28 00:21:57, Rick Byers wrote:
> > as discussed offline, maybe this is a good time to create Source/core/input?

> > EventHandler.* could go there (but that can be a separate CL).  Technically
> > InputDevice isn't strictly coupled to events (we hope to some day have some
> > InputDevice.getAllDevices API).
> 
> +1

I will create a new fold in another CL, and move InputDevice and EventHandler to
it.

https://codereview.chromium.org/1157173003/diff/20001/Source/core/events/Inpu...
Source/core/events/InputDevice.idl:10: // To ease implementation, developers
cannot rely on comparing two InputDevice
On 2015/05/28 15:03:21, tdresser wrote:
> I'd move the "To ease implementation..." section above the description of how
we
> only have two InputDevices, and then get rid of the "Eg. two mice with the
> same..."
> 
> Something like:
> 
> Represents an input device (or group of related devices).
> To ease implementation, developers cannot rely on comparing 
> two InputDevice instances for equality. We only have two InputDevice instances
> right now:
> one represents all the devices which send touch events, the other one covers
> all other devices which do not send touch events, because currently all we
care
> about is if the input device fires touch events or not.

Done.

[tdresser, 2015-05-28 18:18:18]

LGTM.

I think moving this to core/input before landing it might be easier (and then
move EventHandler in a follow-up patch). I don't have any preference for which
you do though.

https://codereview.chromium.org/1157173003/diff/40001/Source/core/events/Inpu...
File Source/core/events/InputDevice.idl (right):

https://codereview.chromium.org/1157173003/diff/40001/Source/core/events/Inpu...
Source/core/events/InputDevice.idl:8: // instances right now:
nit: the line wrapping is a bit weird here.

[lanwei, 2015-05-29 19:12:14]

Patchset #4 (id:60001) has been deleted

[lanwei, 2015-06-01 21:07:32]

Patchset #5 (id:90001) has been deleted

[lanwei, 2015-06-01 22:29:47]

Patchset #5 (id:110001) has been deleted

[Rick Byers, 2015-06-03 17:59:46]

LGTM with nits

Also I think you'll need to update the expectations for the
LayoutTests/webexposed (it should fail indicating you're adding a new
constructor).  Crucially the RuntimeEnabled should ensure you DON'T need to
change virtual/stable/webexposed (that tracks what's really exposed to the web).

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
File Source/core/input/InputDevice.cpp (right):

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:27: DEFINE_TRACE(InputDevice)
Do you need a DEFINE_TRACE even when it's empty?  I thought we only needed trace
methods when we had members that needed tracing.  But maybe they've started
enforcing this to protect against missing trace methods?

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:39: DECLARE_VIRTUAL_TRACE();
Why virtual?  Also perhaps you could just use DECLARE_INLINE_TRACE?

[lanwei, 2015-06-03 20:51:54]

Patchset #6 (id:150001) has been deleted

[lanwei, 2015-06-03 21:08:52]

Patchset #6 (id:170001) has been deleted

[lanwei, 2015-06-03 21:18:01]

Patchset #6 (id:190001) has been deleted

[lanwei, 2015-06-03 21:20:54]

lanwei@chromium.org changed reviewers:
+ tkent@chromium.org

[lanwei, 2015-06-03 21:20:55]

tkent@chromium.org: Please review changes in 

Source/platform/RuntimeEnabledFeatures.in,

I add a new class InputDevice,

thanks.

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
File Source/core/input/InputDevice.cpp (right):

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:27: DEFINE_TRACE(InputDevice)
On 2015/06/03 17:59:45, Rick Byers wrote:
> Do you need a DEFINE_TRACE even when it's empty?  I thought we only needed
trace
> methods when we had members that needed tracing.  But maybe they've started
> enforcing this to protect against missing trace methods?
Once we use DEFINE_INLINE_TRACE, we do not need this anymore.
Done.

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/130001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:39: DECLARE_VIRTUAL_TRACE();
On 2015/06/03 17:59:45, Rick Byers wrote:
> Why virtual?  Also perhaps you could just use DECLARE_INLINE_TRACE?

Done.

[tkent, 2015-06-04 00:00:47]

https://codereview.chromium.org/1157173003/diff/210001/LayoutTests/fast/input...
File LayoutTests/fast/input/input-device-constructor.html (right):

https://codereview.chromium.org/1157173003/diff/210001/LayoutTests/fast/input...
LayoutTests/fast/input/input-device-constructor.html:10: shouldBe("new
InputDevice().firesTouchEvents", "false");
shouldBe -> shouldBeFalse

https://codereview.chromium.org/1157173003/diff/210001/LayoutTests/fast/input...
LayoutTests/fast/input/input-device-constructor.html:11: shouldBe("new
InputDevice({ firesTouchEvents: true }).firesTouchEvents", "true");
shouldBe -> shouldBeTrue

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.cpp (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:4: #include "config.h"
add a blank line before this line.

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:9: InputDevice::InputDevice()
m_firesTouchEvents is not initialized.

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:16: class CORE_EXPORT InputDevice : public
RefCountedWillBeGarbageCollectedFinalized<InputDevice>, public ScriptWrappable {
Can we make this GarbageCollectedFinalized<> instead of
RefCountedWillBeGarbageCollectedFinalized<>?

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:42: InputDevice();
Can we unify three constructors into one?

[Rick Byers, 2015-06-04 13:55:29]

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:42: InputDevice();
On 2015/06/04 00:00:47, tkent wrote:
> Can we unify three constructors into one?

Yeah, perhaps we only want the InputDeviceInit flavor?  We could perhaps even
remove the other two 'create' methods too and rely on all callers explicitly
creating an InputDeviceInit object?

[lanwei, 2015-06-04 21:21:36]

https://codereview.chromium.org/1157173003/diff/210001/LayoutTests/fast/input...
File LayoutTests/fast/input/input-device-constructor.html (right):

https://codereview.chromium.org/1157173003/diff/210001/LayoutTests/fast/input...
LayoutTests/fast/input/input-device-constructor.html:10: shouldBe("new
InputDevice().firesTouchEvents", "false");
On 2015/06/04 00:00:47, tkent wrote:
> shouldBe -> shouldBeFalse

Done.

https://codereview.chromium.org/1157173003/diff/210001/LayoutTests/fast/input...
LayoutTests/fast/input/input-device-constructor.html:11: shouldBe("new
InputDevice({ firesTouchEvents: true }).firesTouchEvents", "true");
On 2015/06/04 00:00:47, tkent wrote:
> shouldBe -> shouldBeTrue

Done.

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.cpp (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:4: #include "config.h"
On 2015/06/04 00:00:47, tkent wrote:
> add a blank line before this line.

Done.

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:16: class CORE_EXPORT InputDevice : public
RefCountedWillBeGarbageCollectedFinalized<InputDevice>, public ScriptWrappable {
On 2015/06/04 00:00:47, tkent wrote:
> Can we make this GarbageCollectedFinalized<> instead of
> RefCountedWillBeGarbageCollectedFinalized<>?

Done.

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:42: InputDevice();
On 2015/06/04 13:55:29, Rick Byers wrote:
> On 2015/06/04 00:00:47, tkent wrote:
> > Can we unify three constructors into one?
> 
> Yeah, perhaps we only want the InputDeviceInit flavor?  We could perhaps even
> remove the other two 'create' methods too and rely on all callers explicitly
> creating an InputDeviceInit object?

I think it is better to keep the one with a bool argument at least, instead of
creating an InputDeviceInit object for it, easy to read. We can refactor it
later if needed.

[tkent, 2015-06-04 22:55:42]

lgtm

https://codereview.chromium.org/1157173003/diff/230001/Source/core/input/Inpu...
File Source/core/input/InputDevice.cpp (right):

https://codereview.chromium.org/1157173003/diff/230001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:4: #include "config.h"
Add a blank line before this line.

https://codereview.chromium.org/1157173003/diff/230001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:5: 
Remove this blank line.

[lanwei, 2015-06-05 02:54:00]

Patchset #8 (id:250001) has been deleted

[lanwei, 2015-06-05 02:58:09]

Patchset #8 (id:270001) has been deleted

[lanwei, 2015-06-05 03:23:20]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2015-06-05 03:23:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, rbyers@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1157173003/#ps290001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-05 03:23:42]

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

[commit-bot: I haz the power, 2015-06-05 04:30:04]

Committed patchset #8 (id:290001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196553

[Rick Byers, 2015-06-10 20:37:05]

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:16: class CORE_EXPORT InputDevice : public
RefCountedWillBeGarbageCollectedFinalized<InputDevice>, public ScriptWrappable {
On 2015/06/04 00:00:47, tkent wrote:
> Can we make this GarbageCollectedFinalized<> instead of
> RefCountedWillBeGarbageCollectedFinalized<>?

I've been wondering about this.  How do we decide when a new class should be
strictly an Oilpan type vs. a WillBe* type?  Perhaps the Oilpan docs
(https://www.chromium.org/blink/blink-gc) should be updated to talk about this -
it's very unclear for random contributors not too familiar with Oilpan to know
how new code should be written...

[tkent, 2015-06-10 22:50:03]

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:16: class CORE_EXPORT InputDevice : public
RefCountedWillBeGarbageCollectedFinalized<InputDevice>, public ScriptWrappable {
On 2015/06/10 20:37:05, Rick Byers wrote:
> On 2015/06/04 00:00:47, tkent wrote:
> > Can we make this GarbageCollectedFinalized<> instead of
> > RefCountedWillBeGarbageCollectedFinalized<>?
> 
> I've been wondering about this.  How do we decide when a new class should be
> strictly an Oilpan type vs. a WillBe* type?  Perhaps the Oilpan docs
> (https://www.chromium.org/blink/blink-gc) should be updated to talk about this
-
> it's very unclear for random contributors not too familiar with Oilpan to know
> how new code should be written...

There are no clear rules.  However, new web-exposed classes should be
GarbageCollected or GarbageCollectedFinalized without WillBe.  All of
web-exposed classes will be on Oilpan heap in the near future, and oilpan-team
is trying to reduce the number classes with WillBe now.

[Rick Byers, 2015-06-10 23:06:53]

On 2015/06/10 22:50:03, tkent wrote:
>
https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
> File Source/core/input/InputDevice.h (right):
> 
>
https://codereview.chromium.org/1157173003/diff/210001/Source/core/input/Inpu...
> Source/core/input/InputDevice.h:16: class CORE_EXPORT InputDevice : public
> RefCountedWillBeGarbageCollectedFinalized<InputDevice>, public ScriptWrappable
{
> On 2015/06/10 20:37:05, Rick Byers wrote:
> > On 2015/06/04 00:00:47, tkent wrote:
> > > Can we make this GarbageCollectedFinalized<> instead of
> > > RefCountedWillBeGarbageCollectedFinalized<>?
> > 
> > I've been wondering about this.  How do we decide when a new class should be
> > strictly an Oilpan type vs. a WillBe* type?  Perhaps the Oilpan docs
> > (https://www.chromium.org/blink/blink-gc) should be updated to talk about
this
> -
> > it's very unclear for random contributors not too familiar with Oilpan to
know
> > how new code should be written...
> 
> There are no clear rules.  However, new web-exposed classes should be
> GarbageCollected or GarbageCollectedFinalized without WillBe.  All of
> web-exposed classes will be on Oilpan heap in the near future, and oilpan-team
> is trying to reduce the number classes with WillBe now.

Makes sense, thanks!

[lanwei, 2015-06-16 18:11:15]

https://codereview.chromium.org/1157173003/diff/230001/Source/core/input/Inpu...
File Source/core/input/InputDevice.cpp (right):

https://codereview.chromium.org/1157173003/diff/230001/Source/core/input/Inpu...
Source/core/input/InputDevice.cpp:4: #include "config.h"
On 2015/06/04 22:55:42, tkent wrote:
> Add a blank line before this line.

Done.