|Summary:||FrameView should make more calls to postLayoutTasks() using the timer|
|Product:||WebKit||Reporter:||Beth Dakin <bdakin>|
|Component:||Layout and Rendering||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||bdakin, ddkilzer, mitz, simon.fraser|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Beth Dakin 2010-08-28 12:47:54 PDT
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 10 Beth Dakin 2010-08-31 16:49:47 PDT
Oops! Forgot the name-change. http://trac.webkit.org/changeset/66555