[New Multicolumn] Minimal support for nested multicol in RenderLayer.

[New Multicolumn] Minimal support for nested multicol in RenderLayer.

Getting proper support for nested multicol layout (or multicol inside paged) is
going to require a lot of work (need to support multiple column rows), but in
the meantime we should at least be able to paint what we have laid out. Layout
already works fine as long as the inner multicol lives in only one outer
column. Now painting works too.

R=chrishtr@chromium.org
BUG=423076
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188169

[chrishtr, 2014-12-19 19:42:27]

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

https://codereview.chromium.org/816683002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:467: if
(ancestorLayer->enclosingPaginationLayer() != paginationLayer) {
Why do you need the recursion both here and in visualOffsetFromAncestor?

[mstensho (USE GERRIT), 2015-01-06 21:35:10]

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

https://codereview.chromium.org/816683002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:467: if
(ancestorLayer->enclosingPaginationLayer() != paginationLayer) {
On 2014/12/19 19:42:27, chrishtr wrote:
> Why do you need the recursion both here and in visualOffsetFromAncestor?

Ideally, we should break the rectangle into a bounding box for the columns
involved, for each relevant fragmentation context.

If you have this:

<div style="-webkit-columns:2;">
    outer<br>
    outer<br>
    <div style="-webkit-columns:2;">
        inner1<br>
        inner2<br>
        inner3<br>
        inner4<br>
    </div>
    outer
    outer
</div>

... which should give something like this in a correct multicol implementation
(not yet in Blink, though):

+--------------------------------------+
| outer             | inner3  | inner4 |
| outer             | outer            |
| inner1  | inner2  | outer            |
+--------------------------------------+

... you need to break the bounding box of the inner flow thread twice to get the
correct visual rectangle. First you need to break it for the inner multicol, so
you get something that can hold this:

inner1  | inner2
inner3  | inner4

Then you need to break it again for the outer multicol, so you get this:

                  | inner3  | inner4
                  |
inner1  | inner2  |

However, Blink still doesn't handle situations where an inner multicol lives in
multiple outer columns (like in this example), so, you have a good point, I'd
better use visualOffsetFromAncestor() instead here, to correctly reflect the
broken rendering that we actually get:

+--------------------------------------+
| outer             |                  |
| outer             | outer            |
| inner1  | inner3  | outer            |
+-inner2  | inner4  -------------------+

Done.

[chrishtr, 2015-01-07 01:06:08]

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

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:438: // Convert a bounding box from flow
thread coordinates, relative to |layer|, to visual coordinates, relative to
|ancestorLayer|.
What's the difference between flow thread coordinates and visual coordinates
again? The "relative to.." clause seems to make them the same given the right
context?

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:439: static void
convertFromFlowThreadToVisualBoundingBoxInAncestor(const RenderLayer* layer,
const RenderLayer* ancestorLayer, LayoutRect& rect)
I still don't quite get why you have two functions here
(convertFromFlowThreadToVisualBoundingBoxInAncestor and
visualOffsetFromAncestor) that are doing such similar things.
It's also still kind of of hard to understand.

What you're doing here seems conceptually straightforward (hopefully I got it
right):
1. Convert from local coords of |layer| to enclosing pagination coords (i.e.
"visual coords")
2. Ditto for |ancestorLayer|
3. Walk up the pagination containment hierarchy from |layer| until you find the
one for |ancestorLayer|, adjusting coords as you go.
4. Subtract the value computed in step 2 from the output of step 3

Does something like the code below work?

// Converts |rect| from |layer|'s coords to the visual coords of |ancestor|.
convertFromFromLayerCoordsToVisualCoords(RenderLayer& layer, RenderLayer&
ancestorLayer, LayoutRect rect)
{
   RenderLayer* layerPaginationContainer = layer.enclosingPaginationLayer();
   ASSERT(layerPaginationContainer);
   RenderLayer* ancestorLayerPaginationContainer =
ancestorLayer.enclosingPaginationLayer();

   LayoutPoint offsetWithinPaginationLayer;
   layer->convertToLayerCoords(ancestorLayerPaginationContainer,
offsetWithinPaginationLayer);
   rect.moveBy(offsetWithinPaginationLayer);

  
rect.moveBy(-ancestorLayer->visualOffsetFromAncestor(ancestorLayerPaginationContainer));
}

}

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1454: if (ancestorLayer == this)
Why is this special case needed?

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1458: paginationLayer =
parent()->enclosingPaginationLayer();
Only two levels of nesting are supported?

[mstensho (USE GERRIT), 2015-01-07 09:41:47]

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

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:438: // Convert a bounding box from flow
thread coordinates, relative to |layer|, to visual coordinates, relative to
|ancestorLayer|.
On 2015/01/07 01:06:08, chrishtr wrote:
> What's the difference between flow thread coordinates and visual coordinates
> again? The "relative to.." clause seems to make them the same given the right
> context?

