Bug 79827 – [chromium] Implement scroll physics architecture for impl/main thread

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Implement scroll physics architecture for impl/main thread [not for review yet]

[W. James MacLean]

Created attachment 129315 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[W. James MacLean]

This is an initial draft of the smooth-scrolling refactoring, not yet ready for formal review.

It's probably over-engineered (every new class got its own file, for example). But, it does show the entire pathway (both main and impl threads). I left in a hack in WebViewImpl that I'm using for testing, in case anyone else wants to try it on the main thread (impl-side testing requires a similar patch in chromium).

So far it seems to have good performance on platforms I've used for test.

The wiring of the GestureCurveTarget* (a pointer to CCLayerTreeHostImpl) into WebCompositorInputHandler seems ugly - thoughts on simplifying/improving this?

The curve in PlatformScrollGestureCurveFling is almost certainly *not* what we'll want finally, although it is likely well suited for mouse-wheel scroll animation.

It should be easy enough to break this into a main-thread patch and an impl-thread patch for landing.

[Nat Duca]

Comment on attachment 129315 [details]
Patch

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

> Source/WebCore/WebCore.gypi:2816
> +            'platform/PlatformGestureActiveAnimation.cpp',

Backwards, this name feels... apologies if I suggested this in the first place.

ActivePlatformGestureAnimation?

Alternatively, webcore calls an "active animation" just an Animation, iirw. We added the "Active" because "Animation" is a pretty generic word. But, PlatformGestureAnimation is pretty clear too.

> Source/WebCore/WebCore.gypi:2820
> +            'platform/PlatformScrollGestureCurveFling.cpp',

More backward-sounding names. FlingScrollPlatformGestureCurve.

> Source/WebCore/WebCore.gypi:3309
> +            'platform/graphics/chromium/cc/CCGestureCurveTarget.h',

Put the target class in the Curve.h  --- in cc/ we tend to put clients/delegates/targets on the class that uses them.

> Source/WebCore/page/EventHandler.cpp:2354
> +    case PlatformEvent::GestureFlingStart:

Feels like a separate patch?

> Source/WebCore/platform/PlatformEvent.h:58
> +        GestureFlingStart,

Pull out to patch with the event handler?

> Source/WebCore/platform/PlatformGestureCurve.h:34
> +    // Returns false if curve has finished and can no longer bge applied.

bge->be

> Source/WebCore/platform/PlatformGestureCurveTarget.h:34
> +    virtual void setScrollIncrement(const IntPoint&) = 0;

Is this like "increment by pt.x"? Perhaps we should have a getScroll() and setScroll()? It seems to me this could be hard to do right... I'm not sure how we can make things mathematically correct with this sort of approach?

How did we do this on droid?

> Source/WebCore/platform/PlatformScrollGestureCurveFling.cpp:36
> +PassOwnPtr<PlatformGestureCurve> PlatformScrollGestureCurveFling::create(const FloatPoint& velocity)

You're calling this FlingScroll  or ScrollFling but the gesture you added to the input events was just Fling. If thats the case, then maybe just Fling is sufficient.

