[webkit-reviews] review denied: [Bug 80607] [chromium] Wire up PlatformGestureCurves for CC, ScrollAnimatorNone. : [Attachment 131113] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 80607: [chromium] Wire up PlatformGestureCurves for CC, ScrollAnimatorNone.
https://bugs.webkit.org/show_bug.cgi?id=80607

Attachment 131113: Patch
https://bugs.webkit.org/attachment.cgi?id=131113&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131113&action=review


This needs ChangeLogs, as I'm sure you know.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:221
> +	       // GestureScrollEnd with non-zero velocity (deltaX/Y) signals
start of Fling.

this is getting to be way too much logic in handleInputEvent - please split
this out into its own function

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:233
> +			      
CCActiveGestureAnimation::create(monotonicallyIncreasingTime(),
PlatformGestureCurveToCCGestureCurveAdapter::create(flingCurve.release()),
this);

i'm not sure this is really the right model - do we want to grab a timestamp
when we handle the event, or put the animation in a "waiting for start" state
and let the animation subsystem decide when the animation starts? this will
change whether or not you 'miss' the start of the curve if the compositor
doesn't tick for a short period of time after the input is handled.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:296
> +void WebCompositorInputHandlerImpl::setScrollIncrement(const IntPoint&
increment)
> +{
> +    if (m_inputHandlerClient)
> +	   m_inputHandlerClient->scrollBy(IntSize(increment.x(),
increment.y()));
> +}

now that i see the implementation this function name really confuses me. I
thought that setScrollIncrement sets a value that changes the granularity of
scrolls, but it looks like it just scrolls you.  What's the meaning of the
"incremeent" in the name?