InitialColumnHeightFinder needs to take all expected rows into account.

InitialColumnHeightFinder needs to take all expected rows into account.

When a balanced multicol is nested inside another balanced multicol, it will
not be able to create any fragmentainer groups in the first layout pass, since
the height of the outer columns is still unknown.

We need to detect this situation, so that we don't limit the number of content
runs (content portions without explicit breaks) to the used value of
column-count. We are going to need ALL content runs, and group them into
imaginary rows, to figure out a minimal height of the entire inner multicol
container in the first balancing pass.

This will help set a better initial outer column height, and, more importantly,
set some sensible height on the inner multicol container right away, so that
we're not going to believe that it's super-short, which might prevent us from
marking it for re-layout when the outer coulmns have been sized.
childNeedsRelayoutForPagination() would simply fail to see that it's actually
going to cross outer column boundaries, and just bail.

We also treat tallestUnbreakableLogicalHeight() somewhat differently in such
situations. We require that the last "row" alone (rather than the entire
multicol container) be at least as tall as this.

Broke a newFragmentainerGroupsAllowed() out of
appendNewFragmentainerGroupIfNeeded(), since the column balancer code now also
needs to know when we're nested but are not allowed to create fragmentainer
groups.

Some, but not all, new tests used to fail before the code changes in this CL.
The passing ones are there to point out regressions that I nearly introduced
while working on this CL.

This is a patch in preparation for removing the relayoutChildren = true thing
in LayoutBlockFlow::layoutBlockFlow() when page logical height changes.

Committed: https://crrev.com/d19d51259c048132deb253a9d7441b40eceb55e9
Cr-Commit-Position: refs/heads/master@{#431844}

[mstensho (USE GERRIT), 2016-11-10 21:01:46]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 21:02:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mstensho (USE GERRIT), 2016-11-10 21:06:51]

mstensho@opera.com changed reviewers:
+ eae@chromium.org, szager@chromium.org

[mstensho (USE GERRIT), 2016-11-10 21:06:51]

You can tell from the long description and all the long comments in this CL that
this is occult rocket surgery. :(

The usage of balanced multicol inside balanced multicol is probably very
limited, but we do have such a test that would start to fail if we remove the
relayoutChildren if pageLogicalHeightChanged mechanism in
LayoutBlockFlow::layoutBlockFlow() - which I intend to get rid of as part of
https://codereview.chromium.org/2471623003/

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/multicol/nested-balanced-inner-with-many-breaks.html
(right):

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/multicol/nested-balanced-inner-with-many-breaks.html:5:
<div>
The extra DIV here (and in the other tests) is needed to trick the
relayoutChildren if pageLogicalHeightChanged in
LayoutBlockFlow::layoutBlockFlow().

[commit-bot: I haz the power, 2016-11-10 22:38:15]

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

[commit-bot: I haz the power, 2016-11-10 22:38:16]

Dry run: This issue passed the CQ dry run.

[eae, 2016-11-10 23:37:12]

LGTM w/questions

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ColumnBalancer.cpp (right):

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.cpp:159: rowLogicalTop +=
LayoutUnit::fromFloatCeil(rowHeight);
why ceil?

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ColumnBalancer.h (right):

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:166: unsigned
lastRunIndex = m_contentRuns.size() - 1;
Is columnCount  guaranteed to be at least one? If not please add a check/DCHECK.

[mstensho (USE GERRIT), 2016-11-11 08:47:46]

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ColumnBalancer.cpp (right):

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.cpp:159: rowLogicalTop +=
LayoutUnit::fromFloatCeil(rowHeight);
On 2016/11/10 23:37:12, eae wrote:
> why ceil?

That's what InitialColumnHeightFinder::ContentRun::columnLogicalHeight() does.
We want to make sure that the LayoutUnit value is at least as large as that of
the float.

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ColumnBalancer.h (right):

https://codereview.chromium.org/2493833004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:166: unsigned
lastRunIndex = m_contentRuns.size() - 1;
On 2016/11/10 23:37:12, eae wrote:
> Is columnCount  guaranteed to be at least one? If not please add a
check/DCHECK.

There should be no way for it to be 0, or we'd be in trouble all over the place.
Still, we actually had an assertion for this in
InitialColumnHeightFinder::recordStrutBeforeOffset(). I'll move it to
ColumnBalancer() instead, so that we always check it.

Done.

[mstensho (USE GERRIT), 2016-11-14 07:31:30]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-11-14 07:31:30]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2493833004/#ps20001
(title: "Code review")

