Bug 15129

Created attachment 16175 [details] test case to demonstrate the problem -- easiest to see on OS X with Quartz Debug

The problem is that the object gets its size while it's not yet at its correct position. The RenderObject::repaintAfterLayoutIfNeeded function does the repaint once it has its correct size. Later, RenderBlock::layoutBlockChildren sets the correct position, and then correctly repaints it at its new location. If the object being laid out had some way of knowing that it was inside RenderBlock::layoutBlockChildren, which was going to take care of the repaint, then we could have avoided the repaint done by repaintAfterLayoutIfNeeded. I've been thinking about multiple ways to fix this. For the symptoms of this particular bug, the only case that we need to optimize is a RenderBlock inside another RenderBlock. 1) Detect in RenderBlock::layoutBlockChildren that the child's old position is really "nowhere" (perhaps by noticing that it has 0 width, 0 height, and no borders), and in that case actually change the Y position to yPosEstimate instead of using the layout delta. This would make the optimization specific to brand new objects. 2) Put all new render objects into a "no repaint" mode by setting a bit, then take them all out of that mode at the end of the first layout, and repaint them then. 3) Start new render objects at some crazy "-10000, -10000" position, and let them repaint all they want far offscreen before they get their correct position. 4) Have RenderBlock::layoutBlockChildren pass a parameter, or set a bit on the child, to indicate that it will take responsibility for repaints due to geometry changes to avoid repaints during the layout of that child. I don't know if there's a bit available, and it's hard for me to be sure that the repaint code in layoutBlockChildren that uses repaintDuringLayoutIfMoved is a superset of what the child's layoutBlock would do. An optimization like (4) seems strongest, because it deals with the general case of objects that both move and change size, which seems like it's probably not all that unusual. But it seems like the hardest one to do.

(2) seems ok to me actually. There might be other interesting things we could do if we knew the object was getting a layout for the first time.

Created attachment 16177 [details] patch with ChangeLog, but no repaint test yet (because I don't know how yet)

Note that if I didn't check "oldRect.isEmpty()" I would optimize more cases, but the cost of more calls to absoluteRepaintRect() might be too great.

Comment on attachment 16177 [details] patch with ChangeLog, but no repaint test yet (because I don't know how yet) + RenderView* view = static_cast<RenderView*>(document()->renderer()); Seeing this made me wonder why you weren't using RenderObject::view(), which in turn made me wonder why that function isn't inlined. Have you considered asserting that m_objectNotToRepaintDuringLayout is 0 at the end of RenderView::layout()? I am slightly uncomfortable with the word "invisible" in "childWasInvisible" because of the CSS notion of visibility, but I can't come up with a good alternative. The only substantial issue I have is the increase in stack size for savedObjectNotToRepaintDuringLayout. I think further analysis may show that it is sufficient to only keep the "stack depth" in the RenderView and return false from checkForRepaintDuringLayout whenever the depth is positive. But that can be changed at a later time.

(In reply to comment #6) > (From update of attachment 16177 [details] [edit]) > + RenderView* view = static_cast<RenderView*>(document()->renderer()); > > Seeing this made me wonder why you weren't using RenderObject::view(), which in > turn made me wonder why that function isn't inlined. Because it's not inlined, I guess. > Have you considered asserting that m_objectNotToRepaintDuringLayout is 0 at the > end of RenderView::layout()? Sure, that'd be fine. > I am slightly uncomfortable with the word "invisible" in "childWasInvisible" > because of the CSS notion of visibility, but I can't come up with a good > alternative. I'm sure I can come up with another name. > The only substantial issue I have is the increase in stack size for > savedObjectNotToRepaintDuringLayout. I think further analysis may show that it > is sufficient to only keep the "stack depth" in the RenderView and return false > from checkForRepaintDuringLayout whenever the depth is positive. But that can > be changed at a later time. I noticed generally layoutBlockChildren() is quite sloppy about using stack size. The local variables top, bottom, marginInfo, compactInfo, legend, previousFloatBottom, oldTopPosMargin, oldTopNegMargin, handled, yPosEstimate, oldRect, postCollapseChildY, finalChildX, and finalChildY clearly don't all need to exist, and the ones that do exist don't all need to be occupying stack when making the recursive call to child->layoutIfNeeded. I don't feel as bad about adding another local given that sloppiness; might be worth tightening up in general though. I like the idea of not needing the global stack depth, but Hyatt and I have discussed more radical changes to repaint that we'd probably do that would probably obviate that further analysis. He's amenable to a separate pass of repaint based on data gathered during the layout process.

Comment on attachment 16177 [details] patch with ChangeLog, but no repaint test yet (because I don't know how yet) You need to make sure you don't call absoluteClippedOverflowRect when a full repaint is happening. If checkForRepaintDuringLayout is false, then you need to avoid making that extra call.

See also commit log for: http://trac.webkit.org/changeset/34128

Created attachment 23361 [details] Use the m_everHadLayout flag to suppress some unnecessary painting A patch using the newly-added m_everHadLayout flag. It fixes this bug and does not regress and fast/repaint tests, but I am not sure it does not introduce repaint regressions, so I am going to try to live with it for a while and also actively think about what can go wrong.

Created attachment 23380 [details] Use the m_everHadLayout flag to suppress some unnecessary painting Slightly more conservative

Comment on attachment 23380 [details] Use the m_everHadLayout flag to suppress some unnecessary painting r=me I don't understand why this patch changes layout test results. I also wish we had a good mechanism for testing these kinds of changes that didn't require comparing images to see if the test passes.

(In reply to comment #12) > (From update of attachment 23380 [details] [edit]) > r=me > > I don't understand why this patch changes layout test results. It changes the pixel result of a repaint test, because there is less excessive repainting. I thought that was a good thing. If that test did not exist I would need to invent it for this patch. > I also wish we had a good mechanism for testing these kinds of changes that > didn't require comparing images to see if the test passes. Maybe as part of addressing bug 14524 there will be a mechanism in the engine for getting invalidated regions, which DumpRenderTree could hook into and use to dump a textual representation of the invalidated region. Thanks for the review!

Fixed in <http://trac.webkit.org/changeset/36515>.

bug 92800 is essentially noting that this optimization does not work for a newly added block containing a table.