Add rails to input wheel events.

Add rails to input wheel events.

Allow wheel events to specify that they wish to be on horizontal or
vertical rails. Honor this specification in wheel event scrolling code
in ScrollableArea and PinchViewport.

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

[Rick Byers, 2015-03-18 23:30:57]

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

[Rick Byers, 2015-03-18 23:30:58]

https://codereview.chromium.org/1018183002/diff/1/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1018183002/diff/1/public/web/WebInputEvent.h#...
public/web/WebInputEvent.h:349: enum RailMode {
We'll want to use this from WebGestureEvent too I think.  Perhaps we should move
the definition into WebInputEvent.h?

https://codereview.chromium.org/1018183002/diff/1/public/web/WebInputEvent.h#...
public/web/WebInputEvent.h:351: RailModeHorizontal = 1 << 0,
these don't need to be flags, right?  I.e. it's not legal to specify
'RailModeHorizontal | RailModeVertical', right?

[ccameron, 2015-03-19 00:05:11]

Good point -- updated.

https://codereview.chromium.org/1018183002/diff/1/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1018183002/diff/1/public/web/WebInputEvent.h#...
public/web/WebInputEvent.h:349: enum RailMode {
On 2015/03/18 23:30:58, Rick Byers wrote:
> We'll want to use this from WebGestureEvent too I think.  Perhaps we should
move
> the definition into WebInputEvent.h?

Good point -- done. Did this to the platform events too.

https://codereview.chromium.org/1018183002/diff/1/public/web/WebInputEvent.h#...
public/web/WebInputEvent.h:351: RailModeHorizontal = 1 << 0,
On 2015/03/18 23:30:58, Rick Byers wrote:
> these don't need to be flags, right?  I.e. it's not legal to specify
> 'RailModeHorizontal | RailModeVertical', right?

The same is true of Phase, so I was guessing this to be a style thing. Changed
to non-flag-values.

[ccameron, 2015-03-19 22:43:37]

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

[ccameron, 2015-03-19 22:43:38]

Ping on this, and adding kbr@ for OWNER of platform+web

[Ken Russell (switch to Gerrit), 2015-03-20 02:35:21]

LGTM overall

https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
File Source/web/WebInputEvent.cpp (right):

https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
Source/web/WebInputEvent.cpp:54: int mousewheelData[11];
These magic constants are awful. Can you think of a better way to handle them?

[ccameron, 2015-03-20 18:45:27]

Thanks!

https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
File Source/web/WebInputEvent.cpp (right):

https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
Source/web/WebInputEvent.cpp:54: int mousewheelData[11];
On 2015/03/20 02:35:21, Ken Russell wrote:
> These magic constants are awful. Can you think of a better way to handle them?

This does seem pretty terrible -- do we know why this is even here? There's no
comment anywhere explaining this bizarre check.

[Rick Byers, 2015-03-20 18:53:52]

I think there's code somewhere that creates a WebMouseWheelEvent from a
PlatformMouseWheelEvent (inverse of PlatformMouseWheelEventBuilder), right? 
That will need to be updated as well (probably could break webview tag behavior
otherwise).  +lazyboy@ who knows about webview tag issues.

Since you're updating WebInputEventConversion you should probably be also adding
a test to WebInputEventConversionTest.

Otherwise looks good.

https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
File Source/web/WebInputEvent.cpp (right):

https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
Source/web/WebInputEvent.cpp:54: int mousewheelData[11];
On 2015/03/20 18:45:27, ccameron wrote:
> On 2015/03/20 02:35:21, Ken Russell wrote:
> > These magic constants are awful. Can you think of a better way to handle
them?
> 
> This does seem pretty terrible -- do we know why this is even here? There's no
> comment anywhere explaining this bizarre check.

I always assumed they were just to ensure we get compile time failures if
someone accidentally increases the size of an IPC message.  I don't know where
they came from or if they're really serving any useful purpose.  Feel free to
file a bug and I can ask someone on input-dev to dig in and explore removing
them.  We recently removed another ugly IPC hack where for some reason we had
been avoiding 'bool' types for no good reason.  Could easily be WebKit legacy
somehow.

[Rick Byers, 2015-03-20 18:57:05]

On 2015/03/20 18:53:52, Rick Byers wrote:
> I think there's code somewhere that creates a WebMouseWheelEvent from a
> PlatformMouseWheelEvent (inverse of PlatformMouseWheelEventBuilder), right? 
> That will need to be updated as well (probably could break webview tag
behavior
> otherwise).  +lazyboy@ who knows about webview tag issues.

Actually, as for https://codereview.chromium.org/920523003/ I think you may need
to plumb this down to the DOM WheelEvent object to avoid risking breaking
webview.  It's possible that it would be OK for that to come in a later CL
though.  lazyboy@ or fsamuel@ are the experts on webview and input implications.

> 
> Since you're updating WebInputEventConversion you should probably be also
adding
> a test to WebInputEventConversionTest.
> 
> Otherwise looks good.
> 
>
https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
> File Source/web/WebInputEvent.cpp (right):
> 
>
https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
> Source/web/WebInputEvent.cpp:54: int mousewheelData[11];
> On 2015/03/20 18:45:27, ccameron wrote:
> > On 2015/03/20 02:35:21, Ken Russell wrote:
> > > These magic constants are awful. Can you think of a better way to handle
> them?
> > 
> > This does seem pretty terrible -- do we know why this is even here? There's
no
> > comment anywhere explaining this bizarre check.
> 
> I always assumed they were just to ensure we get compile time failures if
> someone accidentally increases the size of an IPC message.  I don't know where
> they came from or if they're really serving any useful purpose.  Feel free to
> file a bug and I can ask someone on input-dev to dig in and explore removing
> them.  We recently removed another ugly IPC hack where for some reason we had
> been avoiding 'bool' types for no good reason.  Could easily be WebKit legacy
> somehow.

[lazyboy, 2015-03-20 21:00:33]

On 2015/03/20 18:57:05, Rick Byers wrote:
> On 2015/03/20 18:53:52, Rick Byers wrote:
> > I think there's code somewhere that creates a WebMouseWheelEvent from a
> > PlatformMouseWheelEvent (inverse of PlatformMouseWheelEventBuilder), right? 
> > That will need to be updated as well (probably could break webview tag
> behavior
> > otherwise).  +lazyboy@ who knows about webview tag issues.
> 
> Actually, as for https://codereview.chromium.org/920523003/ I think you may
need
> to plumb this down to the DOM WheelEvent object to avoid risking breaking
> webview.  It's possible that it would be OK for that to come in a later CL
> though.  lazyboy@ or fsamuel@ are the experts on webview and input
implications.
Yes, that would be great.
I agree that WebMouseWheelEvent not including the field will cause bugs for
<webview>,
even if it works today without the field.
> 
> > 
> > Since you're updating WebInputEventConversion you should probably be also
> adding
> > a test to WebInputEventConversionTest.
> > 
> > Otherwise looks good.
> > 
> >
>
https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
> > File Source/web/WebInputEvent.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1018183002/diff/20001/Source/web/WebInputEven...
> > Source/web/WebInputEvent.cpp:54: int mousewheelData[11];
> > On 2015/03/20 18:45:27, ccameron wrote:
> > > On 2015/03/20 02:35:21, Ken Russell wrote:
> > > > These magic constants are awful. Can you think of a better way to handle
> > them?
> > > 
> > > This does seem pretty terrible -- do we know why this is even here?
There's
> no
> > > comment anywhere explaining this bizarre check.
> > 
> > I always assumed they were just to ensure we get compile time failures if
> > someone accidentally increases the size of an IPC message.  I don't know
where
> > they came from or if they're really serving any useful purpose.  Feel free
to
> > file a bug and I can ask someone on input-dev to dig in and explore removing
> > them.  We recently removed another ugly IPC hack where for some reason we
had
> > been avoiding 'bool' types for no good reason.  Could easily be WebKit
legacy
> > somehow.

[ccameron, 2015-03-20 21:03:37]

Sounds good -- added the extra plumbing and tests (umm, and found some sad bugs
in the existing tests) -- ptal.

[Rick Byers, 2015-03-20 21:29:05]

Thanks LGTM.

Changes like this really make the 3 layers of event abstractions obviously silly
(I've been cutting corners lately blending them a little - perhaps we'll
eliminate the Platform* layer at some point).

[ccameron, 2015-03-20 21:57:07]

Thanks!

I see that the use of powers of 2 for non-combinable enums is everywhere around
here -- I suspect it has something about wanting there to be no "gaps" in
structures ... I may try nuking that soon, because it is upsetting.

[ccameron, 2015-03-20 21:57:16]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-03-20 21:57:17]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1018183002/#ps40001
(title: "More plumbing and tests")

[commit-bot: I haz the power, 2015-03-20 21:58:09]

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

[commit-bot: I haz the power, 2015-03-21 00:01:19]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192298