Bug 139062 – [GTK] Timers might never be fired during animations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 242226 [details]
Patch

[Martin Robinson]

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.

[Carlos Garcia Campos]

Committed r177144: <http://trac.webkit.org/changeset/177144>