Bug 90018 - [Gtk] too long function in animation script cause repaint to never happen.
Summary: [Gtk] too long function in animation script cause repaint to never happen.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: arno.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-26 15:04 PDT by arno.
Modified: 2017-03-11 10:45 PST (History)
6 users (show)

See Also:


Attachments
testcase (42 bytes, text/plain)
2012-06-26 15:09 PDT, arno.
no flags Details
testcase (920 bytes, text/html)
2012-06-26 15:10 PDT, arno.
no flags Details
patch proposal; use glib loop instead WebKit shared timer for paint timer (8.14 KB, patch)
2012-06-26 16:57 PDT, arno.
no flags Details | Formatted Diff | Diff
Patch (9.92 KB, patch)
2012-10-01 13:26 PDT, arno.
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2012-10-22 13:17 PDT, arno.
no flags Details | Formatted Diff | Diff
actually, it looks like we need remove the sources. For some reason, the video did not work with previous patch. (8.26 KB, patch)
2012-10-23 15:05 PDT, arno.
no flags Details | Formatted Diff | Diff
sorry, wrong patch. This is the correct one (8.41 KB, patch)
2012-10-23 15:15 PDT, arno.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 2012-06-26 15:04:40 PDT
Hi,
currently, timers are implemented as a heap, whichever is scheduled first is executed first.
Additionally, when a paint is in the heap, but a new paint is scheduled, the paint timer is just rescheduled in the heap.
Also, on some dom operations (may be all of them, but I'm not sure), a layout timer is scheduled, but also a paint timer.
Last, when layout timer is fired, it schedule a paint.

So, if you have a timer (defined with webkitRequestAnimationFrame) which takes a long time to execute, modify the dom, and defines a new timer, the repaint never happens.

After adding an animation with window.webkitRequestAnimationFrame, the timer heap is:

|animation|

During animation, a layout then a repaint are scheduled. And also a new animation. If animation has been running long enough (more than MinimumAnimationInterval), animation is schedule as soon as possible (right after layout and paint). Timer heap is:

|layout paint animation|

When layout timer is fired, it also schedule a paint as soon as possible. But this is after animation though. So, the paint timer is moved in the heap. heap is then:

|animation paint|

Then, animation is executed, layout, paint and animations are scheduled again. heap is:

|layout paint animation|

and the loop starts over. Paint timer is never fired.

If loop executes fast enough, the next animation is scheduled long enough in the future, and the paint scheduled by the layout happens before this animation.
Comment 1 arno. 2012-06-26 15:09:50 PDT
Created attachment 149612 [details]
testcase
Comment 2 arno. 2012-06-26 15:10:45 PDT
Created attachment 149613 [details]
testcase
Comment 3 arno. 2012-06-26 16:57:44 PDT
Created attachment 149634 [details]
patch proposal; use glib loop instead WebKit shared timer for paint timer
Comment 4 arno. 2012-06-26 17:49:49 PDT
add kov as he recently reviewed patches in ChromeClientGtk.cpp
Comment 5 arno. 2012-10-01 13:26:30 PDT
Created attachment 166531 [details]
Patch

updated patch to build on recent trunk
Comment 6 Gustavo Noronha (kov) 2012-10-22 12:01:25 PDT
Comment on attachment 166531 [details]
Patch

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

You have empty entries on top of the actual entries in the changelogs. I'm wondering, why do you remove the source and add a new one when the timer already exists? Doesn't just waiting for the already scheduled run to happen work?

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:571
> +

Empty new line.
Comment 7 arno. 2012-10-22 13:17:18 PDT
Created attachment 169964 [details]
Patch

updated patch to address comments
Comment 8 arno. 2012-10-23 15:05:08 PDT
Created attachment 170243 [details]
actually, it looks like we need remove the sources. For some reason, the video did not work with previous patch.

updated patch to address comments
Comment 9 arno. 2012-10-23 15:15:02 PDT
Created attachment 170247 [details]
sorry, wrong patch. This is the correct one

updated patch
Comment 10 Andreas Kling 2014-02-05 11:03:49 PST
Comment on attachment 170247 [details]
sorry, wrong patch. This is the correct one

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.