[commit-bot: I haz the power, 2016-11-14 07:31:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 08:52:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-14 08:53:52]

Description was changed from

==========
InitialColumnHeightFinder needs to take all expected rows into account.

When a balanced multicol is nested inside another balanced multicol, it will
not be able to create any fragmentainer groups in the first layout pass, since
the height of the outer columns is still unknown.

We need to detect this situation, so that we don't limit the number of content
runs (content portions without explicit breaks) to the used value of
column-count. We are going to need ALL content runs, and group them into
imaginary rows, to figure out a minimal height of the entire inner multicol
container in the first balancing pass.

This will help set a better initial outer column height, and, more importantly,
set some sensible height on the inner multicol container right away, so that
we're not going to believe that it's super-short, which might prevent us from
marking it for re-layout when the outer coulmns have been sized.
childNeedsRelayoutForPagination() would simply fail to see that it's actually
going to cross outer column boundaries, and just bail.

We also treat tallestUnbreakableLogicalHeight() somewhat differently in such
situations. We require that the last "row" alone (rather than the entire
multicol container) be at least as tall as this.

Broke a newFragmentainerGroupsAllowed() out of
appendNewFragmentainerGroupIfNeeded(), since the column balancer code now also
needs to know when we're nested but are not allowed to create fragmentainer
groups.

Some, but not all, new tests used to fail before the code changes in this CL.
The passing ones are there to point out regressions that I nearly introduced
while working on this CL.

This is a patch in preparation for removing the relayoutChildren = true thing
in LayoutBlockFlow::layoutBlockFlow() when page logical height changes.
==========

to

==========
InitialColumnHeightFinder needs to take all expected rows into account.

When a balanced multicol is nested inside another balanced multicol, it will
not be able to create any fragmentainer groups in the first layout pass, since
the height of the outer columns is still unknown.

We need to detect this situation, so that we don't limit the number of content
runs (content portions without explicit breaks) to the used value of
column-count. We are going to need ALL content runs, and group them into
imaginary rows, to figure out a minimal height of the entire inner multicol
container in the first balancing pass.

This will help set a better initial outer column height, and, more importantly,
set some sensible height on the inner multicol container right away, so that
we're not going to believe that it's super-short, which might prevent us from
marking it for re-layout when the outer coulmns have been sized.
childNeedsRelayoutForPagination() would simply fail to see that it's actually
going to cross outer column boundaries, and just bail.

We also treat tallestUnbreakableLogicalHeight() somewhat differently in such
situations. We require that the last "row" alone (rather than the entire
multicol container) be at least as tall as this.

Broke a newFragmentainerGroupsAllowed() out of
appendNewFragmentainerGroupIfNeeded(), since the column balancer code now also
needs to know when we're nested but are not allowed to create fragmentainer
groups.

Some, but not all, new tests used to fail before the code changes in this CL.
The passing ones are there to point out regressions that I nearly introduced
while working on this CL.

This is a patch in preparation for removing the relayoutChildren = true thing
in LayoutBlockFlow::layoutBlockFlow() when page logical height changes.

Committed: https://crrev.com/d19d51259c048132deb253a9d7441b40eceb55e9
Cr-Commit-Position: refs/heads/master@{#431844}
==========

[commit-bot: I haz the power, 2016-11-14 08:53:53]

Patchset 2 (id:??) landed as
https://crrev.com/d19d51259c048132deb253a9d7441b40eceb55e9
Cr-Commit-Position: refs/heads/master@{#431844}