Allow postponed rail application for touch scrolling - blink side.

Allow postponed rail application for touch scrolling - blink side.

This is the first of several CLs to postpone rail application for
touch scrolling. The following CL (https://codereview.chromium.org/1049383003/) will pass the
railsMode correctly to blink, and apply the rails in the
InputHandlerProxy when necessary, aligning the behavior with touchpad
rail on Mac.

BUG=469789

[tdresser, 2015-04-01 17:06:37]

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

[tdresser, 2015-04-01 17:06:37]

ccameron@, can you take a high level look at this and
https://codereview.chromium.org/1049383003/, to make sure the approach looks
sane to you?

I'll get an OWNERS review here once you're happy with the approach.

[ccameron, 2015-04-01 17:26:52]

On 2015/04/01 17:06:37, tdresser wrote:
> ccameron@, can you take a high level look at this and
> https://codereview.chromium.org/1049383003/, to make sure the approach looks
> sane to you?
> 
> I'll get an OWNERS review here once you're happy with the approach.

This part lgtm. For the other one, I have a limited background on the geometry
of the events, so I'm not too useful for the bulk of it.

[tdresser, 2015-04-01 17:29:20]

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

[tdresser, 2015-04-01 17:29:21]

kbr@, can you take a look at Source/*?
rbyers@, can you take a look at public/*?

Thanks!

[Rick Byers, 2015-04-01 17:39:53]

public/ LGTM

[Ken Russell (switch to Gerrit), 2015-04-01 18:33:12]

Looks good overall modulo nits, but where are the tests for this feature? It
seems to me it should be possible to write a test in
Source/web/tests/WebViewTest.cpp.

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

https://codereview.chromium.org/1052433003/diff/20001/Source/web/WebInputEven...
Source/web/WebInputEvent.cpp:58: int gestureData[11];
This magic constant is awful. Can you figure out a way to compute or define it
differently?

https://codereview.chromium.org/1052433003/diff/20001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/1052433003/diff/20001/Source/web/WebViewImpl....
Source/web/WebViewImpl.h:755: WebInputEvent::RailsMode m_railStateOnFlingStart;
This needs to be initialized in the right place in WebViewImpl's initializer
list.

[jdduke (slow), 2015-04-02 16:55:53]

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

[jdduke (slow), 2015-04-02 16:55:53]

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

https://codereview.chromium.org/1052433003/diff/20001/public/web/WebInputEven...
public/web/WebInputEvent.h:483: RailsMode railsMode;
I can't say I'm thrilled with this change. Now, every handler of any kind of
scroll event will have to monitor the rails mode and make the right call (e.g.,
looking at the rails definition to make sure RailsModeHorizontal means snap to
the x-axis, not truncate the x-axis). We're effectively placing the burden on
the common case to support the exceptional case (JS-based scroll customization).

For me the ideal would be to bundle the "filtered" values, as they are now, with
the unfiltered or raw values, then have some bit flags that indicate what kind
of filters was applied, e.g., rails, or slop removal, to map the raw to the
actual. Maybe that's naive, but it makes it a lot more difficult to get things
wrong, and I can think of 7 or 8 places off the top of my head where this change
will break behavior in Chrome.

[ccameron, 2015-04-02 18:49:53]

On 2015/04/02 16:55:53, jdduke wrote:
>
https://codereview.chromium.org/1052433003/diff/20001/public/web/WebInputEvent.h
> File public/web/WebInputEvent.h (right):
> 
>
https://codereview.chromium.org/1052433003/diff/20001/public/web/WebInputEven...
> public/web/WebInputEvent.h:483: RailsMode railsMode;
> I can't say I'm thrilled with this change. Now, every handler of any kind of
> scroll event will have to monitor the rails mode and make the right call
(e.g.,
> looking at the rails definition to make sure RailsModeHorizontal means snap to
> the x-axis, not truncate the x-axis). We're effectively placing the burden on
> the common case to support the exceptional case (JS-based scroll
customization).
> 
> For me the ideal would be to bundle the "filtered" values, as they are now,
with
> the unfiltered or raw values, then have some bit flags that indicate what kind
> of filters was applied, e.g., rails, or slop removal, to map the raw to the
> actual. Maybe that's naive, but it makes it a lot more difficult to get things
> wrong, and I can think of 7 or 8 places off the top of my head where this
change
> will break behavior in Chrome.

I'm inclined to agree -- that way I could just toss out the patch in
https://codereview.chromium.org/1052773002/.

So -- something to the effect of
- event.railedDeltaX
- event.railedDeltaY
- event.railsMode

[aelias_OOO_until_Jul13, 2015-04-02 20:55:58]

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

[aelias_OOO_until_Jul13, 2015-04-02 20:55:59]

In Jared's model, what would event.railsMode be used for?  Is it needed at all?

[jdduke (slow), 2015-04-02 21:27:35]

On 2015/04/02 20:55:59, aelias wrote:
> In Jared's model, what would event.railsMode be used for?  Is it needed at
all?

I *think* the idea was that a scroll customizer may want to tailor the rails
behavior, so they should receive the raw values in some form to be allowed that.
However, there's no mention of this in the scroll customization doc
(https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...),
and I can't seem to find the email thread where this was discussed (actually, it
may have been an input-dev meeting where I proposed exposing the filtered and
raw values both, but that idea was dismissed in favor of piping the raw values
with an indicator of the rails state).

[tdresser, 2015-04-07 13:30:48]

On 2015/04/02 21:27:35, jdduke wrote:
> On 2015/04/02 20:55:59, aelias wrote:
> > In Jared's model, what would event.railsMode be used for?  Is it needed at
> all?
> 
> I *think* the idea was that a scroll customizer may want to tailor the rails
> behavior, so they should receive the raw values in some form to be allowed
that.
> However, there's no mention of this in the scroll customization doc
>
(https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...),
> and I can't seem to find the email thread where this was discussed (actually,
it
> may have been an input-dev meeting where I proposed exposing the filtered and
> raw values both, but that idea was dismissed in favor of piping the raw values
> with an indicator of the rails state).

The scroll customization doc does mention this briefly:
https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...

The reason I believe it makes more sense to pass the railsMode and the unrailed
deltas is that partial scroll consumption is much harder to define if you pass
the railed and unrailed deltas separately.

Suppose I have a scroller which doesn't apply rails within a scroller which does
apply rails.
If I consume some of the unrailed deltas, what impact should that have on the
railed deltas?

I agree that it's annoying to have to update all the consumers of
GestureScrollUpdate events, but I think it's worth it to obtain a more rational
api for scroll customization.

[jdduke (slow), 2015-04-07 14:18:19]

On 2015/04/07 13:30:48, tdresser wrote:
> The scroll customization doc does mention this briefly:
>
https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...
> 
> The reason I believe it makes more sense to pass the railsMode and the
unrailed
> deltas is that partial scroll consumption is much harder to define if you pass
> the railed and unrailed deltas separately.
> 
> Suppose I have a scroller which doesn't apply rails within a scroller which
does
> apply rails.
> If I consume some of the unrailed deltas, what impact should that have on the
> railed deltas?
> 
> I agree that it's annoying to have to update all the consumers of
> GestureScrollUpdate events, but I think it's worth it to obtain a more
rational
> api for scroll customization.

WebGestureEvent details needn't map perfectly to the proposed scroll API. As
long as we have all the necessary bits, I'd much prefer we put them in a
packaged form that is hard to get wrong at the point of application, e.g., have
unrailedDeltaX or unfilteredDeltaX members in addition to the deltaX member,
and/or getters that give us the railed values. 

It's not that it's annoying to update all the consumers, it's that it increases
the surface area for getting things wrong. I suspect that the common case with
scroll customization will be to use the rails data, in which case it's not clear
that we'll want every embedder to do rails application themselves.

[tdresser, 2015-04-07 14:26:51]

On 2015/04/07 14:18:19, jdduke wrote:
> On 2015/04/07 13:30:48, tdresser wrote:
> > The scroll customization doc does mention this briefly:
> >
>
https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...
> > 
> > The reason I believe it makes more sense to pass the railsMode and the
> unrailed
> > deltas is that partial scroll consumption is much harder to define if you
pass
> > the railed and unrailed deltas separately.
> > 
> > Suppose I have a scroller which doesn't apply rails within a scroller which
> does
> > apply rails.
> > If I consume some of the unrailed deltas, what impact should that have on
the
> > railed deltas?
> > 
> > I agree that it's annoying to have to update all the consumers of
> > GestureScrollUpdate events, but I think it's worth it to obtain a more
> rational
> > api for scroll customization.
> 
> WebGestureEvent details needn't map perfectly to the proposed scroll API. As
> long as we have all the necessary bits, I'd much prefer we put them in a
> packaged form that is hard to get wrong at the point of application, e.g.,
have
> unrailedDeltaX or unfilteredDeltaX members in addition to the deltaX member,
> and/or getters that give us the railed values. 
> 
> It's not that it's annoying to update all the consumers, it's that it
increases
> the surface area for getting things wrong. I suspect that the common case with
> scroll customization will be to use the rails data, in which case it's not
clear
> that we'll want every embedder to do rails application themselves.

Yeah, that sounds reasonable to me, though in the long term many consumers of
GestureScrollUpdate may switch to being implemented in terms of the scroll
customization primitives.

There are also some cases where rails are applied where they don't necessarily
make sense. In the ui side, things like window dragging currently have rails
applied, which feels somewhat strange. This approach will allow us to opt out of
rails there, which is nice.

For now, it sounds like the best path forward is for scroll events to have:
railed deltas
unrailed deltas
railsMode

The ScrollState will only take the unrailed deltas and the railsMode.

Sound good?

[Rick Byers, 2015-04-07 14:59:44]

On 2015/04/07 14:26:51, tdresser wrote:
> On 2015/04/07 14:18:19, jdduke wrote:
> > On 2015/04/07 13:30:48, tdresser wrote:
> > > The scroll customization doc does mention this briefly:
> > >
> >
>
https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...
> > > 
> > > The reason I believe it makes more sense to pass the railsMode and the
> > unrailed
> > > deltas is that partial scroll consumption is much harder to define if you
> pass
> > > the railed and unrailed deltas separately.
> > > 
> > > Suppose I have a scroller which doesn't apply rails within a scroller
which
> > does
> > > apply rails.
> > > If I consume some of the unrailed deltas, what impact should that have on
> the
> > > railed deltas?
> > > 
> > > I agree that it's annoying to have to update all the consumers of
> > > GestureScrollUpdate events, but I think it's worth it to obtain a more
> > rational
> > > api for scroll customization.
> > 
> > WebGestureEvent details needn't map perfectly to the proposed scroll API. As
> > long as we have all the necessary bits, I'd much prefer we put them in a
> > packaged form that is hard to get wrong at the point of application, e.g.,
> have
> > unrailedDeltaX or unfilteredDeltaX members in addition to the deltaX member,
> > and/or getters that give us the railed values. 
> > 
> > It's not that it's annoying to update all the consumers, it's that it
> increases
> > the surface area for getting things wrong. I suspect that the common case
with
> > scroll customization will be to use the rails data, in which case it's not
> clear
> > that we'll want every embedder to do rails application themselves.
> 
> Yeah, that sounds reasonable to me, though in the long term many consumers of
> GestureScrollUpdate may switch to being implemented in terms of the scroll
> customization primitives.
> 
> There are also some cases where rails are applied where they don't necessarily
> make sense. In the ui side, things like window dragging currently have rails
> applied, which feels somewhat strange. This approach will allow us to opt out
of
> rails there, which is nice.
> 
> For now, it sounds like the best path forward is for scroll events to have:
> railed deltas
> unrailed deltas
> railsMode
> 
> The ScrollState will only take the unrailed deltas and the railsMode.
> 
> Sound good?

The algorithm for computing the railed deltas from the unrailed deltas and rail
mode is trivial (i.e. not dependent on any GR details), right?  How would the
above design be different from one where instead of having railed deltas in the
event, we just have methods that compute and return the railed deltas?

I'm just trying to avoid an increase in the state space.

[tdresser, 2015-04-07 16:06:51]

On 2015/04/07 14:59:44, Rick Byers (Out until 4-7) wrote:
> On 2015/04/07 14:26:51, tdresser wrote:
> > On 2015/04/07 14:18:19, jdduke wrote:
> > > On 2015/04/07 13:30:48, tdresser wrote:
> > > > The scroll customization doc does mention this briefly:
> > > >
> > >
> >
>
https://docs.google.com/document/d/1VnvAqeWFG9JFZfgG5evBqrLGDZYRE5w6G5jEDORek...
> > > > 
> > > > The reason I believe it makes more sense to pass the railsMode and the
> > > unrailed
> > > > deltas is that partial scroll consumption is much harder to define if
you
> > pass
> > > > the railed and unrailed deltas separately.
> > > > 
> > > > Suppose I have a scroller which doesn't apply rails within a scroller
> which
> > > does
> > > > apply rails.
> > > > If I consume some of the unrailed deltas, what impact should that have
on
> > the
> > > > railed deltas?
> > > > 
> > > > I agree that it's annoying to have to update all the consumers of
> > > > GestureScrollUpdate events, but I think it's worth it to obtain a more
> > > rational
> > > > api for scroll customization.
> > > 
> > > WebGestureEvent details needn't map perfectly to the proposed scroll API.
As
> > > long as we have all the necessary bits, I'd much prefer we put them in a
> > > packaged form that is hard to get wrong at the point of application, e.g.,
> > have
> > > unrailedDeltaX or unfilteredDeltaX members in addition to the deltaX
member,
> > > and/or getters that give us the railed values. 
> > > 
> > > It's not that it's annoying to update all the consumers, it's that it
> > increases
> > > the surface area for getting things wrong. I suspect that the common case
> with
> > > scroll customization will be to use the rails data, in which case it's not
> > clear
> > > that we'll want every embedder to do rails application themselves.
> > 
> > Yeah, that sounds reasonable to me, though in the long term many consumers
of
> > GestureScrollUpdate may switch to being implemented in terms of the scroll
> > customization primitives.
> > 
> > There are also some cases where rails are applied where they don't
necessarily
> > make sense. In the ui side, things like window dragging currently have rails
> > applied, which feels somewhat strange. This approach will allow us to opt
out
> of
> > rails there, which is nice.
> > 
> > For now, it sounds like the best path forward is for scroll events to have:
> > railed deltas
> > unrailed deltas
> > railsMode
> > 
> > The ScrollState will only take the unrailed deltas and the railsMode.
> > 
> > Sound good?
> 
> The algorithm for computing the railed deltas from the unrailed deltas and
rail
> mode is trivial (i.e. not dependent on any GR details), right?  How would the
> above design be different from one where instead of having railed deltas in
the
> event, we just have methods that compute and return the railed deltas?
> 
> I'm just trying to avoid an increase in the state space.

Sounds good to me.

[jdduke (slow), 2015-04-07 16:20:44]

On 2015/04/07 14:59:44, Rick Byers (Out until 4-7) wrote:
> The algorithm for computing the railed deltas from the unrailed deltas and
rail
> mode is trivial (i.e. not dependent on any GR details), right?  How would the
> above design be different from one where instead of having railed deltas in
the
> event, we just have methods that compute and return the railed deltas?
> 
> I'm just trying to avoid an increase in the state space.

Right, that's why I mentioned "getters that give us the railed values".

[Rick Byers, 2015-04-07 21:06:12]

On 2015/04/07 16:20:44, jdduke wrote:
> On 2015/04/07 14:59:44, Rick Byers (Out until 4-7) wrote:
> > The algorithm for computing the railed deltas from the unrailed deltas and
> rail
> > mode is trivial (i.e. not dependent on any GR details), right?  How would
the
> > above design be different from one where instead of having railed deltas in
> the
> > event, we just have methods that compute and return the railed deltas?
> > 
> > I'm just trying to avoid an increase in the state space.
> 
> Right, that's why I mentioned "getters that give us the railed values".

Ah, sorry - I missed that.  Great, I'd definitely prefer to avoid tracking two
sets of deltas, but having two different views into the same state seems great.