Enable sending gesture events from layout tests

Enable sending gesture events from layout tests

BUG=451559
Committed: https://crrev.com/b81b80701a07ead3bf545a8dcda8dfe51c24409c
Cr-Commit-Position: refs/heads/master@{#313844}

[ccameron, 2015-01-29 20:33:16]

ccameron@chromium.org changed reviewers:
+ rbyers@chromium.org

[ccameron, 2015-01-29 20:33:16]

Very small change -- this allows me to add the layout test that verifies
synthetic event generation before moving synthetic event generation to Blink.

[Rick Byers, 2015-01-29 20:43:28]

https://codereview.chromium.org/886633006/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/886633006/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:2030: case
WebInputEvent::GesturePinchBegin:
don't you need a way to specify the source device (touchpad / touchscreen)? 
Looks like we are probably getting away without this for GestureScroll events -
probably because only touchscreen scroll events ever make it to blink so blink
might not even check the source (but ideally we'd be explicitly initializing the
sourceDevice for all GestureScroll events to TouchScreen).

If you don't actually need to differentiate them in your code or have just a
DCHECK to verify it's only touchpad (sorry been swamped today - haven't looked
yet), then it's find to just hardcode to one value here.  Otherwise you may need
to take an extra param to set the sourceDevice.

[ccameron, 2015-01-29 21:30:40]

Thanks -- actually, it turns out that it was undefined in Blink (eeeeeep) --
I've created an optional first argument for all gesture types.

https://codereview.chromium.org/886633006/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/886633006/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:2030: case
WebInputEvent::GesturePinchBegin:
On 2015/01/29 20:43:27, Rick Byers wrote:
> don't you need a way to specify the source device (touchpad / touchscreen)? 
> Looks like we are probably getting away without this for GestureScroll events
-
> probably because only touchscreen scroll events ever make it to blink so blink
> might not even check the source (but ideally we'd be explicitly initializing
the
> sourceDevice for all GestureScroll events to TouchScreen).
> 
> If you don't actually need to differentiate them in your code or have just a
> DCHECK to verify it's only touchpad (sorry been swamped today - haven't looked
> yet), then it's find to just hardcode to one value here.  Otherwise you may
need
> to take an extra param to set the sourceDevice.

Well, I'll want to test that touchscreen events don't produce synthetic events,
too, so I'll need a type.

I had empirically noticed that the type was always touchpad ... BUT it turns out
that was just undefined behavior. I've updated this so that all gestures are by
default touchpad, but if the first argument is a string, that string can be
"touchpad" or "touchscreen" to differentiate the two.

[Rick Byers, 2015-01-29 21:35:01]

LGTM if you can switch the default from touchpad to touchscreen.  Otherwise
let's talk options for fixing this up.  Thanks for digging and making this
better!

https://codereview.chromium.org/886633006/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/886633006/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1994: // argument was added).
are you sure about this part in the parens?  I believe the only type of gesture
events blink handles are touchscreen ones so all the existing code probably just
assumes it's touchscreen (touchpad fling gestures get handled by cc, but IIRC,
aren't passed to blink).  Doing this right probably means updating all existing
layout tests in fast/events/touch/gesture to specify  touchscreen.  Can you make
touchscreen the default instead?

[ccameron, 2015-01-29 21:54:09]

Thanks -- I'll check to see if the tests pass with touchscreen (& if they do
I'll submit).

https://codereview.chromium.org/886633006/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/886633006/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1994: // argument was added).
On 2015/01/29 21:35:01, Rick Byers wrote:
> are you sure about this part in the parens?  I believe the only type of
gesture
> events blink handles are touchscreen ones so all the existing code probably
just
> assumes it's touchscreen (touchpad fling gestures get handled by cc, but IIRC,
> aren't passed to blink).  Doing this right probably means updating all
existing
> layout tests in fast/events/touch/gesture to specify  touchscreen.  Can you
make
> touchscreen the default instead?

Oh -- I was going based on the undefined value's actual value (to the extent
that it was observed empirically), not the intention of the tests.

Updated the default to be touchscreen, and I'll see if the layout tests still
pass (if they don't, there are probably bugs to fix).

[Rick Byers, 2015-01-29 22:00:36]

On 2015/01/29 21:54:09, ccameron wrote:
> Thanks -- I'll check to see if the tests pass with touchscreen (& if they do
> I'll submit).
> 
>
https://codereview.chromium.org/886633006/diff/40001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/event_sender.cc (right):
> 
>
https://codereview.chromium.org/886633006/diff/40001/content/shell/renderer/t...
> content/shell/renderer/test_runner/event_sender.cc:1994: // argument was
added).
> On 2015/01/29 21:35:01, Rick Byers wrote:
> > are you sure about this part in the parens?  I believe the only type of
> gesture
> > events blink handles are touchscreen ones so all the existing code probably
> just
> > assumes it's touchscreen (touchpad fling gestures get handled by cc, but
IIRC,
> > aren't passed to blink).  Doing this right probably means updating all
> existing
> > layout tests in fast/events/touch/gesture to specify  touchscreen.  Can you
> make
> > touchscreen the default instead?
> 
> Oh -- I was going based on the undefined value's actual value (to the extent
> that it was observed empirically), not the intention of the tests.
> 
> Updated the default to be touchscreen, and I'll see if the layout tests still
> pass (if they don't, there are probably bugs to fix).

Thanks.  Yeah, I'll be really surprised if they don't pass.  I think the code
here mostly predates the addition of this field which was added for
chromium<->cc interaction around fling IIRC.

[Rick Byers, 2015-01-29 22:01:38]

still LGTM

https://codereview.chromium.org/886633006/diff/60001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/886633006/diff/60001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1993: // a number of of
pre-existing tests that made this assumption before this
nit: it's also because almost all uses of gesture events in blink are for
touchscreen scenarios :-)

[ccameron, 2015-01-30 00:08:27]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-01-30 00:09:16]

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

[commit-bot: I haz the power, 2015-01-30 00:55:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 00:56:09]

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