Bug 44828 - FrameView should make more calls to postLayoutTasks() using the timer
Summary: FrameView should make more calls to postLayoutTasks() using the timer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-08-28 12:47 PDT by Beth Dakin
Modified: 2010-09-17 14:16 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.71 KB, patch)
2010-08-28 12:52 PDT, Beth Dakin
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2010-08-28 12:47:54 PDT
Right now, FrameView calls FrameView::postLayoutTasks() either synchronously, or via the postLayoutTasksTimer. If we make more of these calls through the timer, then some scripts will be able to run without hanging WebKit or the UI AND without hitting the JavaScript recursion limit. 

This change only allows synchronous calls to postLayoutTasks() if we are not already in a synchronous call to postLayoutTasks(). Furthermore, it de-couples the notion of "the post layout timer should fire" from m_postLayoutTasksTimer.isActive(), instead using a boolean to track the need to use the timer.
Comment 1 Beth Dakin 2010-08-28 12:52:27 PDT
Created attachment 65838 [details]
Patch
Comment 2 mitz 2010-08-28 13:08:49 PDT
Cool! Have you tested that the sidebar in Safari RSS doesn’t jiggle when you scroll? That’s a good test for making sure that at least sometimes you do perform post-layout tasks synchronously.
Comment 3 Beth Dakin 2010-08-28 13:10:37 PDT
I just tested, and it appears to be jiggle-free :-)
Comment 4 mitz 2010-08-28 13:10:58 PDT
The name m_shouldFirePostLayoutTimer should be improved. It is really about whether you have a pending need to perform post-layout tasks. The timer already knows if it should fire :-) you’re  really using this for the case(s) where you stopped the timer before it had a chance to fire.
Comment 5 Beth Dakin 2010-08-28 13:16:31 PDT
(In reply to comment #4)
> The name m_shouldFirePostLayoutTimer should be improved. It is really about whether you have a pending need to perform post-layout tasks. The timer already knows if it should fire :-) you’re  really using this for the case(s) where you stopped the timer before it had a chance to fire.

Yes, I agree. I wasn't happy with the name. Some ideas:

m_needToPerformPostLayoutTasks
m_hasPostLayoutTasksPending
Comment 6 Simon Fraser (smfr) 2010-08-28 15:13:34 PDT
Comment on attachment 65838 [details]
Patch

There's a lot of statefulness here that makes it very hard to follow. Can we pass state via parameters, rather than keeping it in member variables? Alternatively, maybe a single state enum would be easier to follow than two booleans.
Comment 7 Beth Dakin 2010-08-28 17:03:11 PDT
(In reply to comment #6)
> (From update of attachment 65838 [details])
> There's a lot of statefulness here that makes it very hard to follow. Can we pass state via parameters, rather than keeping it in member variables? Alternatively, maybe a single state enum would be easier to follow than two booleans.

I'm not sure either of those would work. A single state enum seems weird to me since I see the two variables as being very different. m_inSynchronousPostLayout represents whether the current callstack would trace back to a synchronous call to performPostLayoutTakss(). m_shouldFirePostLayoutTimer represents whether we have a pending need to call performPostLayoutTaks() on the timer. I think a single variable representing these two concepts would be confusing.

In terms of passing parameters…it seems like that would be a mess for m_inSynchronousPostLayout. I think it would mean passing a parameter to performPostLayoutTasks(), and having performPostLayoutTasks() send that same parameter into m_frame->eventHandler()->sendResizeEvent(), and keep carrying it as a parameter until it got back to FrameView::layout(). It would be similarly sloppy for m_shouldFirePostLayoutTimer. Again, a member variable seems a lot cleaner to me.
Comment 8 Dave Hyatt 2010-08-31 16:09:01 PDT
Comment on attachment 65838 [details]
Patch

r=me
Comment 9 Beth Dakin 2010-08-31 16:22:49 PDT
Thanks Dave! http://trac.webkit.org/changeset/66552
Comment 10 Beth Dakin 2010-08-31 16:49:47 PDT
Oops! Forgot the name-change. http://trac.webkit.org/changeset/66555
Comment 11 David Kilzer (:ddkilzer) 2010-09-17 14:16:57 PDT
<rdar://problem/8064938>