Bug 95997

Summary: [chromium] Add touchscreen specific fling curve parametrization
Product: WebKit Reporter: Robert Kroeger <rjkroege>
Component: UI EventsAssignee: Robert Kroeger <rjkroege>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, jamesr, rakuco, webkit.review.bot, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Robert Kroeger
Reported 2012-09-06 10:51:36 PDT
Add an additional fling curve parametrization specific to touchscreen fling.
Attachments
Patch (4.59 KB, patch)
2012-09-06 11:26 PDT, Robert Kroeger
no flags
Patch (30.88 KB, patch)
2012-09-07 11:13 PDT, Robert Kroeger
no flags
Patch (30.51 KB, patch)
2012-09-07 14:18 PDT, Robert Kroeger
no flags
Robert Kroeger
Comment 1 2012-09-06 11:26:05 PDT
Robert Kroeger
Comment 2 2012-09-06 13:43:45 PDT
Comment on attachment 162545 [details] Patch jamesr@: please take a look.
James Robinson
Comment 3 2012-09-06 13:50:42 PDT
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
Comment 4 2012-09-06 13:54:16 PDT
(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
Comment 5 2012-09-06 14:00:18 PDT
That sounds better.
Robert Kroeger
Comment 6 2012-09-06 14:07:29 PDT
sounds good to me. I'll update the patch.
Robert Kroeger
Comment 7 2012-09-07 11:13:01 PDT
Robert Kroeger
Comment 8 2012-09-07 11:14:39 PDT
please take another look.
James Robinson
Comment 9 2012-09-07 12:23:33 PDT
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 10 2012-09-07 14:16:40 PDT
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
Comment 11 2012-09-07 14:18:05 PDT
WebKit Review Bot
Comment 12 2012-09-07 14:56:53 PDT
Comment on attachment 162867 [details] Patch Clearing flags on attachment: 162867 Committed r127924: <http://trac.webkit.org/changeset/127924>
WebKit Review Bot
Comment 13 2012-09-07 14:56:58 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.