<div style="-webkit-columns:3; -webkit-column-rule:solid;
-webkit-column-gap:20px; line-height:20px; width:220px;">
    line1<br>
    line2<br>
    line3<br>
    line4<br>
    line5<br>
    <div id="elm">
        LINE6<br>
        LINE7<br>
    </div>
    line8<br>
    line9<br>
</div>

Column width will be 60px (220px minus two gaps (2 * 20px), then divided by the
number of columns (3)).

Column height will also be 60px (three lines - 3 * 20px).

It will be rendered like this, visually:

+---------------------------+
|line1   | line4   | LINE7  |
|line2   | line5   | line8  |
|line3   | LINE6   | line9  |
+---------------------------+

But the flow thread is laid out like an imaginary tall single column (i.e. with
a width of 60px), like this:

+-------+
|line1  |
|line2  |
|line3  |
|line4  |
|line5  |
|LINE6  |
|LINE7  |
|line8  |
|line9  |
+-------+

The bounding box of "elm" (the one with LINE6 and LINE7) in flow thread
coordinates, relatively to the multicol container, will be (0, 100px, 60px,
40px). A nice rectangle. But it gets more complicated when we want the visual
bounding box for this, because "elm" lives in two columns. There's one portion
at the bottom in the second column, at (80px, 40px, 60px, 20px) (in visual
coordinates, relatively to the multicol container), and one portion at the top
in the third column, at (160px, 0, 60px, 20px) (in visual coordinates,
relatively to the multicol container). The visual bounding box is the union of
these two portions - i.e. (80px, 0, 140px, 60px).

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:439: static void
convertFromFlowThreadToVisualBoundingBoxInAncestor(const RenderLayer* layer,
const RenderLayer* ancestorLayer, LayoutRect& rect)
On 2015/01/07 01:06:08, chrishtr wrote:
> I still don't quite get why you have two functions here
> (convertFromFlowThreadToVisualBoundingBoxInAncestor and
> visualOffsetFromAncestor) that are doing such similar things.

I may have been able to explain why this is necessary in the previous comment.
:) A flow thread rectangle needs to be sliced into multiple columns in order to
become visual. This is not necessary when you just need the offset (a point). If
it's just a point, you just need to locate the correct column and adjust
accordingly.

> It's also still kind of of hard to understand.
> 
> What you're doing here seems conceptually straightforward (hopefully I got it
> right):
> 1. Convert from local coords of |layer| to enclosing pagination coords (i.e.
> "visual coords")

Yes. RenderFlowTread::fragmentsBoundingBox() expects flow thread ("imaginary
tall single column") coordinates, relatively to the flow thread.

> 2. Ditto for |ancestorLayer|

Not quite. If |ancestorLayer| happens to be within the same pagination context
as |layer|, you need to know |ancestorLayer|'s _visual_ whereabouts in the
pagination context.

> 3. Walk up the pagination containment hierarchy from |layer| until you find
the
> one for |ancestorLayer|, adjusting coords as you go.

Yes, this is what you do when |ancestorLayer| isn't inside the same pagination
context as |layer| ...

> 4. Subtract the value computed in step 2 from the output of step 3

... and this is what you do when |ancestorLayer| IS inside the same pagination
context as |layer|.

> Does something like the code below work?
> 
> // Converts |rect| from |layer|'s coords to the visual coords of |ancestor|.
> convertFromFromLayerCoordsToVisualCoords(RenderLayer& layer, RenderLayer&
> ancestorLayer, LayoutRect rect)
> {
>    RenderLayer* layerPaginationContainer = layer.enclosingPaginationLayer();
>    ASSERT(layerPaginationContainer);
>    RenderLayer* ancestorLayerPaginationContainer =
> ancestorLayer.enclosingPaginationLayer();
> 
>    LayoutPoint offsetWithinPaginationLayer;
>    layer->convertToLayerCoords(ancestorLayerPaginationContainer,
> offsetWithinPaginationLayer);
>    rect.moveBy(offsetWithinPaginationLayer);
> 
>   
>
rect.moveBy(-ancestorLayer->visualOffsetFromAncestor(ancestorLayerPaginationContainer));
> }
> 
> }

No. The clue here is to call RenderFlowThread::fragmentsBoundingBox() to slice
the flow thread bounding box into multiple columns.

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1454: if (ancestorLayer == this)
On 2015/01/07 01:06:08, chrishtr wrote:
> Why is this special case needed?

In case someone tries to find a layer's offset relative to itself.

Similar stuff in RenderLayer::convertToLayerCoords(). Asserting instead would
cause a lot of failures. Here, for example:

