Populates sourceDevice attribute into MouseEvent

Populates sourceDevice attribute into MouseEvent

Since now we have InputDevice information, in this CL we populates sourceDevice
attribute into MouseEvent, which has firesTouchEvents set to be false, if it is from a real mouse device.
If the mouse events are synthesized from touch events, then firesTouchEvents is set to be true.

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 

This CL is depended on Issue 1157173003, which creates the InputDevice class
(https://codereview.chromium.org/1157173003/)
and Issue 1160753005, which Implement UIEvent.sourceDevice
(https://codereview.chromium.org/1160753005/)

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

[lanwei, 2015-06-11 19:06:26]

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

[lanwei, 2015-06-11 19:06:41]

Patchset #1 (id:1) has been deleted

[lanwei, 2015-06-11 19:06:46]

Patchset #1 (id:20001) has been deleted

[lanwei, 2015-06-11 19:06:53]

Patchset #1 (id:40001) has been deleted

[lanwei, 2015-06-11 20:45:04]

Patchset #2 (id:80001) has been deleted

[lanwei, 2015-06-12 05:23:58]

Patchset #2 (id:100001) has been deleted

[tdresser, 2015-06-12 13:06:54]

LGTM with nits.

https://codereview.chromium.org/1174683004/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/uievent-with-inputdevice.html (right):

https://codereview.chromium.org/1174683004/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:21:
document.addEventListener("click", function(event) {
The other tests look at mousedown/mousemove/mouseup.
It would probably make sense to test those here as well.

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

https://codereview.chromium.org/1174683004/diff/120001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:24: static InputDevice* nonTouchEventInstance();
I should have commented on this sooner - I think we can find better names for
these.

Having mouseEvent->sourceDevice() == InputDevice::touchEventInstance() doesn't
make much sense.

Perhaps call these firesTouchEventsInstance and doesntFireTouchEventsInstance?

I'm not completely happy with that naming either, but I do like it better.

[lanwei, 2015-06-16 21:51:43]

https://codereview.chromium.org/1174683004/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/uievent-with-inputdevice.html (right):

https://codereview.chromium.org/1174683004/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:21:
document.addEventListener("click", function(event) {
On 2015/06/12 13:06:54, tdresser wrote:
> The other tests look at mousedown/mousemove/mouseup.
> It would probably make sense to test those here as well.

Done.

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

https://codereview.chromium.org/1174683004/diff/120001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:24: static InputDevice* nonTouchEventInstance();
On 2015/06/12 13:06:54, tdresser wrote:
> I should have commented on this sooner - I think we can find better names for
> these.
> 
> Having mouseEvent->sourceDevice() == InputDevice::touchEventInstance() doesn't
> make much sense.
> 
> Perhaps call these firesTouchEventsInstance and doesntFireTouchEventsInstance?
> 
> I'm not completely happy with that naming either, but I do like it better.

Done.

[lanwei, 2015-06-16 21:58:15]

Patchset #3 (id:140001) has been deleted

[Rick Byers, 2015-06-23 00:55:57]

Sorry for the delay, finally getting caught up from travelling.    Just a couple
suggestions to try to simplify things a little bit.

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html
(right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html:16:
document.addEventListener("mouseup", mouseHandler);
nit: please add a 'click' handler too.
You can do this concisely with something like: 

for (var evt of ['mousemove', 'mousedown', 'mouseup', 'click') {
  document.addEventListener(evt, mouseHandler);
}

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File LayoutTests/fast/events/mouse-event-source-device-event-sender.html
(right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/mouse-event-source-device-event-sender.html:15:
document.addEventListener("mouseup", mouseHandler);
also add click listener here too

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File LayoutTests/fast/events/uievent-with-inputdevice.html (right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:36:
mouseevent.initMouseEvent("mousedown", true, true, window, 0, 0, 0, 0, 0, false,
false, false, false, 0, null, sourceDevice);
nit: please use the 'new MouseEvent' style instead or as well (just like the
'new UIEvent' above).  If we're only going to test one of the patterns than the
modern style is probably the one we really care about.

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.cpp:92: ctrlKey, altKey, shiftKey, metaKey,
isSimulated)
rather than rely on a setSourceDevice method on UIEvent you can use a
conditional expression in the ctor here, eg:
syntheticEventType == PlatformMouseEvent::FromtTouch ?
InputDevice::firestouchEventsInputDevice() :
InputDevice::doesntFireTouchEventsInputDevice();

Ideally the source device in a UIEvent would be immutable (so we don't need to
worry about how to behave if it changes).

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.h:70: short button,
PassRefPtrWillBeRawPtr<EventTarget> relatedTarget, InputDevice* sourceDevice,
unsigned short buttons = 0);
Do you really need to add a new overload or could you just rely on a default
value (sourceDevice = 0)?

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.idl (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.idl:72: [Default=Undefined] InputDevice?
sourceDevice);
Any chance the bindings generator is smart enough to let you add this as
'optional InputDevice? ...' on the above method, even though InputDevice is
RuntimeEnabled?  That would be simpler if it works.  Otherwise temporarily
adding another overload is fine, and we can remove it once InputDevice is
enabled by default.

[lanwei, 2015-06-23 19:43:29]

Patchset #5 (id:200001) has been deleted

[lanwei, 2015-06-23 19:49:58]

Patchset #5 (id:220001) has been deleted

[lanwei, 2015-06-23 22:19:20]

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html
(right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html:16:
document.addEventListener("mouseup", mouseHandler);
On 2015/06/23 00:55:57, Rick Byers wrote:
> nit: please add a 'click' handler too.
> You can do this concisely with something like: 
> 
> for (var evt of ['mousemove', 'mousedown', 'mouseup', 'click') {
>   document.addEventListener(evt, mouseHandler);
> }

Done.

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File LayoutTests/fast/events/mouse-event-source-device-event-sender.html
(right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/mouse-event-source-device-event-sender.html:15:
document.addEventListener("mouseup", mouseHandler);
On 2015/06/23 00:55:57, Rick Byers wrote:
> also add click listener here too

Done.

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File LayoutTests/fast/events/uievent-with-inputdevice.html (right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:36:
mouseevent.initMouseEvent("mousedown", true, true, window, 0, 0, 0, 0, 0, false,
false, false, false, 0, null, sourceDevice);
On 2015/06/23 00:55:57, Rick Byers wrote:
> nit: please use the 'new MouseEvent' style instead or as well (just like the
> 'new UIEvent' above).  If we're only going to test one of the patterns than
the
> modern style is probably the one we really care about.

Done.

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.cpp:92: ctrlKey, altKey, shiftKey, metaKey,
isSimulated)
On 2015/06/23 00:55:57, Rick Byers wrote:
> rather than rely on a setSourceDevice method on UIEvent you can use a
> conditional expression in the ctor here, eg:
> syntheticEventType == PlatformMouseEvent::FromtTouch ?
> InputDevice::firestouchEventsInputDevice() :
> InputDevice::doesntFireTouchEventsInputDevice();
> 
> Ideally the source device in a UIEvent would be immutable (so we don't need to
> worry about how to behave if it changes).

Done.

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.h:70: short button,
PassRefPtrWillBeRawPtr<EventTarget> relatedTarget, InputDevice* sourceDevice,
unsigned short buttons = 0);
On 2015/06/23 00:55:57, Rick Byers wrote:
> Do you really need to add a new overload or could you just rely on a default
> value (sourceDevice = 0)?

Done.

[lanwei, 2015-06-23 22:19:21]

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html
(right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html:16:
document.addEventListener("mouseup", mouseHandler);
On 2015/06/23 00:55:57, Rick Byers wrote:
> nit: please add a 'click' handler too.
> You can do this concisely with something like: 
> 
> for (var evt of ['mousemove', 'mousedown', 'mouseup', 'click') {
>   document.addEventListener(evt, mouseHandler);
> }

Done.

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File LayoutTests/fast/events/mouse-event-source-device-event-sender.html
(right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/mouse-event-source-device-event-sender.html:15:
document.addEventListener("mouseup", mouseHandler);
On 2015/06/23 00:55:57, Rick Byers wrote:
> also add click listener here too

Done.

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
File LayoutTests/fast/events/uievent-with-inputdevice.html (right):

https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:36:
mouseevent.initMouseEvent("mousedown", true, true, window, 0, 0, 0, 0, 0, false,
false, false, false, 0, null, sourceDevice);
On 2015/06/23 00:55:57, Rick Byers wrote:
> nit: please use the 'new MouseEvent' style instead or as well (just like the
> 'new UIEvent' above).  If we're only going to test one of the patterns than
the
> modern style is probably the one we really care about.

Done.

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.cpp:92: ctrlKey, altKey, shiftKey, metaKey,
isSimulated)
On 2015/06/23 00:55:57, Rick Byers wrote:
> rather than rely on a setSourceDevice method on UIEvent you can use a
> conditional expression in the ctor here, eg:
> syntheticEventType == PlatformMouseEvent::FromtTouch ?
> InputDevice::firestouchEventsInputDevice() :
> InputDevice::doesntFireTouchEventsInputDevice();
> 
> Ideally the source device in a UIEvent would be immutable (so we don't need to
> worry about how to behave if it changes).

Done.

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
Source/core/events/MouseEvent.h:70: short button,
PassRefPtrWillBeRawPtr<EventTarget> relatedTarget, InputDevice* sourceDevice,
unsigned short buttons = 0);
On 2015/06/23 00:55:57, Rick Byers wrote:
> Do you really need to add a new overload or could you just rely on a default
> value (sourceDevice = 0)?

Done.

[tdresser, 2015-06-24 12:46:08]

On 2015/06/23 22:19:21, lanwei wrote:
>
https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
> File
> LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html
> (right):
> 
>
https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
>
LayoutTests/fast/events/mouse-event-from-touch-source-device-event-sender.html:16:
> document.addEventListener("mouseup", mouseHandler);
> On 2015/06/23 00:55:57, Rick Byers wrote:
> > nit: please add a 'click' handler too.
> > You can do this concisely with something like: 
> > 
> > for (var evt of ['mousemove', 'mousedown', 'mouseup', 'click') {
> >   document.addEventListener(evt, mouseHandler);
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
> File LayoutTests/fast/events/mouse-event-source-device-event-sender.html
> (right):
> 
>
https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
> LayoutTests/fast/events/mouse-event-source-device-event-sender.html:15:
> document.addEventListener("mouseup", mouseHandler);
> On 2015/06/23 00:55:57, Rick Byers wrote:
> > also add click listener here too
> 
> Done.
> 
>
https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
> File LayoutTests/fast/events/uievent-with-inputdevice.html (right):
> 
>
https://codereview.chromium.org/1174683004/diff/180001/LayoutTests/fast/event...
> LayoutTests/fast/events/uievent-with-inputdevice.html:36:
> mouseevent.initMouseEvent("mousedown", true, true, window, 0, 0, 0, 0, 0,
false,
> false, false, false, 0, null, sourceDevice);
> On 2015/06/23 00:55:57, Rick Byers wrote:
> > nit: please use the 'new MouseEvent' style instead or as well (just like the
> > 'new UIEvent' above).  If we're only going to test one of the patterns than
> the
> > modern style is probably the one we really care about.
> 
> Done.
> 
>
https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
> File Source/core/events/MouseEvent.cpp (right):
> 
>
https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
> Source/core/events/MouseEvent.cpp:92: ctrlKey, altKey, shiftKey, metaKey,
> isSimulated)
> On 2015/06/23 00:55:57, Rick Byers wrote:
> > rather than rely on a setSourceDevice method on UIEvent you can use a
> > conditional expression in the ctor here, eg:
> > syntheticEventType == PlatformMouseEvent::FromtTouch ?
> > InputDevice::firestouchEventsInputDevice() :
> > InputDevice::doesntFireTouchEventsInputDevice();
> > 
> > Ideally the source device in a UIEvent would be immutable (so we don't need
to
> > worry about how to behave if it changes).
> 
> Done.
> 
>
https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
> File Source/core/events/MouseEvent.h (right):
> 
>
https://codereview.chromium.org/1174683004/diff/180001/Source/core/events/Mou...
> Source/core/events/MouseEvent.h:70: short button,
> PassRefPtrWillBeRawPtr<EventTarget> relatedTarget, InputDevice* sourceDevice,
> unsigned short buttons = 0);
> On 2015/06/23 00:55:57, Rick Byers wrote:
> > Do you really need to add a new overload or could you just rely on a default
> > value (sourceDevice = 0)?
> 
> Done.

Good call on sourceDevice being immutable.
I took another look over things, still LGTM.

[Rick Byers, 2015-06-25 03:34:54]

Awesome, now nice and simple - thanks!
LGTM with nits

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
File Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
Source/core/events/MouseEvent.cpp:291: doubleClickEvent->initMouseEvent(nullptr,
EventTypeNames::dblclick, event().bubbles(), event().cancelable(),
event().view(),
nit: You should really be using either an eventInitDict or initMouseEvent - not
both at once.  Can you just put all the things you need here into the init dict?

If not, I don't have a problem with an internal (i.e. not web exposed)
initMouseEvent method that includes an InputDevice argument - especially if you
can simply implement the web-exposed version of it in terms of your new
overload.

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
File Source/core/events/MouseRelatedEvent.h (right):

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
Source/core/events/MouseRelatedEvent.h:71: bool shiftKey, bool metaKey, bool
isSimulated = false, InputDevice* sourceDevice = nullptr);
nit: as discussed offline, default InputDevice values like this are probably
dangerous (make it too easy for a caller to accidentally forget to provide an
InputDevice - if it should be nullptr then ideally that would be explicit at the
call site).  Feel free to just add a "TODO: make this argument non-optional"
comment for now.

[lanwei, 2015-06-25 19:04:57]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2015-06-25 19:04:58]

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

[commit-bot: I haz the power, 2015-06-25 19:05:51]

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

[lanwei, 2015-06-25 19:26:40]

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
File Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
Source/core/events/MouseEvent.cpp:291: doubleClickEvent->initMouseEvent(nullptr,
EventTypeNames::dblclick, event().bubbles(), event().cancelable(),
event().view(),
On 2015/06/25 03:34:54, Rick Byers wrote:
> nit: You should really be using either an eventInitDict or initMouseEvent -
not
> both at once.  Can you just put all the things you need here into the init
dict?
> 
> If not, I don't have a problem with an internal (i.e. not web exposed)
> initMouseEvent method that includes an InputDevice argument - especially if
you
> can simply implement the web-exposed version of it in terms of your new
> overload.

Done.

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
File Source/core/events/MouseRelatedEvent.h (right):

https://codereview.chromium.org/1174683004/diff/260001/Source/core/events/Mou...
Source/core/events/MouseRelatedEvent.h:71: bool shiftKey, bool metaKey, bool
isSimulated = false, InputDevice* sourceDevice = nullptr);
On 2015/06/25 03:34:54, Rick Byers wrote:
> nit: as discussed offline, default InputDevice values like this are probably
> dangerous (make it too easy for a caller to accidentally forget to provide an
> InputDevice - if it should be nullptr then ideally that would be explicit at
the
> call site).  Feel free to just add a "TODO: make this argument non-optional"
> comment for now.

Done.

[commit-bot: I haz the power, 2015-06-25 19:58:46]

Committed patchset #7 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197849