Summary: | [GTK] Scrolling doesn't work in WebKit2 since r110185 | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | jamesr, mrobinson, scottbyer, webkit.review.bot, zan | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2012-03-21 07:33:59 PDT
It works for me if I set m_scrollAnimatorEnabled to false in Settings, so I assumed r109584 introduced the regression. (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? (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() FWIW, the same problem occurs in WebKit1 as well when enable-smooth-scrolling setting is set to true. (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 (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. 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. (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. (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 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. CC-ing author and reviewer from bug #78938 from which r110185 landed. Created attachment 135361 [details]
Patch
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 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 on attachment 135361 [details] Patch Clearing flags on attachment: 135361 Committed r113161: <http://trac.webkit.org/changeset/113161> All reviewed patches have been landed. Closing bug. |