[New Multicolumn] Make layer bounding boxes visual and relative to "root".

[New Multicolumn] Make layer bounding boxes visual and relative to "root".

The bounding box needs to be the union of all the box's fragments.
There's one fragment for each column in which the box occurs.

In addition to being a union of all fragments, the bounding box needs to
be relative to the "root"/"ancestor" layer. If the root layer is also
inside the fragmentation context, we need to find the visual position of
the root layer as well (which may very well be in a different column than
the layer we're working on).

A lot was already almost correct in RenderLayer::fragmentsBoundingBox(),
while it was completely missing in RenderLayer::boundingBoxForCompositing().

Added a helper convertFromFlowThreadToVisualBoundingBoxInAncestor() to
deal with this piece of advanced technology in one place.

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

[mstensho (USE GERRIT), 2014-10-10 11:24:28]

mstensho@opera.com changed reviewers:
+ abarth@chromium.org, chrishtr@chromium.org, jchaffraix@chromium.org

[rune, 2014-10-13 09:35:02]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2014-10-13 09:35:03]

lgtm with comment suggestion, but jchaffraix should have a say.

It's hard to wrap ones head around these coordinate system transforms. It would
be awesome if we were able to make this more understandable. I don't have a
magic solution to it.

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

https://codereview.chromium.org/645023002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:443: // extra work to find the visual
distance from the ancestor.
I think something like this is clearer:

// The ancestor layer is inside the pagination layer, so we need to subtract the
visual distance from the pagination layout to the ancestor layer.

[rune, 2014-10-13 09:40:40]

https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/co...
File LayoutTests/fast/multicol/composited-relpos.html (right):

https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos.html:4: <div
style="transform:translateZ(0); position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
If "will-change: transform" does the same as "transform:translateZ(0)", that's
the kosher way these days.

[mstensho (USE GERRIT), 2014-10-13 11:21:27]

https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/co...
File LayoutTests/fast/multicol/composited-relpos.html (right):

https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos.html:4: <div
style="transform:translateZ(0); position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
On 2014/10/13 09:40:40, rune wrote:
> If "will-change: transform" does the same as "transform:translateZ(0)", that's
> the kosher way these days.

Done.

Kept transform:translateZ(0) in
fast/multicol/composited-opacity-2nd-and-3rd-column.html , because strange
things started to happen if I did will-change:transform instead. Will deal with
that in a follow-up CL.

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

https://codereview.chromium.org/645023002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:443: // extra work to find the visual
distance from the ancestor.
On 2014/10/13 09:35:02, rune wrote:
> I think something like this is clearer:
> 
> // The ancestor layer is inside the pagination layer, so we need to subtract
the
> visual distance from the pagination layout to the ancestor layer.

Done.

[rune, 2014-10-13 11:25:08]

https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/co...
File LayoutTests/fast/multicol/composited-relpos.html (right):

https://codereview.chromium.org/645023002/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos.html:4: <div
style="transform:translateZ(0); position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
On 2014/10/13 11:21:27, mstensho wrote:
> On 2014/10/13 09:40:40, rune wrote:
> > If "will-change: transform" does the same as "transform:translateZ(0)",
that's
> > the kosher way these days.
> 
> Done.
> 
> Kept transform:translateZ(0) in
> fast/multicol/composited-opacity-2nd-and-3rd-column.html , because strange
> things started to happen if I did will-change:transform instead. Will deal
with
> that in a follow-up CL.

Acknowledged.

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

https://codereview.chromium.org/645023002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:443: // extra work to find the visual
distance from the ancestor.
On 2014/10/13 11:21:27, mstensho wrote:
> On 2014/10/13 09:35:02, rune wrote:
> > I think something like this is clearer:
> > 
> > // The ancestor layer is inside the pagination layer, so we need to subtract
> the
> > visual distance from the pagination layout to the ancestor layer.
> 
> Done.

Acknowledged.

[chrishtr, 2014-10-13 16:03:33]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2014-10-13 16:03:34]

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:428:
rect.moveBy(offsetWithinPaginationLayer);
Why is this just an offset in all cases? What if there is a transform between?

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:447:
rect.moveBy(-offsetFromPaginationLayerToAncestor);
Same question here. Why not use something like
RenderLayer::mapRectToPaintInvalidationBacking? (but suitably generalized)

[chrishtr, 2014-10-13 16:03:43]

The CQ bit was unchecked by chrishtr@chromium.org

