Bug 81779 - [GTK] Scrolling doesn't work in WebKit2 since r110185
Summary: [GTK] Scrolling doesn't work in WebKit2 since r110185
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-03-21 07:33 PDT by Carlos Garcia Campos
Modified: 2012-04-04 03:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.94 KB, patch)
2012-04-03 10:45 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-03-21 07:33:59 PDT
Bug #16123 broke WebKit scrolling. For some reason the None scroll animator doesn't work for us in WebKit2. The only way to scroll is by using the scrollbars, but mouse wheel or keys don't work at all
Comment 1 Zan Dobersek 2012-03-21 08:49:44 PDT
Works well with r109584, performing a bisection.
Comment 2 Carlos Garcia Campos 2012-03-21 08:52:32 PDT
It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression.
Comment 3 Zan Dobersek 2012-03-21 08:56:05 PDT
(In reply to comment #2)
> It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression.

Does that bring back only the simple scrolling capability or the entire smoothing effect?
Comment 4 Carlos Garcia Campos 2012-03-21 08:58:31 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression.
> 
> Does that bring back only the simple scrolling capability or the entire smoothing effect?

I think it uses the old simple scrolling, by using ScrollAnimator::scroll() instead of ScrollAnimatorNone::scroll()
Comment 5 Zan Dobersek 2012-03-21 09:17:28 PDT
FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true.
Comment 6 Carlos Garcia Campos 2012-03-21 09:21:53 PDT
(In reply to comment #5)
> FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true.

That makes more sense. We are very late in the release cycle, I've added this bug to the list of things to fix before 1.8

https://trac.webkit.org/wiki/WebKitGTK/1.8.x
Comment 7 Carlos Garcia Campos 2012-03-21 09:34:48 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true.
> 
> That makes more sense. We are very late in the release cycle, I've added this bug to the list of things to fix before 1.8
> 
> https://trac.webkit.org/wiki/WebKitGTK/1.8.x

Sorry, Zan told me that smooth scrolling didn't land in the branch, so I've just removed the bug from the wiki page.
Comment 8 Zan Dobersek 2012-03-22 01:18:49 PDT
The offending commit is r110185
http://trac.webkit.org/changeset/110185

Changes made cause the animation scheduling to be done through ChromeClient only if the port is using display refresh monitor. Gtk is currently using display refresh timer (sort of a fallback), which was not accounted for in that revision. That's causing the scrolling animations not to be scheduled and done.

There's two ways of fixing this - use another timer in Chrome class in case the port is not using the display refresh monitor, or try to implement a display refresh monitor for the Gtk port.

I've looked up in Chromium how the display refresh monitor is done there. It seems that if the --disable-gpu-vsync switch is present, the animation interval is 0, meaning animation updates are made as fast as possible. Otherwise they're aiming for the standard 60FPS with animation intervals of 16 milliseconds.

If we'd be following that approach, implementing the display refresh monitor would be pretty straightforward, at least in WebKit1. I'd like some opinion before approaching this, though.
Comment 9 Martin Robinson 2012-03-22 07:43:48 PDT
(In reply to comment #8)

> I've looked up in Chromium how the display refresh monitor is done there. It seems that if the --disable-gpu-vsync switch is present, the animation interval is 0, meaning animation updates are made as fast as possible. Otherwise they're aiming for the standard 60FPS with animation intervals of 16 milliseconds.

My understanding is that GPU Vsync doesn't really make as much sense in composited window managers. I I could be mistaken, but it makes sense to have an appropriate fallback for when the hardware support isn't present.
Comment 10 Zan Dobersek 2012-03-30 07:46:24 PDT
(In reply to comment #8)
> Changes made cause the animation scheduling to be done through ChromeClient only if the port is using display refresh monitor. Gtk is currently using display refresh timer (sort of a fallback), which was not accounted for in that revision. That's causing the scrolling animations not to be scheduled and done.
> 

I actually got this part wrong. Chromium doesn't use neither display refresh monitor nor the refresh timer, but pipes the request through to ChromeClient (the WebViewImpl[1]). The request is then handled in Chromium and WebViewImpl::updateAnimations[2] is called, relaying the animation serving request back to FrameView.

I wonder if it would make sense to go along a similar way, exposing some API through which user would be able to throttle animation updates to desired refresh rate, enable vsync (if applicable) etc.

[1] http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebViewImpl.cpp#L3207
[2] http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebViewImpl.cpp#L1368
Comment 11 Carlos Garcia Campos 2012-04-01 23:33:24 PDT
I would focus on making scrolling work again and then look for a solution to use the scroll animator, it's been broken for two weeks now.
Comment 12 Zan Dobersek 2012-04-03 10:44:44 PDT
CC-ing author and reviewer from bug #78938 from which r110185 landed.
Comment 13 Zan Dobersek 2012-04-03 10:45:11 PDT
Created attachment 135361 [details]
Patch
Comment 14 Scott Byer 2012-04-03 10:55:24 PDT
What are the downstream products of the GTK port that need this in the short term, and can't deal with temporarily disabling the scroll animator?

The self-timer was always meant to be a short-term stopover, getting the animation timer going for GTK is definitely the better way forward.

If it's critical, this looks OK (an earlier patch in bug #78938 had the code like this).
Comment 15 James Robinson 2012-04-03 11:09:50 PDT
Comment on attachment 135361 [details]
Patch

Looks OK.  A Timer is gonna give you very janky animations, though, you really want to be tied in to the rendering loop of the consumer (window manager or higher level application) to get smoothness.
Comment 16 WebKit Review Bot 2012-04-04 03:29:41 PDT
Comment on attachment 135361 [details]
Patch

Clearing flags on attachment: 135361

Committed r113161: <http://trac.webkit.org/changeset/113161>
Comment 17 WebKit Review Bot 2012-04-04 03:29:46 PDT
All reviewed patches have been landed.  Closing bug.