Bug 95997 – [chromium] Add touchscreen specific fling curve parametrization

• Component: UI Events
• Status: RESOLVED FIXED

[Robert Kroeger]

Add an additional fling curve parametrization specific to touchscreen fling.

[Robert Kroeger]

Created attachment 162545 [details]
Patch

[Robert Kroeger]

Comment on attachment 162545 [details]
Patch

jamesr@: please take a look.

[James Robinson]

Comment on attachment 162545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162545&action=review

> Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.cpp:54
> +        return WebCore::TouchpadFlingPlatformGestureCurve::createForTouchScreen(point, cumulativeScroll);

Touchpad...ForTouchScreen?  What does that mean?  It doesn't seem sensical to create a Touchpad fling curve for a touch screen.

I think what you really want is another fling curve type, if you want to be consistent with the existing curve type hierarchy.  Or refactor the curve stuff to make more sense.

[W. James MacLean]

(In reply to comment #3)
> (From update of attachment 162545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162545&action=review
> 
> > Source/WebCore/platform/chromium/support/PlatformGestureCurveFactory.cpp:54
> > +        return WebCore::TouchpadFlingPlatformGestureCurve::createForTouchScreen(point, cumulativeScroll);
> 
> Touchpad...ForTouchScreen?  What does that mean?  It doesn't seem sensical to create a Touchpad fling curve for a touch screen.
> 
> I think what you really want is another fling curve type, if you want to be consistent with the existing curve type hierarchy.  Or refactor the curve stuff to make more sense.

What if we renamed TouchpadFlingPlatformGestureCurve to TouchFlingPlatformGestureCurve and had functions

TouchFlingPlatformGestureCurve::createForTouchPad()
TouchFlingPlatformGestureCurve::createForTouchScreen()

?

[James Robinson]

That sounds better.

[Robert Kroeger]

sounds good to me. I'll update the patch.

[Robert Kroeger]

Created attachment 162824 [details]
Patch

[Robert Kroeger]

please take another look.

[James Robinson]

Comment on attachment 162824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162824&action=review

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:69
> +// the in-flight patch for https://bugs.webkit.org/show_bug.cgi?id=81663 .

I know you're just moving this code but this FIXME and comment are sad.  https://bugs.webkit.org/show_bug.cgi?id=81663 was abandoned a while ago, do we still need these extra parameters?

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:56
> +    float m_coeffs[5];

No abbreviations in webkit

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:61
> +    static const int m_maxSearchIterations;

Statics shouldn't have the m_ prefix. This probably doesn't should be a member at all - just make it a static const local in the function it's being used in, or a file-static if it's used in multiple places

[Robert Kroeger]

Comment on attachment 162824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162824&action=review

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:69
>> +// the in-flight patch for https://bugs.webkit.org/show_bug.cgi?id=81663 .
> 
> I know you're just moving this code but this FIXME and comment are sad.  https://bugs.webkit.org/show_bug.cgi?id=81663 was abandoned a while ago, do we still need these extra parameters?

True. Cleaned up in the landed patch.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:56
>> +    float m_coeffs[5];
> 
> No abbreviations in webkit

Fixed in landed patch. And I removed the excess coefficients.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:61
>> +    static const int m_maxSearchIterations;
> 
> Statics shouldn't have the m_ prefix. This probably doesn't should be a member at all - just make it a static const local in the function it's being used in, or a file-static if it's used in multiple places

Done in landed patch.

[Robert Kroeger]

Created attachment 162867 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162867 [details]
Patch

Clearing flags on attachment: 162867

Committed r127924: <http://trac.webkit.org/changeset/127924>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.