[Julien - ping for review, 2014-10-13 19:25:03]

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-relpos-clipped.html (right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div
style="will-change:transform; position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
You forgot this will-change:transform.

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-relpos.html (right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-relpos.html:4: <div
style="will-change:transform; position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
Ditto.

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html
(right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html:4:
<div style="will-change:transform; position:absolute; left:0; right:0;
height:9em; background:cyan;"></div>
Ditto.

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html:17:
</div>
I am concerned about these tests that seems very fragile to me as both are
relying on the multi-column to work (ie if it's broken, it's going to be broken
in the same way).

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:428:
rect.moveBy(offsetWithinPaginationLayer);
On 2014/10/13 at 16:03:34, chrishtr wrote:
> Why is this just an offset in all cases? What if there is a transform between?

convertToLayerCoords is expected not to call across transformed layers as it
wouldn't work (which is what you're pointing out). In practice however this
invariant doesn't hold (I tried enforcing it a year or 2 ago and it's violated
all the time) so I don't know how our transform code even works.

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:436: // FIXME: nested fragmentation
contexts. For now just give up if there are different pagination layers
involved.
Let's put the bug number for that as it will make it easier to spot this FIXME.

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:447:
rect.moveBy(-offsetFromPaginationLayerToAncestor);
On 2014/10/13 at 16:03:34, chrishtr wrote:
> Same question here. Why not use something like
RenderLayer::mapRectToPaintInvalidationBacking? (but suitably generalized)

In general, RenderLayer::mapRectToPaintInvalidationBacking doesn't work because
localToContainerQuad assumes that the 'ancestorLayer' is inside your containing
blocks' chain, which doesn't hold for RenderLayers.

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:2340: const bool shouldIncludeTransform =
paintsWithTransform(PaintBehaviorNormal) || (options ==
ApplyBoundsChickenEggHacks && transform());
This looks like it should be moved when needed below.

[mstensho (USE GERRIT), 2014-10-13 20:47:25]

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-relpos-clipped.html (right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div
style="will-change:transform; position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> You forgot this will-change:transform.

Sorry, what do you mean?

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html
(right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html:17:
</div>
On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> I am concerned about these tests that seems very fragile to me as both are
> relying on the multi-column to work (ie if it's broken, it's going to be
broken
> in the same way).

If multicol is THAT broken, surely we already have more basic tests to catch
that?

But if you prefer, I could make the refs not use multicol at all.

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:436: // FIXME: nested fragmentation
contexts. For now just give up if there are different pagination layers
involved.
On 2014/10/13 19:25:03, Julien Chaffraix - PST wrote:
> Let's put the bug number for that as it will make it easier to spot this
FIXME.

Done.

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:2340: const bool shouldIncludeTransform =
paintsWithTransform(PaintBehaviorNormal) || (options ==
ApplyBoundsChickenEggHacks && transform());
On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> This looks like it should be moved when needed below.

Done.

[chrishtr, 2014-10-14 17:10:20]

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:428:
rect.moveBy(offsetWithinPaginationLayer);
On 2014/10/13 19:25:03, Julien Chaffraix - PST wrote:
> On 2014/10/13 at 16:03:34, chrishtr wrote:
> > Why is this just an offset in all cases? What if there is a transform
between?
> 
> convertToLayerCoords is expected not to call across transformed layers as it
> wouldn't work (which is what you're pointing out). In practice however this
> invariant doesn't hold (I tried enforcing it a year or 2 ago and it's violated
> all the time) so I don't know how our transform code even works.

Yuck. Yes you're right. convertToLayerCoords does not understand transforms. The
sad thing is that there is code in RenderObject that does all of this, and
convertToLayerCoords appears to be duplicating it.

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:447:
rect.moveBy(-offsetFromPaginationLayerToAncestor);
On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> On 2014/10/13 at 16:03:34, chrishtr wrote:
> > Same question here. Why not use something like
> RenderLayer::mapRectToPaintInvalidationBacking? (but suitably generalized)
> 
> In general, RenderLayer::mapRectToPaintInvalidationBacking doesn't work
because
> localToContainerQuad assumes that the 'ancestorLayer' is inside your
containing
> blocks' chain, which doesn't hold for RenderLayers.

Why doesn't that hold?

[Julien - ping for review, 2014-10-14 17:52:59]

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/180001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:447:
rect.moveBy(-offsetFromPaginationLayerToAncestor);
On 2014/10/14 17:10:20, chrishtr wrote:
> On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> > On 2014/10/13 at 16:03:34, chrishtr wrote:
> > > Same question here. Why not use something like
> > RenderLayer::mapRectToPaintInvalidationBacking? (but suitably generalized)
> > 
> > In general, RenderLayer::mapRectToPaintInvalidationBacking doesn't work
> because
> > localToContainerQuad assumes that the 'ancestorLayer' is inside your
> containing
> > blocks' chain, which doesn't hold for RenderLayers.
> 
> Why doesn't that hold?

|ancestorLayer| is one of your parents but it can be a layer that is not in your
containing block chain. Here is an example:

<div style="position: relative;">
    <div style="opacity: .5;">
        <div style="position: absolute;"></div>
    </div>
</div>

The absolutely positioned layer and the half-transparent layer have a child <->
parent relationship in the RenderLayer tree but the opacity layer doesn't belong
to the absolutely positioned layer's containing block chain. If you look at
accumulateOffsetTowardsAncestor, it explicitly handles such a case (which
accounts for a lot of the complexity in the function).

[Julien - ping for review, 2014-10-14 17:53:51]

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-relpos-clipped.html (right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div
style="will-change:transform; position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
On 2014/10/13 20:47:25, mstensho wrote:
> On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> > You forgot this will-change:transform.
> 
> Sorry, what do you mean?

There was an earlier comment about not using will-change:transform but a
transform:translateZ(0) instead. Just saying that you changed the first test but
forgot the following.

[mstensho (USE GERRIT), 2014-10-14 18:11:32]

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-relpos-clipped.html (right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-relpos-clipped.html:4: <div
style="will-change:transform; position:absolute; left:0; right:0; height:13em;
background:blue;"></div>
On 2014/10/14 17:53:51, Julien Chaffraix - PST wrote:
> On 2014/10/13 20:47:25, mstensho wrote:
> > On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> > > You forgot this will-change:transform.
> > 
> > Sorry, what do you mean?
> 
> There was an earlier comment about not using will-change:transform but a
> transform:translateZ(0) instead. Just saying that you changed the first test
but
> forgot the following.

Ah, no, it was the other way around, about changing to using will-change,
instead of transform. So I changed to using will-change, except for one test,
because then it would fail. I'd rather deal with that in a separate CL, if
that's okay with you.

[Julien - ping for review, 2014-10-14 18:28:07]

(sorry for the multiple replies, I should have answered all of them at once)

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html
(right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html:17:
</div>
On 2014/10/13 20:47:25, mstensho wrote:
> On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> > I am concerned about these tests that seems very fragile to me as both are
> > relying on the multi-column to work (ie if it's broken, it's going to be
> broken
> > in the same way).
> 
> If multicol is THAT broken, surely we already have more basic tests to catch
> that?

You're making an assumption (that we have enough basic testing) but it seems
that none of us knows if that's the case, which makes me very uneasy. The
difference between the tests and their expected files are minimal meaning that
it's easy for both to be broken in the same way IMO. That's why my slight
paranoia asks for some guarantees that we don't just land tests that can mask /
ignore real failures.

[mstensho (USE GERRIT), 2014-10-14 19:32:10]

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
File LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html
(right):

https://codereview.chromium.org/645023002/diff/180001/LayoutTests/fast/multic...
LayoutTests/fast/multicol/composited-with-child-layer-in-next-column.html:17:
</div>
On 2014/10/14 18:28:07, Julien Chaffraix - PST wrote:
> On 2014/10/13 20:47:25, mstensho wrote:
> > On 2014/10/13 19:25:02, Julien Chaffraix - PST wrote:
> > > I am concerned about these tests that seems very fragile to me as both are
> > > relying on the multi-column to work (ie if it's broken, it's going to be
> > broken
> > > in the same way).
> > 
> > If multicol is THAT broken, surely we already have more basic tests to catch
> > that?
> 
> You're making an assumption (that we have enough basic testing) but it seems
> that none of us knows if that's the case, which makes me very uneasy. The
> difference between the tests and their expected files are minimal meaning that
> it's easy for both to be broken in the same way IMO. That's why my slight
> paranoia asks for some guarantees that we don't just land tests that can mask
/
> ignore real failures.

Done. Let's take no chances and avoid multicol in the refs. :)

[Julien - ping for review, 2014-10-15 14:36:41]

lgtm

https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:427: // First make the flow thread
rectangle relative to the flow thread, not to |layer|.
It seems like this comment should encompass the 3 lines (starting at the
definition of offsetWithinPaginationLayer).

https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:436: // FIXME: nested fragmentation
contexts (crbug.com/423076). For now just give up if there are
Let's make the FIXME actionable:

// FIXME: Handle nested fragmentation contexts...

[mstensho (USE GERRIT), 2014-10-15 16:45:22]

https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:427: // First make the flow thread
rectangle relative to the flow thread, not to |layer|.
On 2014/10/15 14:36:40, Julien Chaffraix - PST wrote:
> It seems like this comment should encompass the 3 lines (starting at the
> definition of offsetWithinPaginationLayer).

Done.

https://codereview.chromium.org/645023002/diff/320001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:436: // FIXME: nested fragmentation
contexts (crbug.com/423076). For now just give up if there are
On 2014/10/15 14:36:41, Julien Chaffraix - PST wrote:
> Let's make the FIXME actionable:
> 
> // FIXME: Handle nested fragmentation contexts...

Done.

[mstensho (USE GERRIT), 2014-10-15 16:45:38]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-10-15 16:46:21]

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

[commit-bot: I haz the power, 2014-10-15 17:50:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-15 17:50:24]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[mstensho (USE GERRIT), 2014-10-15 20:54:52]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-10-15 20:55:12]

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

[commit-bot: I haz the power, 2014-10-15 21:56:44]

Committed patchset #6 (id:360001) as 183766