Bug 74926 – [Qt] Optionally support smooth-scrolling on all platforms

• Component: Platform
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 120016 [details]
Patch

Why would a port want to implement this feature, but then not build it?

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 137540 [details]
Patch

[Simon Hausmann]

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?

[Allan Sandfeld Jensen]

(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.

[Antonio Gomes]

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.

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 138063 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 138063 [details]
Patch

Match current defaults.

[Antonio Gomes]

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?

[Ming Xie]

>> 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.

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Created attachment 143036 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 143037 [details]
Patch

ChangeLog not updated

[Tor Arne Vestbø]

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

[Allan Sandfeld Jensen]

Created attachment 158531 [details]
Patch

Move smooth-scrolling setting from compile-time to run-time

[Simon Hausmann]

Comment on attachment 158531 [details]
Patch

r=me modulo the typos and the default for WK2

[Allan Sandfeld Jensen]

Committed r126291: <http://trac.webkit.org/changeset/126291>