Bug 16123

Relevant GTK+ bug: http://bugzilla.gnome.org/show_bug.cgi?id=444659 Bug 444659 – Add GtkTimeline to ease animations development

Just a relevant thing, I'm currently working on a set of patches to xf86-input-synaptics, and GTK to enable smooth scrolling support for a window, however not smooth scrolling in the firefox sense, but in the sense of the scroll events being reported with pixel granularity by the mouse driver, which I find to be MUCH nicer than firefox smooth scrolling. This is also the model used by OSX. There are some videos of it being used with the Seed webkit browser at: http://www.hortont.com/racarr/?p=20

Created attachment 95622 [details] Patch

This is a start; it adds the plumbing for run-time enablement and an initial implementation (that leaves lots of tuning to do). Currently doesn't affect the standalone WebKit build at all. The initial focus is Linux/Chromium OS, but eventually this code wants to be refactored with the existing but turned off Windows smooth scrolling code into something more cross platform. Some of this will also dovetail with the platform gestures work.

I haven't had the chance to review the code but at the high-level there doesn't seem to be anything linux-specific here. Should the class be renamed to something more generic or move to platform/chromium and become ScrollAnimatorChromium?

No, there really isn't anything Linux specific, and I don't think there will be (except for initialization paramaters, possibly, especially since we want to keep the system provided Mac touchpad behavior). The move to platform/chromium does make sense to me.

This is the wrong bug for this patch. This bug is for smooth scrolling in WebKit GTK+ (e.g., build-webkit --gtk). Please open a new bug for adding smooth scrolling to Chromium.

Comment on attachment 95622 [details] Patch I agree with Vangelis that these files should be in platform/chromium and shouldn't have Linux in the filename if they're not Linux specific.

This bug is for the GTK port, not chromium.

Created attachment 102527 [details] Patch This patch uses ScrollAnimatorNone class to provide smooth scrolling and introduces a new flag to enable it (it is disabled by default)

API looks straight forward. Any reason to keep this behind a compilation option, though? I'd just leave it on and rely on the runtime option.

