WebCore has several animated smooth-scrolling implementations, but they are currently enabled or disabled by the build-systems or by settings in WTF. To support optionally adding this for specific builds, the smooth-scrolling option should be made configurable. At least for webkit platforms that does not explicitly enable or disable it.
Created attachment 120016 [details] Patch
Why would a port want to implement this feature, but then not build it?
It might not be suitable for all devices or configurations, and in this implementation it is not possible to disable it on runtime. Also it is evident that "implementing" it has been as simple as activating it in the build-system, and Qt activates it only for Windows, and Chrome only for Mac. So I assume someone has made that choice at one point, for some reason or another.
I should add that I expect to make the ScrollAnimatorNone default for Qt later, it just needs access to runtime configuration first. As far as I can tell there are three ScrollAnimators now. One with Mac style animated scrolling, one with Win style animated scrolling, and the oddly name ScrollAnimatorNone enables runtime configuration of both the presence and a few different algorithms of animated scrolling. While the Mac-style requires objective-c, the Win-style has no specific requirements to platform. Which is why I chose it as the first default for Qt.
Created attachment 137540 [details] Patch
Comment on attachment 137540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137540&action=review > Tools/Scripts/build-webkit:330 > + define => "ENABLE_SMOOTH_SCROLLING", default => 0, value => \$smoothScrollingSupport }, Is 0 the right default? Won't this disable it on all (non-Qt) builds as well?
(In reply to comment #6) > (From update of attachment 137540 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137540&action=review > > > Tools/Scripts/build-webkit:330 > > + define => "ENABLE_SMOOTH_SCROLLING", default => 0, value => \$smoothScrollingSupport }, > > Is 0 the right default? Won't this disable it on all (non-Qt) builds as well? I don't think so, but I will need to check again. Most platforms that support smooth-scrolling sets it unconditionally, and those that doesn't support does not set it, in the end defaulting to 0.
Comment on attachment 137540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137540&action=review >>> Tools/Scripts/build-webkit:330 >>> + define => "ENABLE_SMOOTH_SCROLLING", default => 0, value => \$smoothScrollingSupport }, >> >> Is 0 the right default? Won't this disable it on all (non-Qt) builds as well? > > I don't think so, but I will need to check again. Most platforms that support smooth-scrolling sets it unconditionally, and those that doesn't support does not set it, in the end defaulting to 0. Please confirm, and I can help with getting this reviewed.
Safari enables smooth-scrolling unconditionally if not iOS in Platform.h. GTK enables it unconditionally in GNUmakefile.am Qt overrides the defaults with the values from features.prf. Chromium seems to set it unconditionally in features.gypi So, the reported default value will be wrong, but it shouldn't override any platforms already enabling smooth scrolling.
Created attachment 138063 [details] Patch
Comment on attachment 138063 [details] Patch Match current defaults.
Comment on attachment 138063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138063&action=review > Source/WebCore/Target.pri:2918 > + platform/ActivePlatformGestureAnimation.h \ > + platform/ScrollAnimatorNone.h \ > + platform/TouchpadFlingPlatformGestureCurve.h \ > + platform/WheelFlingPlatformGestureCurve.h > + SOURCES += \ > + platform/ActivePlatformGestureAnimation.cpp \ > + platform/ScrollAnimatorNone.cpp \ > + platform/TouchpadFlingPlatformGestureCurve.cpp \ > + platform/WheelFlingPlatformGestureCurve.cpp Hum, I did not need these other than ScrollAnimatornone.cpp|h when enabled it to BlackBerry port. > Tools/Scripts/build-webkit:331 > + { option => "smooth-scrolling", desc => "Toggle Animated Smooth Scrolling", > + define => "ENABLE_SMOOTH_SCROLLING", default => (isAppleMacWebKit() || isGtk() || isChromium()), value => \$smoothScrollingSupport }, > + Ming, could we fix BlackBerry here?
>> Tools/Scripts/build-webkit:331 >> + { option => "smooth-scrolling", desc => "Toggle Animated Smooth Scrolling", >> + define => "ENABLE_SMOOTH_SCROLLING", default => (isAppleMacWebKit() || isGtk() || isChromium()), value => \$smoothScrollingSupport }, >> + > Ming, could we fix BlackBerry here? Yes, I think we need isBlackBerry() in the default list too. See trunk@115346 Also, as trunk@116055, WebKit features are moved to Scripts/webitperl/FeatureList.pm You may need to update your patch for that.
(In reply to comment #12) > (From update of attachment 138063 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138063&action=review > > > Source/WebCore/Target.pri:2918 > > + platform/ActivePlatformGestureAnimation.h \ > > + platform/ScrollAnimatorNone.h \ > > + platform/TouchpadFlingPlatformGestureCurve.h \ > > + platform/WheelFlingPlatformGestureCurve.h > > + SOURCES += \ > > + platform/ActivePlatformGestureAnimation.cpp \ > > + platform/ScrollAnimatorNone.cpp \ > > + platform/TouchpadFlingPlatformGestureCurve.cpp \ > > + platform/WheelFlingPlatformGestureCurve.cpp > > Hum, I did not need these other than ScrollAnimatornone.cpp|h when enabled it to BlackBerry port. > They are used from within ScrollAnimatorNone in the functions that the blackberry implementation overrides. Though, even if those virtual functions are never called, I am still surprised you don't get linker errors.
Created attachment 143036 [details] Patch
Created attachment 143037 [details] Patch ChangeLog not updated
Comment on attachment 143037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143037&action=review > Source/WebCore/Target.pri:2935 > + HEADERS += platform/ScrollAnimatorNone.h > + SOURCES += platform/ScrollAnimatorNone.cpp I'd like to see this renamed to something that reflects that it's a runtime-configurable behavior, not a no-op file. > Source/WebCore/platform/ScrollAnimatorNone.h:178 > +#if !PLATFORM(QT) > OwnPtr<ActivePlatformGestureAnimation> m_gestureAnimation; > +#endif > }; Is there a better guard for this? > Tools/Scripts/webkitperl/FeatureList.pm:306 > + { option => "smooth-scrolling", desc => "Toggle Animated Smooth Scrolling", > + define => "ENABLE_SMOOTH_SCROLLING", default => (isAppleMacWebKit() || isGtk() || isChromium() || isBlackBerry()), value => \$smoothScrollingSupport }, > + You need to touch more build system files now that the feature is a build-webkit-visible enable. Some you might be able to auto-generate using the python-script that eseidel has been hacking on (do a git log on this file for details). > Tools/qmake/mkspecs/features/features.prf:208 > +# Animated smooth-scrolling default features.pri (auto-generated from FeatureList.pm) should have ENABLE_SMOOTH_SCROLLING=0
Created attachment 158531 [details] Patch Move smooth-scrolling setting from compile-time to run-time
Comment on attachment 158531 [details] Patch r=me modulo the typos and the default for WK2
Committed r126291: <http://trac.webkit.org/changeset/126291>