Column balancing refactoring. Don't propagate data during layout.

Column balancing refactoring. Don't propagate data during layout.

The column balancing machinery needs the following to calculate an optimal
column height:

1. Flow thread height
2. Flow thread logical top position of all forced breaks
3. The total number of forced breaks
4. Minimum space shortage at any soft break

The first two are needed to estimate an initial column height. The last two are
needed if the initial column height estimate wasn't good enough and we need to
stretch it.

We used to propagate this information to the flow thread continuously during
layout. Then the flow thread, after layout, would use this information for
column balancing (either estimate an initial column height, or stretch columns
based on minimum space shortage) and then re-lay out. This was problematic,
because during layout, we sometimes go back, and re-lay out e.g. a block
because the initial logical top was wrong (due to complex margin collapsing).
This would confuse the column balancing machinery (forced breaks being reported
and counted twice, space shortage being incorrectly reported, and so on).

Instead, traverse the tree after flow thread layout, to collect the information
that we need. This fixes an existing corner-case bug, allows for future
optimizations.

There is still one thing that the flow thread needs to be notified about during
layout, if we have nested multicol: It needs to know where content is laid out
(especially if the content is in a new column), in order to tell if we need to
insert another fragmentainer group (i.e. column row). It would be nice to get
rid of this as well in the future, but we need it for now. Renamed the poorly
named setPageBreak() to the more call-back-like paginatedContentWasLaidOut().

The new class ColumnBalancer and its subclasses are now in charge of column
balancing. Moved / refactored relevant code into those classes.
Taken from LayoutMultiColumnFlowThread, LayoutMultiColumnSet,
MultiColumnFramentainerGroup, and even LayoutBlockFlow.

BUG=521964
R=jchaffraix@chromium.org,leviw@chromium.org

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

[leviw_travelin_and_unemployed, 2015-10-08 09:49:29]

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

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:14: ColumnBalancer(const
MultiColumnFragmentainerGroup&);
Does any of this need to be public? It seems like the two public methods on
MinimumSpaceShortageFinder are the only ones intended to be used outside the
sub-classes.

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:76: void
examineBoxAfterEntering(const LayoutBox&);
mark these as overrides

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:120: Vector<ContentRun,
50> m_contentRuns;
Where did you get this inline size from?

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:135: void
examineBoxAfterEntering(const LayoutBox&);
mark these as overrides

[leviw_travelin_and_unemployed, 2015-10-08 09:53:47]

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

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:75: enum
ColumnIndexCalculationMode {
Is this change related? It doesn't appear that this is being called publicly all
of a sudden.

[mstensho (USE GERRIT), 2015-10-08 10:38:05]

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

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:14: ColumnBalancer(const
MultiColumnFragmentainerGroup&);
On 2015/10/08 09:49:29, leviw wrote:
> Does any of this need to be public? It seems like the two public methods on
> MinimumSpaceShortageFinder are the only ones intended to be used outside the
> sub-classes.

Done.

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:76: void
examineBoxAfterEntering(const LayoutBox&);
On 2015/10/08 09:49:29, leviw wrote:
> mark these as overrides

The entire class is final. Should I still mark them?

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:120: Vector<ContentRun,
50> m_contentRuns;
On 2015/10/08 09:49:29, leviw wrote:
> Where did you get this inline size from?

I just typed something on my keyboard until it compiled. :)

50 is a lot, more than enough for most layouts. Not sure what to put here, but
since these are temporary objects, I was thinking that doing one huge-ish
allocation is better than reallocating every time we run out of space.

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ColumnBalancer.h:135: void
examineBoxAfterEntering(const LayoutBox&);
On 2015/10/08 09:49:29, leviw wrote:
> mark these as overrides

See previous comment.

[leviw_travelin_and_unemployed, 2015-10-08 10:44:39]

On 2015/10/08 at 10:38:05, mstensho wrote:
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/ColumnBalancer.h (right):
> 
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ColumnBalancer.h:14:
ColumnBalancer(const MultiColumnFragmentainerGroup&);
> On 2015/10/08 09:49:29, leviw wrote:
> > Does any of this need to be public? It seems like the two public methods on
> > MinimumSpaceShortageFinder are the only ones intended to be used outside the
> > sub-classes.
> 
> Done.

Woohoo! You win a "super weird class in Blink" award.

> 
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ColumnBalancer.h:76: void
examineBoxAfterEntering(const LayoutBox&);
> On 2015/10/08 09:49:29, leviw wrote:
> > mark these as overrides
> 
> The entire class is final. Should I still mark them?

Nevermind.

> 
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ColumnBalancer.h:120: Vector<ContentRun,
50> m_contentRuns;
> On 2015/10/08 09:49:29, leviw wrote:
> > Where did you get this inline size from?
> 
> I just typed something on my keyboard until it compiled. :)
> 
> 50 is a lot, more than enough for most layouts. Not sure what to put here, but
since these are temporary objects, I was thinking that doing one huge-ish
allocation is better than reallocating every time we run out of space.

We discussed offline. I'm voting 32 :)

> 
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/ColumnBalancer.h:135: void
examineBoxAfterEntering(const LayoutBox&);
> On 2015/10/08 09:49:29, leviw wrote:
> > mark these as overrides
> 
> See previous comment.

[mstensho (USE GERRIT), 2015-10-08 10:49:41]

Changing the initial vector size from 0x32 to 32.

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

https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:75: enum
ColumnIndexCalculationMode {
On 2015/10/08 09:53:47, leviw wrote:
> Is this change related? It doesn't appear that this is being called publicly
all
> of a sudden.

Yeah, no need for this change yet. Reverted.

[leviw_travelin_and_unemployed, 2015-10-08 10:51:37]

On 2015/10/08 at 10:49:41, mstensho wrote:
> Changing the initial vector size from 0x32 to 32.

You got me good on that one ;)

> 
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h
(right):
> 
>
https://codereview.chromium.org/1399493002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h:75: enum
ColumnIndexCalculationMode {
> On 2015/10/08 09:53:47, leviw wrote:
> > Is this change related? It doesn't appear that this is being called publicly
all
> > of a sudden.
> 
> Yeah, no need for this change yet. Reverted.

Sweet!

I'd love for Julien to take a look if he will, since this patch has a lot going
on, but I've combed over it a couple times and it seems like a real progression
in ease-of-understanding as well as fixing this bug. Awesome comments as well.
LGTM.

[mstensho (USE GERRIT), 2015-10-09 07:42:37]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-10-09 07:42:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399493002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399493002/40001

[commit-bot: I haz the power, 2015-10-09 09:10:03]

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

[commit-bot: I haz the power, 2015-10-09 09:10:04]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mstensho (USE GERRIT), 2015-10-09 13:08:03]

That's a real failure.

Need to land https://codereview.chromium.org/1395753004/ before landing this.

[mstensho (USE GERRIT), 2015-10-10 08:18:58]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-10-10 08:19:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399493002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399493002/40001

[commit-bot: I haz the power, 2015-10-10 10:00:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-10 10:01:46]

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