Bug 139062

Summary: [GTK] Timers might never be fired during animations
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, pnormand, svillar, zan
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mrobinson: review+

Description Carlos Garcia Campos 2014-11-26 06:26:11 PST
This can happen in old/slow machines where the time to render layers might take more than 0.016. Since the layer flush timer is using a higher priority than WebCore timers, when scheduling all (or several) layer flushes immediately, no other sources with lower priority are dispatched in the main loop. I could reproduce this in an old machine running the outlook web application that uses a spinner animation during the load. The spinner starts very early and the application adds an even listener for the load event that is never emitted, because fonts are loaded on demand by the CSS font selector using a timer that is never fired.
Comment 1 Carlos Garcia Campos 2014-11-26 06:34:37 PST
Created attachment 242226 [details]
Patch
Comment 2 Martin Robinson 2014-12-11 02:35:24 PST
Comment on attachment 242226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242226&action=review

Looks good to me. I have a couple readability suggestions, but I think the general approach is just fine. Though please double-check that this doesn't harm animations while resizing on faster machines. Thanks!

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:66
> +// Use a higher priority than WebCore timers.
> +static const int layerFlushTimerPriority = GDK_PRIORITY_REDRAW - 1;
> +

Maybe just move this down to right before it is used?

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:264
> +    static const double targetDelay = 1 / 60.0;

I would call this targetFramerate or something like that.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:270
> +    static const double maxDurationImmediateFlush = 0.100;

Maybe call this time maxDurationOfImmediateFlushes.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:272
> +    double nextFlush = std::max(targetDelay - (current - fireTime), 0.0);

I would call this timeToNextFlush.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:281
> +    if (m_lastImmediateFlushTime && m_lastImmediateFlushTime + maxDurationImmediateFlush < current) {
> +        nextFlush = targetDelay;
> +        m_lastImmediateFlushTime = 0;
>      }

Would it be possible to move this to a helper called shouldSkipAFrameBecauseOfContinousImmediateFlushes? I know the name is super silly, but I think it would help the readability of the code here.
Comment 3 Carlos Garcia Campos 2014-12-11 03:21:57 PST
Committed r177144: <http://trac.webkit.org/changeset/177144>