Populates sourceDevice attribute into KeyboardEvent

Populates sourceDevice attribute into KeyboardEvent

Since now we have InputDevice information, in this CL we populates sourceDevice
attribute into KeyboardEvent, which has firesTouchEvents set to be false,
because we treat all the keyboard events the same no matter they are from physical
keyboard or virtual keyboard (it sends keyboard events to chromium not touch events).

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=197921

[lanwei, 2015-06-16 18:05:21]

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

[tdresser, 2015-06-16 19:51:41]

https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
File Source/core/events/UIEvent.idl (right):

https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
Source/core/events/UIEvent.idl:36: [Default=Undefined] optional InputDevice?
sourceDevice);
This should be behind the RuntimeEnabledFeature, shouldn't it?

Did we miss that elsewhere?

[lanwei, 2015-06-17 15:11:43]

https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
File Source/core/events/UIEvent.idl (right):

https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
Source/core/events/UIEvent.idl:36: [Default=Undefined] optional InputDevice?
sourceDevice);
On 2015/06/16 19:51:41, tdresser wrote:
> This should be behind the RuntimeEnabledFeature, shouldn't it?
> 
> Did we miss that elsewhere?

No, other places all have RuntimeEnabled=InputDevice, I am just not sure how to
put this flag.

[tdresser, 2015-06-17 15:18:59]

https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
File Source/core/events/UIEvent.idl (right):

https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
Source/core/events/UIEvent.idl:36: [Default=Undefined] optional InputDevice?
sourceDevice);
On 2015/06/17 15:11:42, lanwei wrote:
> On 2015/06/16 19:51:41, tdresser wrote:
> > This should be behind the RuntimeEnabledFeature, shouldn't it?
> > 
> > Did we miss that elsewhere?
> 
> No, other places all have RuntimeEnabled=InputDevice, I am just not sure how
to
> put this flag.

I think you need a completely separate constructor in the IDL, which is hidden
behind a runtime enabled feature.

[Rick Byers, 2015-06-23 01:01:51]

On 2015/06/17 15:18:59, tdresser wrote:
>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
> File Source/core/events/UIEvent.idl (right):
> 
>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
> Source/core/events/UIEvent.idl:36: [Default=Undefined] optional InputDevice?
> sourceDevice);
> On 2015/06/17 15:11:42, lanwei wrote:
> > On 2015/06/16 19:51:41, tdresser wrote:
> > > This should be behind the RuntimeEnabledFeature, shouldn't it?
> > > 
> > > Did we miss that elsewhere?
> > 
> > No, other places all have RuntimeEnabled=InputDevice, I am just not sure how
> to
> > put this flag.
> 
> I think you need a completely separate constructor in the IDL, which is hidden
> behind a runtime enabled feature.

Ah, I was just asking about this in the MouseEvent CL.  I was hoping the
bindings generator would be smart enough to let you do what you were trying
here.  But actually, the 'init*Event' methods are really deprecated in favor of
event constructors (eg. see
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/initKeyboardEvent).
 Perhaps we shouldn't change the init* methods at all and instead allow this to
be set via event constructors?  That would be simpler for your implementation.