Comment on attachment 102527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102527&action=review Looks good! Is there any compelling reason to make this a compile-time option? Why not just compile it in always. r- because of the missing documentation. We need one more GTK+ reviewer to approve the API, but I think it's pretty non-controversial. Do you think you could add the setting to WebKit2 as well? > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:932 > + * Whether ... You're missing the documentation here. :(

Created attachment 127560 [details] Patch

Comment on attachment 127560 [details] Patch This patch also adds support for enabling smooth scrolling in WebKit2. Documentation should now be properly processed in both WebKit1 and WebKit2. This also adds two additional functions to the core WebKit2 C API, along with a basic preference in WebPreferencesStore. Also settings the cq flag. Thanks for the reviews!

Comment on attachment 127560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127560&action=review This looks good to me. I think we just need some Mac guys to look at the proposed C API changes. > Source/JavaScriptCore/wtf/Platform.h:490 > +#define ENABLE_SMOOTH_SCROLLING 1 Nit. This is probably something that we want to set in the GNUmakefile. We can just do it there unconditionally.

Anders or Sam, do you mind giving a nod to these WebKit2 C API changes?

Created attachment 127700 [details] Patch

(In reply to comment #15) > > Source/JavaScriptCore/wtf/Platform.h:490 > > +#define ENABLE_SMOOTH_SCROLLING 1 > > Nit. This is probably something that we want to set in the GNUmakefile. We can just do it there unconditionally. Created a small section near the top of Source/WebCore/GNUmakefile.am that appends such features to FEATURE_DEFINES and webcore_cppflags in the latest patch. In future if more features require the same, the comment in that section can be expanded to nicely list all such features.

Comment on attachment 127700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127700&action=review Please, add a test the new WEbKit2 setting in Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2053 > + return WKPreferencesGetEnableScrollAnimator(settings->priv->preferences.get()); I'm a bit confused, the scroll animator always does smooth scrolling or are there other types of scroll animations?

(In reply to comment #19) > (From update of attachment 127700 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127700&action=review > > Please, add a test the new WEbKit2 setting in Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp > Sure, will do. > > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2053 > > + return WKPreferencesGetEnableScrollAnimator(settings->priv->preferences.get()); > > I'm a bit confused, the scroll animator always does smooth scrolling or are there other types of scroll animations? The pure ScrollAnimator class only does the normal scrolling (the one in use today). It is platform-independent. Smooth scrolling is provided by ScrollAnimatorWin class for Windows platform, ScrollAnimatorMac class for Mac platform, and ScrollAnimatorNone class, which provides smooth scrolling in a platform-independent way. The ScrollAnimatorNone is obviously the only option, but is only used instead of the core ScollAnimator class if the scroll animator setting is enabled.

Comment on attachment 127700 [details] Patch Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this?

(In reply to comment #21) > (From update of attachment 127700 [details]) > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this? There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately. Though, without the WKPreferences API additions, this would require the connecting and adjusting to the 'gtk-use-animations' value to be performed in WebProcess. But ideally, the GtkSettings instance would be gathered from the specific GtkWidget (the web view), which is a part of UIProcess, and as such AFAIK not reachable from WebProcess. How about keeping the WKPreferences API additions private, in WKPreferencesPrivate.h? [1] http://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-enable-animations

(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 127700 [details] [details]) > > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this? > > There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately. When gtk-use-animations is true though, does GTK+ use smooth scrolling? If not, then we'll need to look elsewhere to maintain consitency with GTK+.

(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (From update of attachment 127700 [details] [details] [details]) > > > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this? > > > > There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately. > > When gtk-use-animations is true though, does GTK+ use smooth scrolling? If not, then we'll need to look elsewhere to maintain consitency with GTK+. Oops, it's actually gtk-enable-animations. The only Gtk application I've seen to date to perform smooth scrolling is the gnome-about dialog. It doesn't respect the gtk-use-animations setting. Grepping through the gtk source, this setting is used to decide whether or not to animate smaller animations, such as expanding the tree view or sliding a toolbar item etc. Considering this, gtk-enable-animations might not be appropriate to control whether or not to enable smooth scrolling.

In the end, smooth scrolling is going to be handled through a new GDK_SMOOTH_SCROLL_MASK evmask flag, but there's not going to be a setting as such to turn it off/on, widgets just do decide which kind of scroll events fit them better

So, I would just enable this unconditionally. If someone asks for a way to disable it we can then discuss whether to add a new setting to enable/disable it.

(In reply to comment #26) > So, I would just enable this unconditionally. If someone asks for a way to disable it we can then discuss whether to add a new setting to enable/disable it. Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree.

(In reply to comment #27) > (In reply to comment #26) > > So, I would just enable this unconditionally. If someone asks for a way to disable it we can then discuss whether to add a new setting to enable/disable it. > > Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree. No, we don't need GDK_SMOOTH_SCROLL_MASK because we have our own scroll animator that does smooth scrolling. GDK_SMOOTH_SCROLL_MASK will not have any effect in WebView because we don't implement GtkScrollable. I'm not sure it's clear what Carlos said. GTK+ doesn't still support smooth scrolling. Current implementation (in smooth-scrolling branch) adds GDK_SMOOTH_SCROLL_MASK which makes that delta information is included in GdkScroll events. I will be implemented in GtkScrolledWindow, and widgets will decide whether they want smooth scrolling or not, by adding GDK_SMOOTH_SCROLL_MASK to its own events masks (typically done in realize()). So, there won't be any API nor setting to enable/disable smooth scrolling in widgets. If GTK+ devs decide that GtkTreeView does smooth scrolling, it will include the GDK_SMOOTH_SCROLL_MASK and will do smooth scrolling unconditionally. My opinion is that we should do the same and decide to use smooth scrolling unconditionally in WebView (in our case using the scroll animator, I don't need GDK_SMOOTH_SCROLL_MASK). If we eventually see that people want to be able to disable it, we can just add a setting later.

(In reply to comment #28) > > Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree. > > My opinion is that we should do the same and decide to use smooth scrolling unconditionally in WebView (in our case using the scroll animator, I don't need GDK_SMOOTH_SCROLL_MASK). If we eventually see that people want to be able to disable it, we can just add a setting later. I would like to understand more about whether there are plans to switch all built-in GTK+ widgets to smooth scrolling or how this decision is made if it's done for some widgets and not others. Either way, adding a setting up-front seems low risk.

(In reply to comment #29) > (In reply to comment #28) > > > > Just to clarify, you mean we should do smooth scrolling unconditionally when the GDK_SMOOTH_SCROLL_MASK flag is active? If so, I believe I agree. > > > > > My opinion is that we should do the same and decide to use smooth scrolling unconditionally in WebView (in our case using the scroll animator, I don't need GDK_SMOOTH_SCROLL_MASK). If we eventually see that people want to be able to disable it, we can just add a setting later. > > I would like to understand more about whether there are plans to switch all built-in GTK+ widgets to smooth scrolling or how this decision is made if it's done for some widgets and not others. Either way, adding a setting up-front seems low risk. It will depend on every widget, I think tree view, icon view, text view will do smooth scrolling, for example. So it's up to use to decide. I don't mind if we add the setting in wk1, but for for wk2 we could try to enable it by default and add the setting later if we see we really needed.

(In reply to comment #30) > It will depend on every widget, I think tree view, icon view, text view will do smooth scrolling, for example. So it's up to use to decide. I don't mind if we add the setting in wk1, but for for wk2 we could try to enable it by default and add the setting later if we see we really needed. Sounds like a good compromise to me.

(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 127700 [details] [details]) > > Does this really need to be a setting? Isn't there some default "use animations" setting or style property in gtk? When would you ever want to disable this? > > There indeed is a 'gtk-use-animations' property in GtkSettings, which defaults to true. If acceptable to reviewers, the code could just connect to the notify::gtk-use-animation signal of GtkSettings and change the WebCore setting appropriately. > > Though, without the WKPreferences API additions, this would require the connecting and adjusting to the 'gtk-use-animations' value to be performed in WebProcess. But ideally, the GtkSettings instance would be gathered from the specific GtkWidget (the web view), which is a part of UIProcess, and as such AFAIK not reachable from WebProcess. > Sure, you'd have to keep track of the state in the UI process and send it over to the web process when it changes. We do this for a number of different settings.

(In reply to comment #31) > (In reply to comment #30) > > It will depend on every widget, I think tree view, icon view, text view will do smooth scrolling, for example. So it's up to use to decide. I don't mind if we add the setting in wk1, but for for wk2 we could try to enable it by default and add the setting later if we see we really needed. > > Sounds like a good compromise to me. Very well, this actually requires no changes to be made in any part of WebKit2. Patch incoming.

Created attachment 127937 [details] Patch

(In reply to comment #34) > Created an attachment (id=127937) [details] > Patch Contains wrong 'Since' version in signal definition, will upload the correct one.

Created attachment 128212 [details] Patch

Comment on attachment 128212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964 > + * Since: 1.7.90 This should probably be 1.9.0 now, since we've already branched for stable.

(In reply to comment #37) > (From update of attachment 128212 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review > > > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964 > > + * Since: 1.7.90 > > This should probably be 1.9.0 now, since we've already branched for stable. 1.10 I guess, 1.9.x will be the unstable versions of the next release cycle.

(In reply to comment #38) > (In reply to comment #37) > > (From update of attachment 128212 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review > > > > > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964 > > > + * Since: 1.7.90 > > > > This should probably be 1.9.0 now, since we've already branched for stable. > > 1.10 I guess, 1.9.x will be the unstable versions of the next release cycle. There's plenty of places where an unstable version is given as the 'Since:' value. This will be a part of the next unstable release (1.9.0) and subsequently of the next stable release as well (1.10.0). All the 'Since:' tags should either have a value of an unstable release (1.7.5, 1.9.0, 1.9.1 etc.), stating in which release exactly API was added, or a value of a stable release (1.6, 1.8, 1.10 etc.), stating in what stable release API was first present. AFAIK WebKitGtk+ is mostly if not completely using the first approach.

(In reply to comment #39) > (In reply to comment #38) > > (In reply to comment #37) > > > (From update of attachment 128212 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=128212&action=review > > > > > > > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:964 > > > > + * Since: 1.7.90 > > > > > > This should probably be 1.9.0 now, since we've already branched for stable. > > > > 1.10 I guess, 1.9.x will be the unstable versions of the next release cycle. > > There's plenty of places where an unstable version is given as the 'Since:' value. This will be a part of the next unstable release (1.9.0) and subsequently of the next stable release as well (1.10.0). I think that comes from the time were webkitgtk did follow the gnome release cycle with stable/unstable releases, I'm not sure, though. > All the 'Since:' tags should either have a value of an unstable release (1.7.5, 1.9.0, 1.9.1 etc.), stating in which release exactly API was added, or a value of a stable release (1.6, 1.8, 1.10 etc.), stating in what stable release API was first present. > > AFAIK WebKitGtk+ is mostly if not completely using the first approach. So, maybe we should consider changing it.

(In reply to comment #40) > > AFAIK WebKitGtk+ is mostly if not completely using the first approach. > > So, maybe we should consider changing it. I agree that we should consider changing it, but probably it should be handled separately from this bug. If such a change it made, we can go back and fix all existing Since: tags.

Created attachment 129852 [details] Updated patch Since: tag updated to 1.9.0

(In reply to comment #42) > Created an attachment (id=129852) [details] > Updated patch > > Since: tag updated to 1.9.0 I still think we shouldn't use unstable version numbers in Since: tags. If this patrch will be merged into 1.8, this should be Since: 1.8 otherwise it should be 1.10

Comment on attachment 129852 [details] Updated patch Clearing flags on attachment: 129852 Committed r109584: <http://trac.webkit.org/changeset/109584>

All reviewed patches have been landed. Closing bug.

(In reply to comment #43) > (In reply to comment #42) > > Created an attachment (id=129852) [details] [details] > > Updated patch > > > > Since: tag updated to 1.9.0 > > I still think we shouldn't use unstable version numbers in Since: tags. If this patrch will be merged into 1.8, this should be Since: 1.8 otherwise it should be 1.10 Should I open a new bug covering this issue?

(In reply to comment #46) > Should I open a new bug covering this issue? Yeah, as I suggested above, deciding what versions to use in Since tags is not a question that this bug needs to solve and shouldn't block this patch. Probably it deserves a short note to the mailing list to gather opinions. It's such a minor issue that hopefully it shouldn't be too controversial. :) Once we decide what to do, we can open a bug to fix all the existing Since tags. It's totally orthoganol to smooth scrolling though.