Bug 94527 – Remove RenderView::layoutDelta() now that it's no longer needed

• Component: New Bugs
• Status: NEW

[Eric Seidel (no email)]

Look at removing layoutDelta

[Eric Seidel (no email)]

Created attachment 159521 [details]
for the EWS

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 159578 [details]
Another patch for EWS bots

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 159587 [details]
Patch

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Created attachment 159635 [details]
Patch

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Dave 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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Thank you mitz.