[New Multicolumn] Preparatory work for nested multicol support.

[New Multicolumn] Preparatory work for nested multicol support.

Introducing the class MultiColumnRow. A RenderMultiColumnSet will have one
or more of those. For now, only one row is supported. Moved a lot of code
from RenderMultiColumnSet into MultiColumnRow.

Additionally, since it would just be just cumbersome and waseful to store the
flow thread rectangle in RenderRegion (or RenderMultiColumnSet, for that
matter) now, create a rectangle on the fly when requested.

No behavioral changes are intended.

R=jchaffraix@chromium.org
BUG=447718
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189966

[Julien - ping for review, 2015-01-29 17:37:47]

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Mu...
File Source/core/rendering/MultiColumnRow.h (right):

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Mu...
Source/core/rendering/MultiColumnRow.h:12: class MultiColumnRow {
We would need a class comment about this. I still don't like the name even (if
it comes from the specification) as it's very misleading and generic.

We talked about this with Morten and (MultiColumn)FragmentainerGroup would be
accurate and a better name.

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

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.cpp:199: for (MultiColumnRow* row =
firstColumnRow(); row; row = row->nextRow())
Could we use C++11 for-loop here? (and probably for most for-loop but I am lazy
and I won't write it again for all of them)

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnSet.h (right):

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.h:59: class RenderMultiColumnSet :
public RenderRegion {
It would be nice to know how much is left of RenderMultiColumnSet after the
change. Maybe there won't be much meat left and we could think of a more simple
design?

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.h:151: MultiColumnRow*
lastColumnRow()
Do we really want first / last? How about just some iterator instead of those
explicit columns?

[mstensho (USE GERRIT), 2015-02-02 17:57:24]

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Mu...
File Source/core/rendering/MultiColumnRow.h (right):

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Mu...
Source/core/rendering/MultiColumnRow.h:12: class MultiColumnRow {
On 2015/01/29 17:37:47, Julien Chaffraix - CET wrote:
> We would need a class comment about this. I still don't like the name even (if
> it comes from the specification) as it's very misleading and generic.
> 
> We talked about this with Morten and (MultiColumn)FragmentainerGroup would be
> accurate and a better name.

Done.

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

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.cpp:199: for (MultiColumnRow* row =
firstColumnRow(); row; row = row->nextRow())
On 2015/01/29 17:37:47, Julien Chaffraix - CET wrote:
> Could we use C++11 for-loop here? (and probably for most for-loop but I am
lazy
> and I won't write it again for all of them)

Done.

I was planning to use a home-made linked list for the fragmentainer groups (and
I was planning to do it in a follow-up CL), but I was too lazy to write the
iterator boilerplate required by the range-based for loops (and it kind of felt
silly to do it just for the sake of being able to use range-based for loops), so
I ended up using a Vector instead. Should be fine. Groups will always be
inserted at the end, and the number of groups will typically not vary a lot for
a given multicol container.

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnSet.h (right):

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.h:59: class RenderMultiColumnSet :
public RenderRegion {
On 2015/01/29 17:37:47, Julien Chaffraix - CET wrote:
> It would be nice to know how much is left of RenderMultiColumnSet after the
> change. Maybe there won't be much meat left and we could think of a more
simple
> design?

Yeah, those are interesting thoughts. I think RenderMultiColumnSet is still
useful though, for example when it comes to representing a contiguous run of
column content, but maybe we could reconsider that in the future?

Maybe we could get rid of both RenderMultiColumnSet and
RenderMultiColumnSpannerPlaceholder. But then we need to do manually what
RenderBlock layout does for us for free now (mainly positioning within the
multicol container, including margin collapsing).

https://codereview.chromium.org/883293004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnSet.h:151: MultiColumnRow*
lastColumnRow()
On 2015/01/29 17:37:47, Julien Chaffraix - CET wrote:
> Do we really want first / last? How about just some iterator instead of those
> explicit columns?

The first and the last groups are of special significance in certain parts of
the code (e.g. when finding the total flow thread portion for a column set).

Note that I've changed this code now, since groups are stored in a Vector now.
But we still have first / last.

[mstensho (USE GERRIT), 2015-02-04 08:52:32]

ping - it would be great to have this resolved as soon as possible, since it
moves a lot of code around, and thus blocks other work, and it's also very
conflict-prone, of course.

[mstensho (USE GERRIT), 2015-02-09 21:43:41]

@jchaffraix - can you take another look, please? I'd very much like to resolve
this CL before making any other changes to these files. Plus, I get quite a few
merge conflicts because of the rendering -> layout rename storm.

[Julien - ping for review, 2015-02-11 07:25:27]

jchaffraix@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Julien - ping for review, 2015-02-11 07:25:29]

lgtm, I have skimmed the patch as this is only a move.

+Dan, if they have more time, we need more people involved in rendering reviews.

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
Source/core/layout/MultiColumnFragmentainerGroup.cpp:35: // passes as well.
Maybe there is some ASSERT we could land to avoid this trap. RenderTable has a
LayoutForbiddenScope for this very reason as it's a big foot gun.

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
File Source/core/layout/MultiColumnFragmentainerGroup.h (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
Source/core/layout/MultiColumnFragmentainerGroup.h:76: unsigned
findRunWithTallestColumns() const;
I think we should use size_t for indexes (we are inconsistent but we should try
to fix this) per the Google style guide.

https://codereview.chromium.org/883293004/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.cpp (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.cpp:174: for (auto columnSet :
m_multiColumnSetList)
I assume this doesn't do a deep copy behind the scene?

[mstensho (USE GERRIT), 2015-02-11 09:49:55]

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
File Source/core/layout/MultiColumnFragmentainerGroup.cpp (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
Source/core/layout/MultiColumnFragmentainerGroup.cpp:35: // passes as well.
On 2015/02/11 07:25:29, Julien Chaffraix - AEST wrote:
> Maybe there is some ASSERT we could land to avoid this trap. RenderTable has a
> LayoutForbiddenScope for this very reason as it's a big foot gun.

This very code already makes sure that we only balance the last group.

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
File Source/core/layout/MultiColumnFragmentainerGroup.h (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
Source/core/layout/MultiColumnFragmentainerGroup.h:76: unsigned
findRunWithTallestColumns() const;
On 2015/02/11 07:25:29, Julien Chaffraix - AEST wrote:
> I think we should use size_t for indexes (we are inconsistent but we should
try
> to fix this) per the Google style guide.

This is just code that got moved. Let's fix this in a separate CL. There's quite
some usage of unsigned in this code, to specify column number, column count,
content runs, and maybe more. Better clean up everything while we're at it.

https://codereview.chromium.org/883293004/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.cpp (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.cpp:174: for (auto columnSet :
m_multiColumnSetList)
On 2015/02/11 07:25:29, Julien Chaffraix - AEST wrote:
> I assume this doesn't do a deep copy behind the scene?

Correct. No copying.

[mstensho (USE GERRIT), 2015-02-11 09:50:07]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-02-11 09:50:58]

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

[commit-bot: I haz the power, 2015-02-11 11:23:05]

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

[commit-bot: I haz the power, 2015-02-11 11:23:06]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12...)
  win_gpu_triggered_tests on tryserver.chromium.gpu (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[mstensho (USE GERRIT), 2015-02-11 11:25:16]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-02-11 11:26:24]

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

[commit-bot: I haz the power, 2015-02-11 12:16:29]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189966

[Nico, 2015-04-09 04:39:33]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-04-09 04:39:34]

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
File Source/core/layout/MultiColumnFragmentainerGroup.h (right):

https://codereview.chromium.org/883293004/diff/140001/Source/core/layout/Mult...
Source/core/layout/MultiColumnFragmentainerGroup.h:139: class
MultiColumnFragmentainerGroupList : public Vector<MultiColumnFragmentainerGroup,
1> {
Let's not derive from container types. If your class is like a container, give
it a container member and delegate the methods you need to that field. You
probably don't need all the methods.

jchaffraix: This was more than just a move from what I can tell (this code
wasn't there before), please take a slightly more careful look next time.