Implement UIEvent.sourceDevice

Implement UIEvent.sourceDevice

Since now we have InputDevice information, in this CL we populates sourceDevice
attribute into UIEvent, then later on all types of UIEvent will have their own
sourceDevice information.

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/)

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

[lanwei, 2015-05-27 19:09:00]

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

[lanwei, 2015-05-27 19:21:10]

Patchset #1 (id:1) has been deleted

[Rick Byers, 2015-05-28 00:11:43]

Please reference the intent-to-implement thread in the CL description.

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

https://codereview.chromium.org/1160753005/diff/20001/LayoutTests/fast/events...
LayoutTests/fast/events/uievent-with-inputdevice.html:13: var onclick =
function(e) {
You should probably make this document.addEventListener('click', function(e) {
.. });

You could use 'document.onclick =' or even 'onclick =' (which is the same as
window.onclick), but that pattern is legacy - we generally prefer the
addEventListener pattern.

Then you should see your handler getting invoked and you can test the
sourceDevice.

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
File Source/core/events/UIEvent.idl (right):

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
Source/core/events/UIEvent.idl:25: readonly attribute InputDevice? sourceDevice;
this should be [RuntimeEnabled=InputDevice]

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
File Source/core/events/UIEventInit.idl (right):

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
Source/core/events/UIEventInit.idl:10: InputDevice? sourceDevice = null;
RuntimeEnabled

[lanwei, 2015-05-29 17:57:05]

Patchset #2 (id:40001) has been deleted

[lanwei, 2015-05-29 18:02:13]

Patchset #2 (id:60001) has been deleted

[lanwei, 2015-05-29 18:02:39]

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
File Source/core/events/UIEvent.idl (right):

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
Source/core/events/UIEvent.idl:25: readonly attribute InputDevice? sourceDevice;
On 2015/05/28 00:11:43, Rick Byers (Out until 6-2) wrote:
> this should be [RuntimeEnabled=InputDevice]

Done.

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
File Source/core/events/UIEventInit.idl (right):

https://codereview.chromium.org/1160753005/diff/20001/Source/core/events/UIEv...
Source/core/events/UIEventInit.idl:10: InputDevice? sourceDevice = null;
On 2015/05/28 00:11:43, Rick Byers (Out until 6-2) wrote:
> RuntimeEnabled

Done.

[tdresser, 2015-06-01 17:36:46]

Can you clarify in the change description what this patch actually does? (i.e.
the fact that we don't actually populate UIEvent.sourceDevice yet)

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

https://codereview.chromium.org/1160753005/diff/80001/LayoutTests/fast/events...
LayoutTests/fast/events/uievent-with-inputdevice.html:6: description("This tests
that all the subclasses of UIEvent will have sourceDevice set to be null by
default, and it can also be passed when initialization.")
We aren't actually checking if sourceDevice is set to null, just that it's
falsy, right? Let's check if it's null.

https://codereview.chromium.org/1160753005/diff/80001/Source/core/events/UIEv...
File Source/core/events/UIEvent.h (right):

https://codereview.chromium.org/1160753005/diff/80001/Source/core/events/UIEv...
Source/core/events/UIEvent.h:34: #include <stdlib.h>
What's stdlib being used for?

[lanwei, 2015-06-02 14:08:31]

https://codereview.chromium.org/1160753005/diff/80001/Source/core/events/UIEv...
File Source/core/events/UIEvent.h (right):

https://codereview.chromium.org/1160753005/diff/80001/Source/core/events/UIEv...
Source/core/events/UIEvent.h:34: #include <stdlib.h>
On 2015/06/01 17:36:46, tdresser wrote:
> What's stdlib being used for?

Done.

[tdresser, 2015-06-02 14:43:55]

LGTM with nits.

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

https://codereview.chromium.org/1160753005/diff/100001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:13:
document.addEventListener("click", function(event) {  firesTouchEvents =
event.sourceDevice.firesTouchEvents; } );
Can you add some newlines here? Something like:

document.addEventListener("click", function(event) {  
  firesTouchEvents = event.sourceDevice.firesTouchEvents; 
});

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

https://codereview.chromium.org/1160753005/diff/100001/Source/core/input/Inpu...
Source/core/input/InputDevice.h:47: InputDevice(bool);
InputDevice(bool firesTouchEvents);

https://codereview.chromium.org/1160753005/diff/100001/Source/core/testing/In...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1160753005/diff/100001/Source/core/testing/In...
Source/core/testing/Internals.h:358: void setFiresTouchEvents(UIEvent*, bool);
bool firesTouchEvents

[Rick Byers, 2015-06-04 14:52:35]

Sorry I didn't catch this sooner, but I don't think this is quite the right
incremental next step along the path to implementing this correctly.  Attaching
a new fake InputDevice object to every single UIEvent is pretty far from the
ultimate behavior and leads to testing patterns that are throw-away.

I'd suggest either:
1) Leave UIEvent.sourceDevice null in ALL cases except when an event is created
from JS and an explicit InputDevice is supplied to the EventInitDict.  Then you
can test that you can pass your own InputDevice along to a UIEvent.

2) If you prefer to have a temporary dummy InputDevice object on all UIEvent
instances that's OK with me, but it should be there only for UIEvents that are
created by blink itself, not those created from JavaScript (those should be null
unless explicitly supplied).

More discussion below.

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

https://codereview.chromium.org/1160753005/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:14: firesTouchEvents =
event.sourceDevice.firesTouchEvents;
sourceDevice is actually supposed to be null here (by default, events created
from script should not have a sourceDevice - although one can be provided
explicitly in the EventInit dictionary).

Maybe for this CL you could just make sourceDevice null for the script-generated
events, and your dummy default value for some/all other generated events and
then test that, for example, a 'click' event generated from script has null, but
a 'click' event generated via eventSender.mousedown/mouseup has non-null
sourceDevice and sourceDevice.firesTouchEvents false.

https://codereview.chromium.org/1160753005/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:21:
shouldBeFalse('firesTouchEvents');
nit: move this shouldBe into the event handler itself.  That way the test output
will change if, for example, the handler gets invoked more than once for some
reason.

https://codereview.chromium.org/1160753005/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:24:
internals.setFiresTouchEvents(e, true);
I don't think this internals API is worth the cost - you're not really testing
much here.

Instead our test coverage of the different default values on InputDevice should
come through event sender.  I.e. once you've wired up mouse events to get a
different InputDevice than TouchEvents, then we should use EventSender to
generate mouse, touch and gesture events and verify that
sourceDevice.firesTouchEvents is what we expect.

For now I'd suggest removing this part of the test.

https://codereview.chromium.org/1160753005/diff/120001/Source/core/events/UIE...
File Source/core/events/UIEvent.cpp (right):

https://codereview.chromium.org/1160753005/diff/120001/Source/core/events/UIE...
Source/core/events/UIEvent.cpp:30: , m_sourceDevice(InputDevice::create(false))
These lines that create a source device out of nothing (without really knowing
where the event is coming from) are temporary, and we'll have to make sure we
find and remove them all before shipping source device.  So please add a comment
like this to each one:
"TODO(lanwei): Get correct InputDevice for this scenario"

https://codereview.chromium.org/1160753005/diff/120001/Source/platform/Runtim...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1160753005/diff/120001/Source/platform/Runtim...
Source/platform/RuntimeEnabledFeatures.in:122: RegionBasedColumns status=stable
whoa, how did this change get into this patch?

[tdresser, 2015-06-04 15:34:24]

not LGTM, just so I can take another look once you've made the changes Rick has
proposed.

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

https://codereview.chromium.org/1160753005/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:14: firesTouchEvents =
event.sourceDevice.firesTouchEvents;
On 2015/06/04 14:52:35, Rick Byers wrote:
> sourceDevice is actually supposed to be null here (by default, events created
> from script should not have a sourceDevice - although one can be provided
> explicitly in the EventInit dictionary).
> 
> Maybe for this CL you could just make sourceDevice null for the
script-generated
> events, and your dummy default value for some/all other generated events and
> then test that, for example, a 'click' event generated from script has null,
but
> a 'click' event generated via eventSender.mousedown/mouseup has non-null
> sourceDevice and sourceDevice.firesTouchEvents false.

Oops, thanks Rick.

I caught that
here(https://codereview.chromium.org/1161783006/diff/20001/LayoutTests/fast/events/touch/create-touch-event-source-device.html#newcode19)
but I missed it in this patch.

I'll take another look at this patch once this is fixed.

[lanwei, 2015-06-05 04:17:28]

Patchset #5 (id:140001) has been deleted

[tdresser, 2015-06-05 12:35:39]

LGTM with nits, thanks.

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

https://codereview.chromium.org/1160753005/diff/160001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:12: shouldBeNonNull("new
UIEvent('eventType', { sourceDevice: new InputDevice({ firesTouchEvents: false
}) }).sourceDevice");
Might as well create and cache this UIEvent, instead of creating the same one
twice.

https://codereview.chromium.org/1160753005/diff/160001/LayoutTests/fast/event...
LayoutTests/fast/events/uievent-with-inputdevice.html:15: shouldBeNonNull("new
UIEvent('eventType', { sourceDevice: new InputDevice({ firesTouchEvents: true })
}).sourceDevice");
Same as above.

[Rick Byers, 2015-06-08 14:31:06]

Thanks, this is now definitely a simple step in the right direction.  LGTM

[lanwei, 2015-06-08 18:07:36]

Patchset #6 (id:180001) has been deleted

[lanwei, 2015-06-08 18:10:31]

Patchset #6 (id:200001) has been deleted

[lanwei, 2015-06-08 22:14:25]

Patchset #6 (id:220001) has been deleted

[lanwei, 2015-06-09 02:16:38]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2015-06-09 02:16:38]

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/1160753005/#ps240001
(title: "Layout test and webexposed")

[commit-bot: I haz the power, 2015-06-09 02:17:13]

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

[commit-bot: I haz the power, 2015-06-09 03:04:01]

Committed patchset #6 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196731