[chromium] Implement scroll physics architecture for impl/main thread [not for review yet]
Created attachment 129315 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
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/
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.
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.
Created attachment 129705 [details] Patch
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.
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?
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.
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
Created attachment 130692 [details] Patch
(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.
Comment on attachment 130692 [details] Patch Attachment 130692 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11850415
(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.
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.
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 !!
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 (?)
Created attachment 130838 [details] Patch
Revised as per suggestions. This splits out the curves, will post WCIH/SAN plumbing in separate patch momentarily.
Comment on attachment 130838 [details] Patch LGTM, thanks for splitting this.
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
Created attachment 130953 [details] Patch
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.
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.
Created attachment 130959 [details] Patch
(In reply to comment #26) > Created an attachment (id=130959) [details] > Patch Also fixes non-existent entry in WebCore.gypi from previous patch.
(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.
Comment on attachment 130959 [details] Patch R=me
Comment on attachment 130959 [details] Patch Clearing flags on attachment: 130959 Committed r110344: <http://trac.webkit.org/changeset/110344>
All reviewed patches have been landed. Closing bug.