The only problem is that TouchEvent doesn't (yet) have a constructor.  This
would be good reason to define one (it's been on my TODO list).

WDYT?

[blink-reviews, 2015-06-23 12:49:45]

Adding a constructor to TouchEvent sounds like a good plan - is the
constructor spec'ed? In my quick skim of the spec just now, I didn't notice
a spec.

On Mon, Jun 22, 2015 at 9:01 PM <rbyers@chromium.org> wrote:

> On 2015/06/17 15:18:59, tdresser wrote:
>
>
>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
> > File Source/core/events/UIEvent.idl (right):
>
>
>
>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
> > Source/core/events/UIEvent.idl:36: [Default=Undefined] optional
> > InputDevice?
> > sourceDevice);
> > On 2015/06/17 15:11:42, lanwei wrote:
> > > On 2015/06/16 19:51:41, tdresser wrote:
> > > > This should be behind the RuntimeEnabledFeature, shouldn't it?
> > > >
> > > > Did we miss that elsewhere?
> > >
> > > No, other places all have RuntimeEnabled=InputDevice, I am just not
> > sure how
> > to
> > > put this flag.
>
> > I think you need a completely separate constructor in the IDL, which is
> > hidden
> > behind a runtime enabled feature.
>
> Ah, I was just asking about this in the MouseEvent CL.  I was hoping the
> bindings generator would be smart enough to let you do what you were trying
> here.  But actually, the 'init*Event' methods are really deprecated in
> favor of
> event constructors (eg. see
>
>
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/initKeyboardEvent
> ).
>   Perhaps we shouldn't change the init* methods at all and instead allow
> this to
> be set via event constructors?  That would be simpler for your
> implementation.
>
> The only problem is that TouchEvent doesn't (yet) have a constructor.  This
> would be good reason to define one (it's been on my TODO list).
>
> WDYT?
>
> https://codereview.chromium.org/1182313006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Rick Byers, 2015-06-23 14:49:20]

Not yet: https://github.com/w3c/touch-events/issues/10

I'm fine leaving it as-is for TouchEvent for now (no way to customize from
JS) but maybe file a bug to track (we really shouldn't ship InputDevice
until it's customizeable in all event types).  But if this is our plan,
then we can avoid changing all the other init* methods.

Rick

On Tue, Jun 23, 2015 at 8:49 AM, Tim Dresser <tdresser@google.com> wrote:

> Adding a constructor to TouchEvent sounds like a good plan - is the
> constructor spec'ed? In my quick skim of the spec just now, I didn't notice
> a spec.
>
> On Mon, Jun 22, 2015 at 9:01 PM <rbyers@chromium.org> wrote:
>
>> On 2015/06/17 15:18:59, tdresser wrote:
>>
>>
>>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
>> > File Source/core/events/UIEvent.idl (right):
>>
>>
>>
>>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
>> > Source/core/events/UIEvent.idl:36: [Default=Undefined] optional
>> > InputDevice?
>> > sourceDevice);
>> > On 2015/06/17 15:11:42, lanwei wrote:
>> > > On 2015/06/16 19:51:41, tdresser wrote:
>> > > > This should be behind the RuntimeEnabledFeature, shouldn't it?
>> > > >
>> > > > Did we miss that elsewhere?
>> > >
>> > > No, other places all have RuntimeEnabled=InputDevice, I am just not
>> > sure how
>> > to
>> > > put this flag.
>>
>> > I think you need a completely separate constructor in the IDL, which is
>> > hidden
>> > behind a runtime enabled feature.
>>
>> Ah, I was just asking about this in the MouseEvent CL.  I was hoping the
>> bindings generator would be smart enough to let you do what you were
>> trying
>> here.  But actually, the 'init*Event' methods are really deprecated in
>> favor of
>> event constructors (eg. see
>>
>>
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/initKeyboardEvent
>> ).
>>   Perhaps we shouldn't change the init* methods at all and instead allow
>> this to
>> be set via event constructors?  That would be simpler for your
>> implementation.
>>
>> The only problem is that TouchEvent doesn't (yet) have a constructor.
>> This
>> would be good reason to define one (it's been on my TODO list).
>>
>> WDYT?
>>
>> https://codereview.chromium.org/1182313006/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[lanwei, 2015-06-23 22:18:57]

On 2015/06/23 01:01:51, Rick Byers wrote:
> On 2015/06/17 15:18:59, tdresser wrote:
> >
>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent.idl
> > File Source/core/events/UIEvent.idl (right):
> > 
> >
>
https://codereview.chromium.org/1182313006/diff/1/Source/core/events/UIEvent....
> > Source/core/events/UIEvent.idl:36: [Default=Undefined] optional InputDevice?
> > sourceDevice);
> > On 2015/06/17 15:11:42, lanwei wrote:
> > > On 2015/06/16 19:51:41, tdresser wrote:
> > > > This should be behind the RuntimeEnabledFeature, shouldn't it?
> > > > 
> > > > Did we miss that elsewhere?
> > > 
> > > No, other places all have RuntimeEnabled=InputDevice, I am just not sure
how
> > to
> > > put this flag.
> > 
> > I think you need a completely separate constructor in the IDL, which is
hidden
> > behind a runtime enabled feature.
> 
> Ah, I was just asking about this in the MouseEvent CL.  I was hoping the
> bindings generator would be smart enough to let you do what you were trying
> here.  But actually, the 'init*Event' methods are really deprecated in favor
of
> event constructors (eg. see
>
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/initKeyboardEvent).
>  Perhaps we shouldn't change the init* methods at all and instead allow this
to
> be set via event constructors?  That would be simpler for your implementation.
> 
> The only problem is that TouchEvent doesn't (yet) have a constructor.  This
> would be good reason to define one (it's been on my TODO list).
> 
> WDYT?

I agree, init* method and constructors are redundant, so deprecating it is a
good idea. I will add the constructor to TouchEvent later.

[Rick Byers, 2015-06-25 17:12:30]

LGTM once rebased against https://codereview.chromium.org/1174683004/

https://codereview.chromium.org/1182313006/diff/60001/Source/core/input/Input...
File Source/core/input/InputDevice.h (right):

https://codereview.chromium.org/1182313006/diff/60001/Source/core/input/Input...
Source/core/input/InputDevice.h:23: static InputDevice*
doesntFireTouchEventsInputDevice();
This change is also in your MouseEvent CL, right?  I assume that will land first
then this (and changes in TouchEvent.cpp and InputDevice.cpp) will not be
present in this CL once rebased.

[tdresser, 2015-06-26 12:21:49]

On 2015/06/26 03:35:17, lanwei wrote:

LGTM, thanks!

[lanwei, 2015-06-26 14:47:16]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2015-06-26 14:47:17]

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

[commit-bot: I haz the power, 2015-06-26 14:47:29]

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

[commit-bot: I haz the power, 2015-06-26 14:50:34]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197921