Bug 94527

Created attachment 159521 [details] for the EWS

Attachment 159521 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/RenderView.h:123: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

On my Mac Lion box, I see 4 changes with this patch: fast/repaint/add-table-overpaint.html - FIXED! fast/repaint/bugzilla-3509.html -Underpainting :( fast/repaint/float-in-new-block-with-layout-delta.html - FIXED! fast/repaint/table-cell-move.html - Overpainting. :( Investigating.

Comment on attachment 159521 [details] for the EWS Attachment 159521 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13543513 New failing tests: fast/table/border-collapsing/cached-cell-append.html fast/repaint/table-cell-move.html fast/repaint/add-table-overpaint.html fast/repaint/table-extra-bottom-grow.html fast/repaint/bugzilla-3509.html fast/repaint/float-in-new-block-with-layout-delta.html

Created attachment 159547 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

fast/repaint/bugzilla-3509.html underpaints because margin collapsing happens after child layout. (See RenderBlock#2420) We don't invalidate the child if we move it here. We probably should.

I believe this block in RenderBlock::layoutBlockChild should be either marking the child for layout (since we moved it!) or telling it to repaint: // Now we have a final top position. See if it really does end up being different from our estimate. // clearFloatsIfNeeded can also mark the child as needing a layout even though we didn't move. This happens // when collapseMargins dynamically adds overhanging floats because of a child with negative margins. if (logicalTopAfterClear != logicalTopEstimate || child->needsLayout()) { if (child->shrinkToAvoidFloats()) { // The child's width depends on the line width. // When the child shifts to clear an item, its width can // change (because it has more available line width). // So go ahead and mark the item as dirty. child->setChildNeedsLayout(true, MarkOnlyThis); } if (childRenderBlock) { if (!child->avoidsFloats() && childRenderBlock->containsFloats()) childRenderBlock->markAllDescendantsWithFloatsForLayout(); if (!child->needsLayout()) child->markForPaginationRelayoutIfNeeded(); } // Our guess was wrong. Make the child lay itself out again. child->layoutIfNeeded(); } I'm not sure which.

Created attachment 159578 [details] Another patch for EWS bots

I'm not sure my repaint() call is in quite the right place. It's possible that LayoutSize childOffset = child->location() - oldRect.location(); if (childOffset.width() || childOffset.height()) { // If the child moved, we have to repaint it as well as any floating/positioned // descendants. An exception is if we need a layout. In this case, we know we're going to // repaint ourselves (and the child) anyway. if (childHadLayout && !selfNeedsLayout() && child->checkForRepaintDuringLayout()) child->repaintDuringLayoutIfMoved(oldRect); } a few lines lower is supposed to have done the repaint for me. I'll investigate again in the debugger.

Created attachment 159587 [details] Patch

I expect there may be a couple png rebaselines needed. As soon as they're provided by the cr-linux EWS, I'll update the patch. I'd appreciate your review, Señors Hyatt or Mitz.

Comment on attachment 159587 [details] Patch Attachment 159587 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13539897 New failing tests: fast/repaint/bugzilla-3509.html fast/repaint/table-cell-move.html fast/repaint/add-table-overpaint.html fast/repaint/float-in-new-block-with-layout-delta.html fast/repaint/table-extra-bottom-grow.html

Created attachment 159609 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 159635 [details] Patch

This patch should pass the cr-linux EWS. If it doesn't, i'll update it again. :)

Looks like the EWS bots believe this is ready for landing, if you agree mitz/hyatt?

Comment on attachment 159635 [details] Patch There are a few things making me uncomfortable about this (and I really think we want Dan's input here). (1) You have a bunch of results that are repainting tightly now overpainting. These test cases are obviously regressing. (2) I'm not convinced the one multi-pass spot you patched is good enough. We have numerous other places where we do multi-pass layout. Do they all have to be patched? If they do all have to be patched, is that the precise problem layoutDelta was trying to solve? I think we absolutely need to get Dan's input here, since he seems to be the only one who knows what layoutDelta's purpose really is.

(In reply to comment #17) > (From update of attachment 159635 [details]) > There are a few things making me uncomfortable about this (and I really think we want Dan's input here). > > (1) You have a bunch of results that are repainting tightly now overpainting. These test cases are obviously regressing. Yup. Several which are obviously progressing as well. I'm happy to work on making them overpaint less, either as part of this patch or another. > (2) I'm not convinced the one multi-pass spot you patched is good enough. We have numerous other places where we do multi-pass layout. Do they all have to be patched? If they do all have to be patched, is that the precise problem layoutDelta was trying to solve? Perhaps. I have to trust our testing here. If it's insufficient, we'll have to make it better. > I think we absolutely need to get Dan's input here, since he seems to be the only one who knows what layoutDelta's purpose really is. Yup. I've emailed him.

The patch is incorrect. You can see this if you take fast/repaint/bugzilla-3509.html and modify it so that the "im" div starts out with a width of 100px and repaintTest() shrinks it to 50px. Now the newly added call to repaint() can only invalidate the new size of 50×100 at the correct position, but the other 50×100 pixels that are no longer part of the object were not invalidated correctly, because that invalidation happened when the object was at an intermediate position. This is exactly the sort of thing layoutDelta was meant to address.

Thank you mitz.