#0  0x0000000002e3a532 in blink::RenderLayer::visualOffsetFromAncestor
(this=0x1330d80103d0, ancestorLayer=0x1330d80103d0) at
../../third_party/WebKit/Source/core/rendering/RenderLayer.cpp:1454
#1  0x0000000002e3f226 in
blink::convertFromFlowThreadToVisualBoundingBoxInAncestor (layer=0x1330d80103d0,
ancestorLayer=0x1330d80103d0, rect=...) at
../../third_party/WebKit/Source/core/rendering/RenderLayer.cpp:462
#2  0x0000000002e396e8 in blink::RenderLayer::fragmentsBoundingBox
(this=0x1330d80103d0, ancestorLayer=0x1330d80103d0) at
../../third_party/WebKit/Source/core/rendering/RenderLayer.cpp:2324
#3  0x0000000002e3f2a2 in
blink::RenderLayer::boundingBoxForCompositingOverlapTest (this=0x1330d80103d0)
at ../../third_party/WebKit/Source/core/rendering/RenderLayer.cpp:2330
#4  0x0000000002f60608 in blink::CompositingInputsUpdater::updateRecursive
(this=0x7fffdc21fb98, layer=0x1330d80103d0,
updateType=blink::CompositingInputsUpdater::ForceUpdate, info=...) at
../../third_party/WebKit/Source/core/rendering/compositing/CompositingInputsUpdater.cpp:112

boundingBoxForCompositingOverlapTest() calls fragmentsBoundingBox(this) (i.e. it
wants the bounding box relatively to itself), and
convertFromFlowThreadToVisualBoundingBoxInAncestor() doesn't bother to check if
|layer| and |ancestorLayer| are the same (and I don't think it should need to,
either).

https://codereview.chromium.org/816683002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1458: paginationLayer =
parent()->enclosingPaginationLayer();
On 2015/01/07 01:06:08, chrishtr wrote:
> Only two levels of nesting are supported?

No, this method is called recursively. :)

[chrishtr, 2015-01-08 20:14:35]

Code looks good, now that I understand the coordinate systems. :) Please do
write up your very good description of the difference between the coordinate
systems, put it on chromium.org, and link to it from RenderLayer.h or similar.

[mstensho (USE GERRIT), 2015-01-08 20:27:45]

On 2015/01/08 20:14:35, chrishtr wrote:
> Code looks good, now that I understand the coordinate systems. :) Please do
> write up your very good description of the difference between the coordinate
> systems, put it on http://chromium.org, and link to it from RenderLayer.h or
similar.

I can do that. I also held a multicol break-out session at BlinkOn2 in Zürich. I
can probably use some material from that presentation as well.

This is going to take some time (I certainly won't be able to make it tonight),
but in the meantime, could you tell me exactly how and where to put stuff on
chromium.org?

[rune, 2015-01-08 20:56:12]

On 2015/01/08 at 20:27:45, mstensho wrote:

> This is going to take some time (I certainly won't be able to make it
tonight), but in the meantime, could you tell me exactly how and where to put
stuff on chromium.org?

If you sign in with your opera.com account on chromium.org, you can edit the
page in google docs (pencil icon at the top of the page). That's how I linked to
the style invalidation document from
https://sites.google.com/a/chromium.org/dev/developers/design-documents. Looks
like you can create pages as well as editing them.

[mstensho (USE GERRIT), 2015-01-09 12:49:30]

On 2015/01/08 20:14:35, chrishtr wrote:
> Code looks good, now that I understand the coordinate systems. :) Please do
> write up your very good description of the difference between the coordinate
> systems, put it on http://chromium.org, and link to it from RenderLayer.h or
similar.

Done.

Thanks for the instructions, Rune.

[chrishtr, 2015-01-09 17:24:24]

lgtm

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

https://codereview.chromium.org/816683002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:438: // Convert a bounding box from flow
thread coordinates, relative to |layer|, to visual coordinates, relative to
|ancestorLayer|.
Add a link to the design doc page here.

https://codereview.chromium.org/816683002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/816683002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.h:218: LayoutPoint
visualOffsetFromAncestor(const RenderLayer* ancestorLayer) const;
Add a link here to the design doc page.

Also please add a paragraph at the top defining the terms Flow Thread
Coordinates and Visual Coordinates. And also ping hartmanng@chromium.org to
coordinate with adding these coordinate systems to the new coordinate type
system scheme he's working on.

[mstensho (USE GERRIT), 2015-01-09 18:26:41]

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

https://codereview.chromium.org/816683002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:438: // Convert a bounding box from flow
thread coordinates, relative to |layer|, to visual coordinates, relative to
|ancestorLayer|.
On 2015/01/09 17:24:23, chrishtr wrote:
> Add a link to the design doc page here.

Done.

https://codereview.chromium.org/816683002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/816683002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.h:218: LayoutPoint
visualOffsetFromAncestor(const RenderLayer* ancestorLayer) const;
On 2015/01/09 17:24:23, chrishtr wrote:
> Add a link here to the design doc page.
> 
> Also please add a paragraph at the top defining the terms Flow Thread
> Coordinates and Visual Coordinates. And also ping
mailto:hartmanng@chromium.org to
> coordinate with adding these coordinate systems to the new coordinate type
> system scheme he's working on.

Done, done and done.

[mstensho (USE GERRIT), 2015-01-09 18:26:48]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-09 18:27:23]

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

[commit-bot: I haz the power, 2015-01-09 19:44:05]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188169