Bug 139062 - [GTK] Timers might never be fired during animations
Summary: [GTK] Timers might never be fired during animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2014-11-26 06:26 PST by Carlos Garcia Campos
Modified: 2014-12-11 03:21 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.80 KB, patch)
2014-11-26 06:34 PST, Carlos Garcia Campos
mrobinson: review+
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 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>