Bug 44828 – FrameView should make more calls to postLayoutTasks() using the timer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

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.

[Beth Dakin]

Created attachment 65838 [details]
Patch

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.

[Beth Dakin]

I just tested, and it appears to be jiggle-free :-)

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.

[Beth Dakin]

(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

[Simon Fraser (smfr)]

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.

[Beth Dakin]

(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.

[Dave Hyatt]

Comment on attachment 65838 [details]
Patch

r=me

[Beth Dakin]

Thanks Dave! http://trac.webkit.org/changeset/66552

[Beth Dakin]

Oops! Forgot the name-change. http://trac.webkit.org/changeset/66555

[David Kilzer (:ddkilzer)]

<rdar://problem/8064938>