Does https://codereview.chromium.org/1907443003/ solve your problem?

Excellent! Your change fixes the test case above, and most of the visual rect bugs in the layout test results above as well. There are still five layout tests failing with the visual rect patch, but that should not block your change landing. They all look specific to composited layers.

[1/5] fast/multicol/composited-layer-nested.html failed unexpectedly (reference mismatch)
[2/5] fast/multicol/composited-layer-multiple-fragments.html failed unexpectedly (reference mismatch)
[3/5] fast/multicol/composited-relpos-resize.html failed unexpectedly (reference mismatch)
[4/5] fast/multicol/composited-layer-will-change.html failed unexpectedly (reference mismatch)
[5/5] fast/multicol/composited-layer-multiple-fragments-translated.html failed unexpectedly (reference mismatch)

Attached are the results. I am happy to look further or open to your further insight/work. To reproduce patch http://crrev.com/1484163002 and http://crrev.com/1907443003 and run fast/multicol layout tests.

Deleted: layout-test-results.zip 235 KB

layout-test-results.zip 235 KB Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e60405b884e8e5ac517a27e03a955a0f3a4f2c98

commit e60405b884e8e5ac517a27e03a955a0f3a4f2c98
Author: mstensho <mstensho@opera.com>
Date: Thu Apr 21 04:24:31 2016

Translate flow thread coords to the nearest enclosing coord space when appropriate.

We used to always convert to the visual coordinate space, meaning that we
walked all enclosing fragmentation contexts. However, only the PaintLayer code
wants this behavior, while everyone else typically wants to do one
fragmentation context at a time, e.g. when walking the ancestry with
LayoutObject::mapLocalToAncestor().

For nested multicol, this caused invalidation bugs, problems with
getClientRects(), and maybe more.

Added an enum CoordinateSpaceConversion (with values "Containing" and "Visual")
for flowThreadTranslationAtOffset() to use to determine which conversion to
perform. The old behavior was to always do CoordinateSpaceConversion::Visual.

BUG= 604883 

Review URL: https://codereview.chromium.org/1907443003

