Send wheel gestures from event sender.

Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This conditional
code will eventually be removed once wheel gestures are enabled by
default for a full release. Event sender really should be sending
events up to the browser and this code wouldn't be necessary but
this is good for now. Log crbug.com/596095 to cover that.

Remove unused wheel scrolling code in event sender.

BUG=568183
Committed: https://crrev.com/43f18e9e7c90f40d6306f720791f60c1a09165dc
Cr-Commit-Position: refs/heads/master@{#382224}

[dtapuska, 2016-03-16 20:41:30]

Description was changed from

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This code will
eventually be removed once wheel gestures are enabled by default
for a full release.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

to

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This code will
eventually be removed once wheel gestures are enabled by default
for a full release.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

[dtapuska, 2016-03-16 20:41:30]

dtapuska@chromium.org changed reviewers:
+ mkwst@chromium.org, rbyers@chromium.org

[Mike West, 2016-03-17 16:19:37]

mkwst@chromium.org changed required reviewers:
+ rbyers@chromium.org

[Mike West, 2016-03-17 16:19:38]

It looks pretty reasonable to me, and the fact that the bots are still green is
a good sign. I'd like Rick to take a look, though, as he might actually be able
to judge whether the code is correct. :)

I'm happy to stamp //content/shell, but I'll defer the rest to Rick, % some
nits.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1942: void
EventSender::MouseScrollBy(gin::Arguments* args, bool continuous) {
Nit: I'd prefer for this to be an enum so that it's clear at the callsite what
"true" or "false" means.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1962: if
(begin_event.data.scrollBegin.deltaXHint)
Nit: {} around multi-line body.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1965: if
(begin_event.data.scrollBegin.deltaYHint)
Nit: {} around multi-line body.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1976: memset(&update_event.data, 0,
sizeof(update_event.data));
Could we move this to a `clear()` method on WebGestureEvent or something
similar? I don't like seeing direct memory operations scattered throughout the
codebase.

[dtapuska, 2016-03-17 18:05:52]

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1942: void
EventSender::MouseScrollBy(gin::Arguments* args, bool continuous) {
On 2016/03/17 16:19:38, Mike West wrote:
> Nit: I'd prefer for this to be an enum so that it's clear at the callsite what
> "true" or "false" means.

Done.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1962: if
(begin_event.data.scrollBegin.deltaXHint)
On 2016/03/17 16:19:38, Mike West wrote:
> Nit: {} around multi-line body.

Done.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1965: if
(begin_event.data.scrollBegin.deltaYHint)
On 2016/03/17 16:19:38, Mike West wrote:
> Nit: {} around multi-line body.

Done.

https://codereview.chromium.org/1806103002/diff/1/components/test_runner/even...
components/test_runner/event_sender.cc:1976: memset(&update_event.data, 0,
sizeof(update_event.data));
On 2016/03/17 16:19:38, Mike West wrote:
> Could we move this to a `clear()` method on WebGestureEvent or something
> similar? I don't like seeing direct memory operations scattered throughout the
> codebase.

I've just cloned the fields over manually and avoided the base object cloning.

[Rick Byers, 2016-03-18 15:36:25]

Logic looks good.  Please clarify the CL description that it's just the
conditional bit that will go away (making this on by default).

https://codereview.chromium.org/1806103002/diff/20001/components/test_runner/...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1806103002/diff/20001/components/test_runner/...
components/test_runner/event_sender.cc:1952: begin_event.sourceDevice =
blink::WebGestureDeviceTouchpad;
Generally our approach in eventSender is to generate only the events asked for
by the test (leaving it to the test to define exactly what the event stream
should look like).  But given the conditional nature, I agree this is the right
short term choice.  Longer term most tests should probably be migrated to use
browser-level synthetic input or (when we really want a blink input-handling
unit test) the test should have to explicitly request the wheel and gesture
events separately.

Can you please file a bug to track that cleanup and add a TODO here referencing
it?  In fact, it would probably be a little nicer if you put all the synthetic
wheel generation logic into it's own function (since the primary purpose of this
function is still just sending a wheel event).

You could probably also make this a little less verbose with a helper that
creates a WebGestureEvent from a WebMouseWheelEvent with the common bits, but
that's not a big deal - up to you.

[dtapuska, 2016-03-18 17:04:20]

Description was changed from

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This code will
eventually be removed once wheel gestures are enabled by default
for a full release.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

to

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This conditional
code will eventually be removed once wheel gestures are enabled by
default for a full release. Event sender really should be sending
events up to the browser and this code wouldn't be necessary but
this is good for now. Log crbug.com/596095 to cover that.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

[dtapuska, 2016-03-18 17:04:43]

https://codereview.chromium.org/1806103002/diff/20001/components/test_runner/...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1806103002/diff/20001/components/test_runner/...
components/test_runner/event_sender.cc:1952: begin_event.sourceDevice =
blink::WebGestureDeviceTouchpad;
On 2016/03/18 15:36:25, Rick Byers wrote:
> Generally our approach in eventSender is to generate only the events asked for
> by the test (leaving it to the test to define exactly what the event stream
> should look like).  But given the conditional nature, I agree this is the
right
> short term choice.  Longer term most tests should probably be migrated to use
> browser-level synthetic input or (when we really want a blink input-handling
> unit test) the test should have to explicitly request the wheel and gesture
> events separately.
> 
> Can you please file a bug to track that cleanup and add a TODO here
referencing
> it?  In fact, it would probably be a little nicer if you put all the synthetic
> wheel generation logic into it's own function (since the primary purpose of
this
> function is still just sending a wheel event).
> 
> You could probably also make this a little less verbose with a helper that
> creates a WebGestureEvent from a WebMouseWheelEvent with the common bits, but
> that's not a big deal - up to you.

