[New Multicolumn] Make computeOffsetFromCompositedAncestor() flowthread-aware.

[New Multicolumn] Make computeOffsetFromCompositedAncestor() flowthread-aware.

This function needs to produce the visual offset, and you cannot get that
by calling convertToLayerCoords().

Introducing RenderLayer::visualOffsetFromAncestor() to calculate the
correct visual offset. This also allows for reducing
convertFromFlowThreadToVisualBoundingBoxInAncestor() by a few lines.

We can now add compositing/columns to the virtual/regionbasedmulticol/
testsuite, since both tests there have started to pass.

The fast/multicol/composited-layer-single-fragment.html test is based on work by
andersr@opera.com

The test for hit-testing something 3D-transformed has already been passing
for some time (since https://codereview.chromium.org/625903004), but
actual painting was wrong, until now.

BUG=359877
R=chrishtr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187413

[chrishtr, 2014-12-16 17:33:45]

https://codereview.chromium.org/799403006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/799403006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:1458: if (!paginationLayer ||
paginationLayer == this) {
Do you need to special-case paginationLayer == this?

https://codereview.chromium.org/799403006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:1471: if
(!ancestorLayer->enclosingPaginationLayer() ||
ancestorLayer->enclosingPaginationLayer() != paginationLayer) {
Why are lines 1471 - 1472 not implementable as a recursion into
paginationLayer->visualOffsetFromAncestor(ancestorLayer)?

[mstensho (USE GERRIT), 2014-12-16 22:31:47]

https://codereview.chromium.org/799403006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/799403006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:1458: if (!paginationLayer ||
paginationLayer == this) {
On 2014/12/16 17:33:45, chrishtr wrote:
> Do you need to special-case paginationLayer == this?

If paginationLayer == this, this is the flow thread, and therefore we're not
really inside the columns. So, rather pointless to do the flow-thread-to-visual
point conversions.

Furthermore, with the recursion that you suggested below, it's actually crucial
that we end up here, or we'd get an infinite recursion.

https://codereview.chromium.org/799403006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:1471: if
(!ancestorLayer->enclosingPaginationLayer() ||
ancestorLayer->enclosingPaginationLayer() != paginationLayer) {
On 2014/12/16 17:33:45, chrishtr wrote:
> Why are lines 1471 - 1472 not implementable as a recursion into
> paginationLayer->visualOffsetFromAncestor(ancestorLayer)?

Done.

[chrishtr, 2014-12-17 19:11:00]

https://codereview.chromium.org/799403006/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/799403006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1469: // FIXME: Handle nested
fragmentation contexts (crbug.com/423076). For now just give up if there
Does this FIXME still apply now that there is recursion?

https://codereview.chromium.org/799403006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1477: LayoutPoint
offsetFromPaginationLayerToAncestor;
How about replacing 1477-1479 by:

offset.moveBy(-ancestorLayer->visualOffsetFromAncestor(paginationLayer)) ?

[mstensho (USE GERRIT), 2014-12-17 19:29:53]

https://codereview.chromium.org/799403006/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/799403006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1469: // FIXME: Handle nested
fragmentation contexts (crbug.com/423076). For now just give up if there
On 2014/12/17 19:11:00, chrishtr wrote:
> Does this FIXME still apply now that there is recursion?

Yes, we're still not quite there. I have prepared a CL for that as well, though.
Just need to land this one first.

https://codereview.chromium.org/799403006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1477: LayoutPoint
offsetFromPaginationLayerToAncestor;
On 2014/12/17 19:11:00, chrishtr wrote:
> How about replacing 1477-1479 by:
> 
> offset.moveBy(-ancestorLayer->visualOffsetFromAncestor(paginationLayer)) ?

Done.

[chrishtr, 2014-12-17 19:35:55]

https://codereview.chromium.org/799403006/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/composited-layer-single-fragment.html (right):

https://codereview.chromium.org/799403006/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/composited-layer-single-fragment.html:40: <div
class="element e1"></div>
Can this test be reduced? It's quite large.

https://codereview.chromium.org/799403006/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/799403006/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1454: LayoutPoint
RenderLayer::visualOffsetFromAncestor(const RenderLayer* ancestorLayer) const
Should we generalize convertToLayerCoords rather than adding a new method? Does
it do something conceptually different?

[mstensho (USE GERRIT), 2014-12-17 20:49:27]

https://codereview.chromium.org/799403006/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/799403006/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1454: LayoutPoint
RenderLayer::visualOffsetFromAncestor(const RenderLayer* ancestorLayer) const
On 2014/12/17 19:35:55, chrishtr wrote:
> Should we generalize convertToLayerCoords rather than adding a new method?
Does
> it do something conceptually different?

There are quite a few cases where the (new) multicol implementation in
RenderLayer / LayerPainter expects (non-visual) flow thread coordinates (which
is what converToLayerCoords() gives us). It uses this together with (non-visual)
flow thread rectangles, to figure out the exact fragments to paint, and where to
clip. If we were to use visual coordinates and rectangles everywhere, it would
no longer be possible to reliably express a clip area (e.g. a layer with
overflow:hidden inside multicol) as a rectangle, since blocks that cross a
column boundary has two fragments: one at the bottom of one column and one at
the top of the next one. I think it would be very cumbersome to work with. It's
convenient to work on flow thread coordinates and rectangles as long as and as
much as we can. That's what the render objects do too. I don't think we want to
support non-rectangular render objects. You don't want the line layout code to
be aware of the visual whereabouts of each line box (if one line happens to end
up in the next column), for instance. That's why we have the flow thread, which
represents the multicol container as one single column strip, that can be sliced
and painted into columns during painting.

So I really think we need both.

[chrishtr, 2014-12-17 21:53:03]

On 2014/12/17 at 20:49:27, mstensho wrote:
>
https://codereview.chromium.org/799403006/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/799403006/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:1454: LayoutPoint
RenderLayer::visualOffsetFromAncestor(const RenderLayer* ancestorLayer) const
> On 2014/12/17 19:35:55, chrishtr wrote:
> > Should we generalize convertToLayerCoords rather than adding a new method?
Does
> > it do something conceptually different?
> 
> There are quite a few cases where the (new) multicol implementation in
RenderLayer / LayerPainter expects (non-visual) flow thread coordinates (which
is what converToLayerCoords() gives us). It uses this together with (non-visual)
flow thread rectangles, to figure out the exact fragments to paint, and where to
clip. If we were to use visual coordinates and rectangles everywhere, it would
no longer be possible to reliably express a clip area (e.g. a layer with
overflow:hidden inside multicol) as a rectangle, since blocks that cross a
column boundary has two fragments: one at the bottom of one column and one at
the top of the next one. I think it would be very cumbersome to work with. It's
convenient to work on flow thread coordinates and rectangles as long as and as
much as we can. That's what the render objects do too. I don't think we want to
support non-rectangular render objects. You don't want the line layout code to
be aware of the visual whereabouts of each line box (if one line happens to end
up in the next column), for instance. That's why we have the flow thread, which
represents the multicol container as one single column strip, that can be sliced
and painted into columns during painting.
> 
> So I really think we need both.

Ok.

[mstensho (USE GERRIT), 2014-12-17 22:26:53]

Bleh, uploaded a new patch, but forgot to actually mark the test change as done.

https://codereview.chromium.org/799403006/diff/40001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/composited-layer-single-fragment.html (right):

https://codereview.chromium.org/799403006/diff/40001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/composited-layer-single-fragment.html:40: <div
class="element e1"></div>
On 2014/12/17 19:35:55, chrishtr wrote:
> Can this test be reduced? It's quite large.

Done.

[chrishtr, 2014-12-17 22:31:03]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-12-17 22:31:03]

lgtm

[commit-bot: I haz the power, 2014-12-17 22:31:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/799403006/60001

[commit-bot: I haz the power, 2014-12-18 00:43:54]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187413