Cr-Commit-Position: refs/heads/master@{#388692}

[add] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/LayoutTests/fast/multicol/client-rect-nested-expected.txt
[add] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/LayoutTests/fast/multicol/client-rect-nested.html
[add] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/LayoutTests/fast/repaint/multicol-nested-expected.html
[add] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/LayoutTests/fast/repaint/multicol-nested.html
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/LayoutFlowThread.h
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.cpp
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/LayoutMultiColumnFlowThread.h
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.h
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp
[modify] https://crrev.com/e60405b884e8e5ac517a27e03a955a0f3a4f2c98/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h

I think I need help from someone who knows the painting code better. Here's a simplified test that fails with http://crrev.com/1484163002 applied:

<!DOCTYPE html>
<p>There should be a green square below.</p>
<div style="columns:2; column-fill:auto; width:200px; height:50px;">
    <div style="will-change:transform; padding-top:50px;">
        <div style="display:table; width:20px; height:20px; background:green;"></div>
    </div>
</div>

The table that constitutes the green square is missing. Note that it doesn't really have to be a table (it can be a regular block instead). That's just something I did to make it easier to set breakpoints.

I get to TablePainter::paintBoxDecorationBackground(), and it seems to attempt to paint, so I don't know why we aren't seeing anything.

Another observation: If I simply add a background to the composited object, painting stuff inside seems to work slightly better:

<div style="columns:2; column-fill:auto; width:200px; height:50px;">
    <div style="will-change:transform; padding-top:50px; background:yellow;">
        <div style="display:table; width:20px; height:20px; background:green;"></div>
    </div>
</div>

Now we see the green square. Let's increase the top padding from 50px to 63px.

<div style="columns:2; column-fill:auto; width:200px; height:50px;">
    <div style="will-change:transform; padding-top:63px; background:yellow;">
        <div style="display:table; width:20px; height:20px; background:green;"></div>
    </div>
</div>

Still fine. Now increase it to 64px:

<div style="columns:2; column-fill:auto; width:200px; height:50px;">
    <div style="will-change:transform; padding-top:64px; background:yellow;">
        <div style="display:table; width:20px; height:20px; background:green;"></div>
    </div>
</div>

And the square is gone! But the yellow background is still there. There seems to be something going on with multiples of 64?

Any help? Where's the code that fails to put the square on the screen?

With http://crrev.com/1484163002 add logging to PaintArtifact.cpp:102 such as:

LOG(ERROR) << "PaintArtifact: item=" << displayItem.asDebugString() << ", visualRect=" << m_displayItemList.visualRect(visualRectIndex).toString();

and the visual rect for the clip and background look like they're not in the space of their containing GraphicsLayer, note this is your initial test case but w/o the <p> text:

[1:1:0506/133243:1468410439303:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x251fcc14a660 LayoutView #document", type: "DrawingDebugRedFill", rect: [0.000000,0.000000,800.000000,600.000000]}, visualRect=0,0 800x600
[1:1:0506/133243:1468410439630:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc804018 LayoutView #document", type: "DrawingDocumentBackground", rect: [0.000000,0.000000,800.000000,600.000000]}, visualRect=0,0 800x600
[1:1:0506/133243:1468410439960:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc814108 LayoutBlockFlow HTML", type: "Subsequence"}, visualRect=0,0 800x66
[1:1:0506/133243:1468410440201:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc830018 LayoutMultiColumnSet (anonymous)", type: "DrawingBoxDecorationBackground"}, visualRect=8,8 200x50
[1:1:0506/133243:1468410440463:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc814108 LayoutBlockFlow HTML", type: "EndSubsequence"}, visualRect=0,0 800x66
* [1:1:0506/133243:1468410441009:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc818360 LayoutBlockFlow DIV", type: "ClipLayerFragmentPaintPhaseChildBlockBackgrounds", skippedCache: true, scope: 4, clipRect: [100,0,100,20]}, visualRect=0,0 92x70
* [1:1:0506/133243:1468410441223:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc838018 LayoutTable DIV", type: "DrawingBoxDecorationBackground", skippedCache: true, scope: 4, rect: [108.000000,0.000000,20.000000,20.000000]}, visualRect=0,50 20x20
[1:1:0506/133243:1468410441440:ERROR:PaintArtifact.cpp(102)] PaintArtifact: item={client: "0x2aafcc818360 LayoutBlockFlow DIV", type: "EndClipLayerFragmentPaintPhaseChildBlockBackgrounds", skippedCache: true, scope: 4}, visualRect=0,0 92x70

I would expect a correct visual rect for the second * item above to be at x-pos around 100. So I expect that somehow we're missing a translation or similar. visualRect() impl for LayoutTable reduces to LayoutObject::visualRect() which just defers to previousPaintInvalidationRect(), perhaps the paint invalidation rect is similarly incorrect and we are just getting lucky invalidation-wise without the patch  due to container being invalidated and encompassing.

The patch under test that actually makes use of the visual rects alters cc-side rastering logic by querying for DisplayItems to raster based on their visual rect. That rect is intended to be in the space of its containing GraphicsLayer. So if it is not, then we won't pick it up in the RTree query and we won't raster it.

I haven't double checked yet but I expect your fiddling with padding ends up leading to some DisplayItems being painted due to moving their visual rect to where, when we query in cc/playback/display_item_list.cc Raster(), the visual rect intersects with the canvas_playback_rect. Can add logging to display_item_list.cc if helpful as well.

We have been working through fixing visual rects suffering from similar computation issues for a while, unfortunately fixes have been different in most every case. It could be that we're missing some logic around fragments/pagination. We had to previously fix an issue where we weren't accounting for offsetFromLayoutObject in composited layers, for example: http://crrev.com/1669933002.

I will look further but wanted to add some initial notes.

So we're just using LayoutObject::m_previousPaintInvalidationRect. What is it? 

Looks like the documentation of previousPaintInvalidationRectIncludingCompositedScrolling() may be of help:

    // The previous paint invalidation rect, in the the space of the paint invalidation container (*not* the graphics layer that paints
    // this object).

There are two invalidation containers in this document. The LayoutView root, and the LayoutBlockFlow that establishes a transform. The table is inside the transform, and the transform LayoutBlockFlow lives inside a flow thread, so if the documentation is correct, the m_previousPaintInvalidationRect of anything inside the transformed block should be in flow thread coordinates (i.e. not visual) as well. The LayoutTable has {m_location = LayoutPoint(0px, 50px), m_size = LayoutSize(20px, 20px)}, so that seems correct according to the documentation, since the invalidation container of the table is the transformed block, which lives in the flow thread. The invalidation container of the transformed block, on the other hand, is the LayoutView. Does that mean that m_previousPaintInvalidationRect should be in visual coordinates? Anyway, it is not. It is {m_location = LayoutPoint(0px, 0px), m_size = LayoutSize(92px, 70px)}, i.e. in flow thread coordinates relative to the flow thread.

What's very obvious, though, is that LayoutObject::visualRect() doesn't return visual rectangles.

m_previousPaintInvalidationRect is in physical coordinates ("visual").
The comment you referenced is meant to explain that it's in the physical coordinates relative to the paint invalidation container. This can be different than that of the
GraphicsLayer which backs the paint invalidation container since, for technical reasons,
there may be a translation offset involved.

The comments also needs update for squashed contents because for them the rect is in the real backing instead of the paint invalidation container.

TL;DR: composited layers shouldn't be fragmented; the example from comment 4 is one example. I think the cleanest
solution is to fix the problem directly by not fragmenting them, as discussed on paint-dev@.

Second TL;DR is that Morten's commentary in #6 is correct about the details.

Repeating the example:

<!DOCTYPE html>
<p>There should be a green square below.</p>
<div style="columns:2; column-fill:auto; width:200px; height:50px;">
    <div id="parent" style="will-change:transform; padding-top:50px;">
        <div id="child" style="display:table; width:20px; height:20px; background:green;"></div>
    </div>
</div>

To recap/add more points:

1. The composited bounds of "parent" are set up to be both columns.
2. It's the paint invalidation container of "child".
3. The paint invalidation rect of "child" does not leave flow thread coordinates, since we compute this rect in the 
space of the paint invalidation container. Since both are inside the multi-column container, there is no fragmentation
happening.

In principle we could figure out how to map to the bounds of the composited layer for "parent", but it goes against the desired
architecture of the current system, and also is in conflict with the plan of record for SPv2. So we should simply refuse to fragment
composited content.

And BTW, as expected, paint invalidation in general is totally busted right
now on that example. Try for example this in the console:

child.style.background = 'red'

These two multicol tests are still failing with latest patch and ToT:

fast/multicol/column-rules-fixed-height.html (non-composited)
[new] fast/multicol/composited-with-child-layer-in-next-column.html

The former just has to be investigated.

The latter looks like one of the recently updated ones as part of  http://crbug.com/616287  so presumably though it is now not fragmented it still renders differently with patch applied to use visual rects, thus likely paint invalidation/visual rect is incorrect.

The column rules are painted once for each column. We need to union all of their
bounds when computing a paint invalidation rect.

(that is the solution to fast/multicol/column-rules-fixed-height.html at least)

fast/multicol/composited-with-child-layer-in-next-column.html is something else.

fast/multicol/composited-with-child-layer-in-next-column.html looks different
only up to subpixels. It looks to me like it should just be rebaselined.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77c27204cf3e0094cdc42337d9780d0246f192ed

commit 77c27204cf3e0094cdc42337d9780d0246f192ed
Author: chrishtr <chrishtr@chromium.org>
Date: Sat Jul 02 03:35:47 2016

Include column rules for LayoutMultiColumnSet paint invalidation rects.

BUG= 604883 

Review-Url: https://codereview.chromium.org/2115013002
Cr-Commit-Position: refs/heads/master@{#403601}

[modify] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/ed79618bc8706c583c4f693c4a3a3bc4a2678fcf/third_party/WebKit/LayoutTests/fast/multicol/column-rules-fixed-height-expected.html
[add] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/LayoutTests/fast/multicol/column-rules-fixed-height-expected.png
[add] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/LayoutTests/fast/multicol/column-rules-fixed-height-expected.txt
[modify] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/LayoutTests/fast/multicol/column-rules-fixed-height.html
[modify] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp
[modify] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.h
[modify] https://crrev.com/77c27204cf3e0094cdc42337d9780d0246f192ed/third_party/WebKit/Source/core/paint/MultiColumnSetPainter.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5fa9cf39496b97e0db21e27b5949bc2efca0d60

commit a5fa9cf39496b97e0db21e27b5949bc2efca0d60
Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org>
Date: Sat Jul 02 09:04:18 2016

Auto-rebaseline for r403601

https://chromium.googlesource.com/chromium/src/+/77c27204c

BUG= 604883 
TBR=chrishtr@chromium.org

Review URL: https://codereview.chromium.org/2118003002 .

Cr-Commit-Position: refs/heads/master@{#403610}

[modify] https://crrev.com/a5fa9cf39496b97e0db21e27b5949bc2efca0d60/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/a5fa9cf39496b97e0db21e27b5949bc2efca0d60/third_party/WebKit/LayoutTests/fast/multicol/column-rules-fixed-height-expected.txt
[rename] https://crrev.com/a5fa9cf39496b97e0db21e27b5949bc2efca0d60/third_party/WebKit/LayoutTests/platform/linux/fast/multicol/column-rules-fixed-height-expected.png
[add] https://crrev.com/a5fa9cf39496b97e0db21e27b5949bc2efca0d60/third_party/WebKit/LayoutTests/platform/linux/fast/multicol/column-rules-fixed-height-expected.txt
[add] https://crrev.com/a5fa9cf39496b97e0db21e27b5949bc2efca0d60/third_party/WebKit/LayoutTests/platform/mac/fast/multicol/column-rules-fixed-height-expected.png
[add] https://crrev.com/a5fa9cf39496b97e0db21e27b5949bc2efca0d60/third_party/WebKit/LayoutTests/platform/win/fast/multicol/column-rules-fixed-height-expected.png

Added rebaseline for fast/multicol/composited-with-child-layer-in-next-column.html to http://crrev.com/1484163002.

Should this still be blocked on 616287?

No, not blocked on 616287. Also marking as fixed since the other rebaseline
is now tracked elsewhere, per your comment.