Done.

[Rick Byers, 2016-03-18 19:34:11]

LGTM

[Mike West, 2016-03-19 06:07:32]

LGTM 2. Thanks Rick!

[dtapuska, 2016-03-20 18:08:04]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-03-20 18:08:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806103002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1806103002/60001

[commit-bot: I haz the power, 2016-03-20 20:21:54]

Description was changed from

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This conditional
code will eventually be removed once wheel gestures are enabled by
default for a full release. Event sender really should be sending
events up to the browser and this code wouldn't be necessary but
this is good for now. Log crbug.com/596095 to cover that.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

to

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This conditional
code will eventually be removed once wheel gestures are enabled by
default for a full release. Event sender really should be sending
events up to the browser and this code wouldn't be necessary but
this is good for now. Log crbug.com/596095 to cover that.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

[commit-bot: I haz the power, 2016-03-20 20:21:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-20 20:23:01]

Description was changed from

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This conditional
code will eventually be removed once wheel gestures are enabled by
default for a full release. Event sender really should be sending
events up to the browser and this code wouldn't be necessary but
this is good for now. Log crbug.com/596095 to cover that.

Remove unused wheel scrolling code in event sender.

BUG=568183
==========

to

==========
Send wheel gestures from event sender.

When wheel gestures are enabled on the content shell send them in the
event sender when mouse wheel scrolls occur. This conditional
code will eventually be removed once wheel gestures are enabled by
default for a full release. Event sender really should be sending
events up to the browser and this code wouldn't be necessary but
this is good for now. Log crbug.com/596095 to cover that.

Remove unused wheel scrolling code in event sender.

BUG=568183

Committed: https://crrev.com/43f18e9e7c90f40d6306f720791f60c1a09165dc
Cr-Commit-Position: refs/heads/master@{#382224}
==========

[commit-bot: I haz the power, 2016-03-20 20:23:03]

Patchset 4 (id:??) landed as
https://crrev.com/43f18e9e7c90f40d6306f720791f60c1a09165dc
Cr-Commit-Position: refs/heads/master@{#382224}