> Source/WebCore/platform/PlatformScrollGestureCurveFling.cpp:37
> +{

How does this class relate to the similar Android subclass? Is this fling the "generic" fling?

> Source/WebCore/platform/PlatformScrollGestureCurveGeneric.h:39
> +class PlatformScrollGestureCurveGeneric : public PlatformGestureCurve {

I'm a little confused the role of this class. Its like its a generic base class for something, but what's it value add?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:461
> +            animationTimerFired(&m_animationTimer);

Why fire the timer here and not startOneShot?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:537
> +#endif // ENABLE(SMOOTH_SCROLLING

looks like you accidentally dropped a )

> Source/WebCore/platform/ScrollAnimatorNone.h:56
>  

Might add a 
  // PlatformGestureCurveTarget implementation

And put the functions in a logical place so it stands out separate from the ScrollAnimator functiosn

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.cpp:5
> + * modification, are permitted provided that the following conditions

do we need a cpp file for this?

> Source/WebKit/chromium/WebKit.gyp:288
> +                'public/platform/PlatformGestureCurveToCCGestureCurveAdapter.h',

move to src/

[W. James MacLean]

I'll upload a new patch after I get some feedback on the best way to proceed with splitting out the EventHandler part of the patch (see comments below).

(In reply to comment #4)
> (From update of attachment 129315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129315&action=review
> 
> > Source/WebCore/WebCore.gypi:2816
> > +            'platform/PlatformGestureActiveAnimation.cpp',
> 
> Backwards, this name feels... apologies if I suggested this in the first place.
> 
> ActivePlatformGestureAnimation?
> 
> Alternatively, webcore calls an "active animation" just an Animation, iirw. We added the "Active" because "Animation" is a pretty generic word. But, PlatformGestureAnimation is pretty clear too.


Fixed (I chose ActivePlatformGestureAnimation).

> > Source/WebCore/WebCore.gypi:2820
> > +            'platform/PlatformScrollGestureCurveFling.cpp',
> 
> More backward-sounding names. FlingScrollPlatformGestureCurve.

How about 'PlatformGestureCurveFling'? Since we'll want to tack on things like 'Android' to the end for other fling-curve implementations, this might be easier? See further comments below.

> > Source/WebCore/WebCore.gypi:3309
> > +            'platform/graphics/chromium/cc/CCGestureCurveTarget.h',
> 
> Put the target class in the Curve.h  --- in cc/ we tend to put clients/delegates/targets on the class that uses them.

Fixed.

> > Source/WebCore/page/EventHandler.cpp:2354
> > +    case PlatformEvent::GestureFlingStart:
> 
> Feels like a separate patch?

Sure, I can pull this out to a separate patch. Would this land *after* the gesture animation patch (so handleGestureEvent will already exist in ScrollAnimatorNone)? If we do it the other way around, I'm worried about the handGestureEvents getting ripped out again because they are 'unused' (like they did last week).

> > Source/WebCore/platform/PlatformEvent.h:58
> > +        GestureFlingStart,
> 
> Pull out to patch with the event handler?

Ditto.

> > Source/WebCore/platform/PlatformGestureCurve.h:34
> > +    // Returns false if curve has finished and can no longer bge applied.
> 
> bge->be

Fixed.

> > Source/WebCore/platform/PlatformGestureCurveTarget.h:34
> > +    virtual void setScrollIncrement(const IntPoint&) = 0;
> 
> Is this like "increment by pt.x"? Perhaps we should have a getScroll() and setScroll()? It seems to me this could be hard to do right... I'm not sure how we can make things mathematically correct with this sort of approach?

Perhaps the main challenge here is not over-scrolling at the end of the ranges? Otherwise, I'm not sure what defines "correct". I had figured we would eventually add setScroll() to the targets for physics where the absolute scroll is known, but something open-ended like Fling might be just as well done incrementally (since the physics is natural to describe in terms of velocities/momentums).

> How did we do this on droid?

The cc-side code in Clank also seems to be doing incremental offset, calling scrollBy() in CCLayerTreeHostImpl.

> > Source/WebCore/platform/PlatformScrollGestureCurveFling.cpp:36
> > +PassOwnPtr<PlatformGestureCurve> PlatformScrollGestureCurveFling::create(const FloatPoint& velocity)
> 
> You're calling this FlingScroll  or ScrollFling but the gesture you added to the input events was just Fling. If thats the case, then maybe just Fling is sufficient.

I like just 'Fling'.

> > Source/WebCore/platform/PlatformScrollGestureCurveFling.cpp:37
> > +{
> 
> How does this class relate to the similar Android subclass? Is this fling the "generic" fling?

Yes, it's the generic fling, although as mentioned in the comments the physics will likely change before we land this. At present it's more like 'PlatformGestureCurveFlingMouseWheel', where each turn of the wheel generates a mini-fling that accelerates and then decelerates. A proper generic fling would start immediately at the input velocity (because, if I am thinking straight, it will follow on from a GestureScroll event, so things will already be moving).

> > Source/WebCore/platform/PlatformScrollGestureCurveGeneric.h:39
> > +class PlatformScrollGestureCurveGeneric : public PlatformGestureCurve {
> 
> I'm a little confused the role of this class. Its like its a generic base class for something, but what's it value add?

None. Zero. Zip. Nada. ;-)

I probably should have deleted it before uploading the patch, but I guess it was just a reminder of the (soon to be) family of PlaformGestureCurve*s that will follow.

> > Source/WebCore/platform/ScrollAnimatorNone.cpp:461
> > +            animationTimerFired(&m_animationTimer);
> 
> Why fire the timer here and not startOneShot?

No reason I guess, other than following the method used for smooth scroll. It would be nice to get the animation started right away, so I assume it's OK to call one-shot with a delay of 0?

> > Source/WebCore/platform/ScrollAnimatorNone.cpp:537
> > +#endif // ENABLE(SMOOTH_SCROLLING
> 
> looks like you accidentally dropped a )

Thanks, fixed.

> > Source/WebCore/platform/ScrollAnimatorNone.h:56
> >  
> 
> Might add a 
>   // PlatformGestureCurveTarget implementation

Done.

> And put the functions in a logical place so it stands out separate from the ScrollAnimator functiosn

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.cpp:5
> > + * modification, are permitted provided that the following conditions
> 
> do we need a cpp file for this?

Nope. It's a remnant from early on when I thought there might be common functionality to go in here.

Removed.

> > Source/WebKit/chromium/WebKit.gyp:288
> > +                'public/platform/PlatformGestureCurveToCCGestureCurveAdapter.h',
> 
> move to src/

Done.

[Robert Kroeger]

Comment on attachment 129315 [details]
Patch

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

>>> Source/WebCore/page/EventHandler.cpp:2354
>>> +    case PlatformEvent::GestureFlingStart:
>> 
>> Feels like a separate patch?
> 
> Sure, I can pull this out to a separate patch. Would this land *after* the gesture animation patch (so handleGestureEvent will already exist in ScrollAnimatorNone)? If we do it the other way around, I'm worried about the handGestureEvents getting ripped out again because they are 'unused' (like they did last week).

GestureScrollEnd == the start of a fling if the velocity > 0

> Source/WebCore/page/EventHandler.cpp:2355
> +    case PlatformEvent::GestureFlingCancel: {

don't need this either. stop the in-progress fling on a GestureTapBegin

> Source/WebCore/platform/ScrollAnimator.cpp:127
> +{

we need non-animated scrolls (but not flings) here to make the layout tests pass. My patch adds I think.

[W. James MacLean]

Created attachment 129705 [details]
Patch

[W. James MacLean]

Uploaded a new patch that addresses Rob's comments.

*** I haven't split out the code from EventHandler to a separate bug yet, as I'd like to keep everything in a single patch until everything looks OK, and tests are developed (I'm working on that now).

I changed the Fling physics to use a CDF curve as opposed to the density function that was in the first patch. Again, this will probably change before the patch lands, but I wanted to address Nat's concerns about the incremental nature of the scroll being robust to jitter in the tick interval.

Also, want to run this new patch through the bots to see if I have resolved the Mac compile issue.

[James Robinson]

Could you summarize the relationship between this and the logic in ScrollAnimatorNone at a behavioral level (not a code sharing level)?  Is it the same model, or something different?

[Robert Kroeger]

Comment on attachment 129705 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2380
> +        // GestureScrollEnd with non-zero velocity (deltaX/Y) signals start of Fling.

this will go to the "right" target assuming 67822 lands.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:449
> +#if ENABLE(GESTURE_EVENTS)

isn't there a bunch of code here that you could tear out by using the curves? Maybe in a subsequent CL.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:463
> +    case PlatformGestureEvent::GestureTapDown:

I think you will need to call the superclass to get scroll updates.

[WebKit Review Bot]

Comment on attachment 129705 [details]
Patch

Attachment 129705 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11808300

New failing tests:
fast/events/platform-wheelevent-paging-x-in-scrolling-page.html
fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html
fast/events/remove-child-onscroll.html
css3/filters/effect-invert-hw.html
fast/events/platform-wheelevent-paging-y-in-non-scrolling-page.html
fast/events/platform-wheelevent-paging-x-in-scrolling-div.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/repaint/overflow-scroll-in-overflow-scroll-scrolled.html
accessibility/aria-disabled.html
fast/events/wheelevent-direction-inverted-from-device.html
fast/loader/text-document-wrapping.html
css2.1/20110323/abspos-containing-block-initial-004d.htm
fast/repaint/overflow-auto-in-overflow-auto-scrolled.html
fast/events/platform-wheelevent-paging-xy-in-scrolling-div.html
fast/events/platform-wheelevent-paging-xy-in-scrolling-page.html
fast/events/continuous-platform-wheelevent-in-scrolling-div.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
compositing/reflections/nested-reflection-opacity.html
fast/events/platform-wheelevent-paging-y-in-scrolling-page.html
compositing/reflections/nested-reflection-transformed.html
fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
editing/selection/select-line-break-with-opposite-directionality.html
http/tests/inspector/inspect-element.html
fast/events/platform-wheelevent-in-scrolling-div.html
fast/events/platform-wheelevent-paging-y-in-non-scrolling-div.html
fast/events/platform-wheelevent-paging-x-in-non-scrolling-page.html
fast/events/platform-wheelevent-paging-y-in-scrolling-div.html
fast/events/platform-wheelevent-paging-x-in-non-scrolling-div.html

[W. James MacLean]

Created attachment 130692 [details]
Patch

[W. James MacLean]

(In reply to comment #12)
> Created an attachment (id=130692) [details]
> Patch

This version removes all development code and adds tests.

The curve classes are designed to manage errors from float -> int conversion internally so they do not accumulate.

This patch, combined with Rob's follow-on patch, will provide physics-based scroll on both impl and software path.

[Collabora GTK+ EWS bot]

Comment on attachment 130692 [details]
Patch

Attachment 130692 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11850415

[W. James MacLean]

(In reply to comment #14)
> (From update of attachment 130692 [details])
> Attachment 130692 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11850415

GTK turned on SMOOTH_SCROLLING last week, will add necessary files to GTK build in next revision of patch.

[Nat Duca]

Comment on attachment 130692 [details]
Patch

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

Looks ready for JamesR eyes. James, I'd suggest focusing on curves and ignore the WebCompositorInputHandlerImpl changes, as those will move out to a new patch in the morning.

> Source/WebCore/ChangeLog:10
> +        This patch adds PlatformGestureCurve as a way of using different scroll physics in a

For posterity and those doing git blame down the road, you might phrase this in a more general way.

Namely, the point of PlatformGestureCurve is to pull out the physical simulation from control concerns. This allows the physics to be reused in alternate places.

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:37
> +class ActivePlatformGestureAnimation {

Might put a // explaining what the logical purpose of this class is. Though maybe thats non-webkitty... (:

> Source/WebCore/platform/PlatformGestureCurve.h:32
> +class PlatformGestureCurve {

Again, // explaining the logical purpose of a curve...

> Source/WebCore/platform/ScrollAnimatorNone.cpp:491
> +    if (!!m_gestureAnimation) {

did you really mean "!!"? Is that normative?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:515
> +// PlatformGestureCurveTarget implementation

complete sentence comments.

> Source/WebCore/platform/ScrollAnimatorNone.h:91
> +    // PlatformGestureCurveTarget implementation

complete sentence

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:44
> +    // FIXME: consider imaking the value of sigma settable in the constructor.

Comment might make more sense in the implementation of this function instead of header since sigma isn't even visible on the header

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:138
> +    m_client->setNeedsCommitOnImplThread();

I think it should be just setNeedsRedraw() here.

The setNeedsCommit should be called by the thing that is mutating the tree. SetNeedsCommit says "we need to push something back to the main frame" --- we shoudl only do that when we really do change something.... so, logically, the m_activeGestureAnimation->animate() function should itself end up calling setNeedsCommit.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:757
> +            m_client->setNeedsCommitOnImplThread();

See my note before. I think this is a bit wonky too - if i read this right, if you do a fling, then we only commit at fling end. Thats fine with me, but I dont think thats part of the ticking logic --- I think the person implementing the CCActiveGestureAnimation should be the one to call setNeedsCommitOnImpl thread... seem kosher?

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:54
> +    static PassOwnPtr<WebCompositorInputHandlerImpl> create(WebCore::CCInputHandlerClient*, WebCore::CCGestureCurveTarget*);

Can we pull this out to a different patch?

I'm not sure this is the right embedding.

I think you want the curve target to be just another class in the wcih cpp file. That can itself be a class that impelments curve target and calls methods on CCInputHandlerClient to manipulate scroll position.

[James Robinson]

Comment on attachment 130692 [details]
Patch

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

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:41
> +    virtual bool animate(double ts);

please don't use abbreviations or one or two letter variable names. spell it out

i'm assuming all the timestamps/etc in this patch are in seconds - is that correct? if anything is in millis please type-tag the variable/parameter to reflect that.  webkit usually uses double seconds and we're converging on that in the compositor as well, so seconds is preferred

> Source/WebCore/platform/PlatformGestureCurve.h:35
> +    virtual bool apply(double ts, PlatformGestureCurveTarget*) = 0;

bad parameter name. expand

it's very rare to have a pure virtual class without a declare d'tor. please add a virtual d'tor in either the public section (if it's expected that classes will retain ownership of objects via a PlatformGestureCurve*) or in the protected section (if classes will only retain ownership by a pointer to a more specific type)

> Source/WebCore/platform/PlatformGestureCurve.h:38
> +}; // namespace

no ; to close a namespace

> Source/WebCore/platform/PlatformGestureCurveTarget.h:35
> +    // FIXME: add interfaces for setScroll(), setPageScaleAndScroll(), etc.

same comment as above re: d'tor

> Source/WebCore/platform/PlatformGestureCurveTarget.h:38
> +}; // namespace

no ; to close a namespace

> Source/WebCore/platform/ScrollAnimatorNone.cpp:451
> +    if (!!m_gestureAnimation)

no !!. you can just call .clear(), it's fine to do that on an OwnPtr<> that's already cleared

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:491
>> +    if (!!m_gestureAnimation) {
> 
> did you really mean "!!"? Is that normative?

OwnPtr<> has an operator bool that does the right thing, just rely on that. this isn't javascript - we have a (somewhat) real type system and don't need to coorce to bool with these contortions

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:37
> +class TouchFlingPlatformGestureCurve : public PlatformGestureCurve {

since this class has virtuals declare a d'tor explicitly in the header and define it in the .cpp

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:41
> +    virtual bool apply(double ts, PlatformGestureCurveTarget*);

expand parameter name

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:45
> +    TouchFlingPlatformGestureCurve(const FloatPoint& velocity);

explicit

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:48
> +    float m_timeScaler;

i think this should be named like a noun, but it feels more like a verb. this isn't a float that scales time, it's some sort of scale factor

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.cpp:42
> +    ASSERT(std::abs(velocity.x() || std::abs(velocity.y())));

using namespace std;, etc. or even better, why not just velocity != FloatPoint::zero() ?

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:41
> +    virtual bool apply(double ts, PlatformGestureCurveTarget*);

need better parameter name

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:45
> +    // FIXME: consider making the value of sigma settable in the constructor.
> +    WheelFlingPlatformGestureCurve(const FloatPoint& velocity);

comment should probably move as nduca suggested above, c'tor needs explicit

> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:51
> +}; // namespace

no ; on namespace close. if you're going to comment that we're closing a namespace at least say which namespace it is

> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.h:42
> +    virtual bool animate(double ts);

guess what?

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:37
> +    virtual void setScrollIncrement(const IntPoint &) = 0;

put the & right next to the type, no space

please declare d'tor in appropriate visibility

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:43
> +    virtual bool apply(double ts, CCGestureCurveTarget*) = 0;

same as above (param name, d'tor)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:753
> +    if (!!m_activeGestureAnimation) {

no !!

[W. James MacLean]

Comment on attachment 130692 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        This patch adds PlatformGestureCurve as a way of using different scroll physics in a
> 
> For posterity and those doing git blame down the road, you might phrase this in a more general way.
> 
> Namely, the point of PlatformGestureCurve is to pull out the physical simulation from control concerns. This allows the physics to be reused in alternate places.

OK, will expand as suggested.

>> Source/WebCore/platform/ActivePlatformGestureAnimation.h:37
>> +class ActivePlatformGestureAnimation {
> 
> Might put a // explaining what the logical purpose of this class is. Though maybe thats non-webkitty... (:

Done. Let me know if what I've written is what you were looking for.

>> Source/WebCore/platform/ActivePlatformGestureAnimation.h:41
>> +    virtual bool animate(double ts);
> 
> please don't use abbreviations or one or two letter variable names. spell it out
> 
> i'm assuming all the timestamps/etc in this patch are in seconds - is that correct? if anything is in millis please type-tag the variable/parameter to reflect that.  webkit usually uses double seconds and we're converging on that in the compositor as well, so seconds is preferred

Fixed. All the times in this patch are in seconds, not milliseconds.

>> Source/WebCore/platform/PlatformGestureCurve.h:32
>> +class PlatformGestureCurve {
> 
> Again, // explaining the logical purpose of a curve...

Done.

>> Source/WebCore/platform/PlatformGestureCurve.h:35
>> +    virtual bool apply(double ts, PlatformGestureCurveTarget*) = 0;
> 
> bad parameter name. expand
> 
> it's very rare to have a pure virtual class without a declare d'tor. please add a virtual d'tor in either the public section (if it's expected that classes will retain ownership of objects via a PlatformGestureCurve*) or in the protected section (if classes will only retain ownership by a pointer to a more specific type)

Thanks, fixed.

>> Source/WebCore/platform/PlatformGestureCurve.h:38
>> +}; // namespace
> 
> no ; to close a namespace

Fixed.

>> Source/WebCore/platform/PlatformGestureCurveTarget.h:35
>> +    // FIXME: add interfaces for setScroll(), setPageScaleAndScroll(), etc.
> 
> same comment as above re: d'tor

Fixed.

>> Source/WebCore/platform/PlatformGestureCurveTarget.h:38
>> +}; // namespace
> 
> no ; to close a namespace

Fixed.

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:451
>> +    if (!!m_gestureAnimation)
> 
> no !!. you can just call .clear(), it's fine to do that on an OwnPtr<> that's already cleared

Fixed.

>>> Source/WebCore/platform/ScrollAnimatorNone.cpp:491
>>> +    if (!!m_gestureAnimation) {
>> 
>> did you really mean "!!"? Is that normative?
> 
> OwnPtr<> has an operator bool that does the right thing, just rely on that. this isn't javascript - we have a (somewhat) real type system and don't need to coorce to bool with these contortions

Cool, did not know this. I had seen the !! elsewhere and thought it was normative.

Fixed.

>> Source/WebCore/platform/ScrollAnimatorNone.cpp:515
>> +// PlatformGestureCurveTarget implementation
> 
> complete sentence comments.

Fixed.

>> Source/WebCore/platform/ScrollAnimatorNone.h:91
>> +    // PlatformGestureCurveTarget implementation
> 
> complete sentence

Fixed.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:37
>> +class TouchFlingPlatformGestureCurve : public PlatformGestureCurve {
> 
> since this class has virtuals declare a d'tor explicitly in the header and define it in the .cpp

Done.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:41
>> +    virtual bool apply(double ts, PlatformGestureCurveTarget*);
> 
> expand parameter name

Done.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:44
>> +    // FIXME: consider imaking the value of sigma settable in the constructor.
> 
> Comment might make more sense in the implementation of this function instead of header since sigma isn't even visible on the header

Removed.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:45
>> +    TouchFlingPlatformGestureCurve(const FloatPoint& velocity);
> 
> explicit

Done.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.h:48
>> +    float m_timeScaler;
> 
> i think this should be named like a noun, but it feels more like a verb. this isn't a float that scales time, it's some sort of scale factor

Good idea - done.

>> Source/WebCore/platform/WheelFlingPlatformGestureCurve.cpp:42
>> +    ASSERT(std::abs(velocity.x() || std::abs(velocity.y())));
> 
> using namespace std;, etc. or even better, why not just velocity != FloatPoint::zero() ?

Good idea - Done.

>> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:41
>> +    virtual bool apply(double ts, PlatformGestureCurveTarget*);
> 
> need better parameter name

Fixed.

>> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:45
>> +    WheelFlingPlatformGestureCurve(const FloatPoint& velocity);
> 
> comment should probably move as nduca suggested above, c'tor needs explicit

Yup, done.

>> Source/WebCore/platform/WheelFlingPlatformGestureCurve.h:51
>> +}; // namespace
> 
> no ; on namespace close. if you're going to comment that we're closing a namespace at least say which namespace it is

Done and done.

>> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.h:42
>> +    virtual bool animate(double ts);
> 
> guess what?

:) Done

>> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:37
>> +    virtual void setScrollIncrement(const IntPoint &) = 0;
> 
> put the & right next to the type, no space
> 
> please declare d'tor in appropriate visibility

Ooops, typo on the '&' - fixed.

D'tor declared.

>> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:43
>> +    virtual bool apply(double ts, CCGestureCurveTarget*) = 0;
> 
> same as above (param name, d'tor)

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:138
>> +    m_client->setNeedsCommitOnImplThread();
> 
> I think it should be just setNeedsRedraw() here.
> 
> The setNeedsCommit should be called by the thing that is mutating the tree. SetNeedsCommit says "we need to push something back to the main frame" --- we shoudl only do that when we really do change something.... so, logically, the m_activeGestureAnimation->animate() function should itself end up calling setNeedsCommit.

Done. I've placed the setNeedsCommit in CCLayerTreeHostImpl::setScrollIncrement(), is this is where the animate function calls through to. Otherwise, the m_activeGestureAnimation object doesn't have a CCLayerTreeHostImplClient* in order to call from ...

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:753
>> +    if (!!m_activeGestureAnimation) {
> 
> no !!

Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:757
>> +            m_client->setNeedsCommitOnImplThread();
> 
> See my note before. I think this is a bit wonky too - if i read this right, if you do a fling, then we only commit at fling end. Thats fine with me, but I dont think thats part of the ticking logic --- I think the person implementing the CCActiveGestureAnimation should be the one to call setNeedsCommitOnImpl thread... seem kosher?

OK, fixed as indicated above. But we still need the redraw call, right?

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:54
>> +    static PassOwnPtr<WebCompositorInputHandlerImpl> create(WebCore::CCInputHandlerClient*, WebCore::CCGestureCurveTarget*);
> 
> Can we pull this out to a different patch?
> 
> I'm not sure this is the right embedding.
> 
> I think you want the curve target to be just another class in the wcih cpp file. That can itself be a class that impelments curve target and calls methods on CCInputHandlerClient to manipulate scroll position.

Hmm, ok - but this then require an icky cast of a WebCore::CCInputHandlerClient* to a WebCore::CCGestureCurveTarget* that leaves me rather uneasy. Is that considered OK?

Also, this means we can't test the adapters in PlatformGestureCurveTest.cpp (?)

[W. James MacLean]

Created attachment 130838 [details]
Patch

[W. James MacLean]

Revised as per suggestions.

This splits out the curves, will post WCIH/SAN plumbing in separate patch momentarily.

[Nat Duca]

Comment on attachment 130838 [details]
Patch

LGTM, thanks for splitting this.

[James Robinson]

Comment on attachment 130838 [details]
Patch

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

Seems pretty good. I think it's worth taking another pass to tighten up interface, etc, then we're good to land.

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:42
> +class ActivePlatformGestureAnimation {
> +public:

WTF_MAKE_NONCOPYABLE please

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:43
> +    static PassOwnPtr<ActivePlatformGestureAnimation> create(double startTime, PassOwnPtr<PlatformGestureCurve>, PlatformGestureCurveTarget*);

can you document the requirements of the target here? looks like the target has to outlive the animation, right?

> Source/WebCore/platform/ActivePlatformGestureAnimation.h:45
> +    virtual bool animate(double time);

why is this virtual?

> Source/WebCore/platform/PlatformGestureCurve.h:38
> +    virtual ~PlatformGestureCurve() { }

why is this d'tor public (as opposed to protected)? it's important to think carefully about this distinction for all of your interface classes - do you expect that users who see only this interface should be able to take ownership of the object (and delete it) or should owners have access to a pointer of a more specific type?

> Source/WebCore/platform/PlatformGestureCurveTarget.h:34
> +    virtual ~PlatformGestureCurveTarget() { }

same comment as above re: visibility of d'tor

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:43
> +    , m_timeScaleFactor(1000 / max(1.f, max(abs(velocity.x()), abs(velocity.y()))))

if this is supposed to take a floating point velocity, did you mean to use fabs()?

> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:45
> +    ASSERT(abs(velocity.x()) || abs(velocity.y()));

the "abs()" calls here are a bit surprising. they'll coerce to int. are you trying to ASSERT that velocity is not too close to zero or that it's not actually zero? i think either way this assertion could be written in a much clearer fashion.

> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.h:43
> +    virtual bool animate(double time);

again - why virtual? if you expect subclasses then the d'tor should be declared virtual

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:29
> +#include <wtf/Noncopyable.h>
> +#include <wtf/PassOwnPtr.h>

you don't appear to need either of these #includes in this header

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:37
> +    virtual ~CCGestureCurveTarget() { }

feels like it should be protected, is that right?

> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:45
> +    virtual ~CCGestureCurve() { }

protected?

> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

[W. James MacLean]

Created attachment 130953 [details]
Patch

[W. James MacLean]

Comment on attachment 130838 [details]
Patch

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

I also added an explicit destructor for ActivePlatformGestureAnimation.

>> Source/WebCore/platform/ActivePlatformGestureAnimation.h:42
>> +public:
> 
> WTF_MAKE_NONCOPYABLE please

Done.

>> Source/WebCore/platform/ActivePlatformGestureAnimation.h:43
>> +    static PassOwnPtr<ActivePlatformGestureAnimation> create(double startTime, PassOwnPtr<PlatformGestureCurve>, PlatformGestureCurveTarget*);
> 
> can you document the requirements of the target here? looks like the target has to outlive the animation, right?

Done.

>> Source/WebCore/platform/ActivePlatformGestureAnimation.h:45
>> +    virtual bool animate(double time);
> 
> why is this virtual?

Good point, doesn't need to be.

Fixed.

>> Source/WebCore/platform/PlatformGestureCurve.h:38
>> +    virtual ~PlatformGestureCurve() { }
> 
> why is this d'tor public (as opposed to protected)? it's important to think carefully about this distinction for all of your interface classes - do you expect that users who see only this interface should be able to take ownership of the object (and delete it) or should owners have access to a pointer of a more specific type?

Right. PlatformGestureCurveTarget derived classes will (1) always be represented as pointers to the base class, and (2) this pointer is managed by OwnPtr. So, unless I'm mistaken, doesn't the destructor need to be public in this case?

If I'm wrong let me know and I'll change it.

>> Source/WebCore/platform/PlatformGestureCurveTarget.h:34
>> +    virtual ~PlatformGestureCurveTarget() { }
> 
> same comment as above re: visibility of d'tor

This case is a bit different. Here PlatformGestureCurveTarget is one interface of (potentially) many a class could implement (e.g. ScrollAnimatorNone), so we should declare the d'tor protected so as not to allow someone who owns a PlatformGestureCurveTarget pointer to delete the derived object.

Let me know if my reasoning here is faulty.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:43
>> +    , m_timeScaleFactor(1000 / max(1.f, max(abs(velocity.x()), abs(velocity.y()))))
> 
> if this is supposed to take a floating point velocity, did you mean to use fabs()?

fabs() might be better ... in most cases it's not too critical if we truncate the velocities to integers, as they will generally be large, and the timeScalefactor need not be overly precise.

That being said, no reason not to use fabs() either, so we'll go with that.

>> Source/WebCore/platform/TouchFlingPlatformGestureCurve.cpp:45
>> +    ASSERT(abs(velocity.x()) || abs(velocity.y()));
> 
> the "abs()" calls here are a bit surprising. they'll coerce to int. are you trying to ASSERT that velocity is not too close to zero or that it's not actually zero? i think either way this assertion could be written in a much clearer fashion.

Fixed. (Don't recall now why I was using abs ...)

>> Source/WebCore/platform/graphics/chromium/cc/CCActiveGestureAnimation.h:43
>> +    virtual bool animate(double time);
> 
> again - why virtual? if you expect subclasses then the d'tor should be declared virtual

Nope, no need to be virtual - fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:29
>> +#include <wtf/PassOwnPtr.h>
> 
> you don't appear to need either of these #includes in this header

Right, removed.

>> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:37
>> +    virtual ~CCGestureCurveTarget() { }
> 
> feels like it should be protected, is that right?

yes, makes sense ... this way we cannot delete via a base-class pointer, right?

>> Source/WebCore/platform/graphics/chromium/cc/CCGestureCurve.h:45
>> +    virtual ~CCGestureCurve() { }
> 
> protected?

No, same reasoning as PlatformGestureCurve above. (Unless I'm wrong.)

>> Source/WebKit/chromium/tests/PlatformGestureCurveTest.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> 2012

Fixed.

[W. James MacLean]

Apparently I was wrong about the CCGestureCurveTarget's d'tor visibility, as it won't compile when it's made protected since we need it to construct GestureCurveTarget in WebCompositorInputHandlerImpl. (Not sure why I missed this earlier, expect I got my multiple patch parts out of order ...). Will revise patch shortly.

[W. James MacLean]

Created attachment 130959 [details]
Patch

[W. James MacLean]

(In reply to comment #26)
> Created an attachment (id=130959) [details]
> Patch

Also fixes non-existent entry in WebCore.gypi from previous patch.

[W. James MacLean]

(In reply to comment #25)
> Apparently I was wrong about the CCGestureCurveTarget's d'tor visibility, as it won't compile when it's made protected since we need it to construct GestureCurveTarget in WebCompositorInputHandlerImpl. (Not sure why I missed this earlier, expect I got my multiple patch parts out of order ...). Will revise patch shortly.

Argh! After all this, I managed to revert the change making the d'tor public before submitting the patch. It's getting late, and my internet connection is poor, so I'll fix this in the follow-on patch.

[James Robinson]

Comment on attachment 130959 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 130959 [details]
Patch

Clearing flags on attachment: 130959

Committed r110344: <http://trac.webkit.org/changeset/110344>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.