gesture: Include velocity in scroll-update gestures.

gesture: Include velocity in scroll-update gestures.

BUG=143244
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152369

[rjkroege, 2012-08-20 14:52:17]

Do we need the friend stuff? I would hope that there would be a nicer way to do
this.

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:276: float CalibrateVelocity(float
velocity) {
thanks for pulling this out. Since odds are that we'll be tinkering with it for
days.

nit: Could we call this CalibrateFlingVelocity? Because it's for fling only
right?

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types.h
File ui/base/gestures/gesture_types.h (right):

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types...
ui/base/gestures/gesture_types.h:89: friend class GestureSequence;
grumble.

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types...
ui/base/gestures/gesture_types.h:109: } velocity;
it would be nice to align these fields with the union change in WebGestureEvent
yes?

[sadrul, 2012-08-20 15:48:34]

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
File ui/base/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
ui/base/gestures/gesture_sequence.cc:276: float CalibrateVelocity(float
velocity) {
On 2012/08/20 14:52:17, rjkroege wrote:
> thanks for pulling this out. Since odds are that we'll be tinkering with it
for
> days.
> 
> nit: Could we call this CalibrateFlingVelocity? Because it's for fling only
> right?

We are also using the same calibration for the scroll-update velocities.

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types.h
File ui/base/gestures/gesture_types.h (right):

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types...
ui/base/gestures/gesture_types.h:89: friend class GestureSequence;
On 2012/08/20 14:52:17, rjkroege wrote:
> grumble.

This is mostly to allow only GestureSequence to be able to set these parameters.
Perhaps it makes sense to just make these public instead?

http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types...
ui/base/gestures/gesture_types.h:109: } velocity;
On 2012/08/20 14:52:17, rjkroege wrote:
> it would be nice to align these fields with the union change in
WebGestureEvent
> yes?

I think these match the change in WebGestuerEvent
(http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebInputEv...)

[rjkroege, 2012-08-20 17:33:48]

On 2012/08/20 15:48:34, sadrul wrote:
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
> File ui/base/gestures/gesture_sequence.cc (right):
> 
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
> ui/base/gestures/gesture_sequence.cc:276: float CalibrateVelocity(float
> velocity) {
> On 2012/08/20 14:52:17, rjkroege wrote:
> > thanks for pulling this out. Since odds are that we'll be tinkering with it
> for
> > days.
> > 
> > nit: Could we call this CalibrateFlingVelocity? Because it's for fling only
> > right?
> 
> We are also using the same calibration for the scroll-update velocities.

We must not. The velocities in ScrollUpdate need to be real.

> 
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types.h
> File ui/base/gestures/gesture_types.h (right):
> 
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types...
> ui/base/gestures/gesture_types.h:89: friend class GestureSequence;
> On 2012/08/20 14:52:17, rjkroege wrote:
> > grumble.
> 
> This is mostly to allow only GestureSequence to be able to set these
parameters.
> Perhaps it makes sense to just make these public instead?

I would have a tentative yes to that in preference to friends. Friend shares
everything. :-)

> 
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_types...
> ui/base/gestures/gesture_types.h:109: } velocity;
> On 2012/08/20 14:52:17, rjkroege wrote:
> > it would be nice to align these fields with the union change in
> WebGestureEvent
> > yes?
> 
> I think these match the change in WebGestuerEvent
>
(http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebInputEv...)

scroll vs scrollUpdate, flingStart vs velocity? We could have better alignment?

[sadrul, 2012-08-20 18:08:28]

Addressed the comments. PTAL

On 2012/08/20 17:33:48, rjkroege wrote:
> On 2012/08/20 15:48:34, sadrul wrote:
> >
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
> > File ui/base/gestures/gesture_sequence.cc (right):
> > 
> >
>
http://codereview.chromium.org/10837329/diff/1/ui/base/gestures/gesture_seque...
> > ui/base/gestures/gesture_sequence.cc:276: float CalibrateVelocity(float
> > velocity) {
> > On 2012/08/20 14:52:17, rjkroege wrote:
> > > thanks for pulling this out. Since odds are that we'll be tinkering with
it
> > for
> > > days.
> > > 
> > > nit: Could we call this CalibrateFlingVelocity? Because it's for fling
only
> > > right?
> > 
> > We are also using the same calibration for the scroll-update velocities.
> 
> We must not. The velocities in ScrollUpdate need to be real.

Ah, I see. Fixed.

> > > grumble.
> > 
> > This is mostly to allow only GestureSequence to be able to set these
> parameters.
> > Perhaps it makes sense to just make these public instead?
> 
> I would have a tentative yes to that in preference to friends. Friend shares
> everything. :-)

Fair enough. Made the API public instead.

> > > it would be nice to align these fields with the union change in
> > WebGestureEvent
> > > yes?
> > 
> > I think these match the change in WebGestuerEvent
> >
>
(http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebInputEv...)
> 
> scroll vs scrollUpdate, flingStart vs velocity? We could have better
alignment?

Ah, I misunderstood. Fixed.

[rjkroege, 2012-08-20 18:21:17]

lgtm

Nice.

LGTM