Region based multicol: support explicit column breaks

Region based multicol: support explicit column breaks

Merely supporting insertion of explicit (forced) column breaks in
the region based multicol implementation is really simple: just
hook up with what the CSS regions code is already doing.

However, there is one complication: column balancing. In order to
balance columns as nicely as possible when there are explicit
breaks, we need to figure out between which explicit breaks the
implicit breaks will occur (if there's room for any at all).

This is a merge of https://bugs.webkit.org/show_bug.cgi?id=123993
Reviewed in WebKit by David Hyatt.

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

[eseidel, 2014-02-07 00:25:01]

Julien or Ojan are better reviewers than I, but I don't want you to feel blocked
here.  I'll plan to look again Monday if neither of them are able to get to this
today.  Sorry for the delay!

[ojan, 2014-02-08 02:26:11]

lgtm

The latest patch looks like it didn't upload correctly. I reviewed the first
patchset. lgtm assuming you're just merging to tip of tree.

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

https://codereview.chromium.org/143383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:124:
*offsetBreakAdjustment = pageLogicalHeightForOffset(offset) ?
pageRemainingLogicalHeightForOffset(offset, IncludePageBoundary) :
LayoutUnit(0);
Nit: LayoutUnit() is cheaper because it doesn't do the bound checks.

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

https://codereview.chromium.org/143383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:132: LayoutUnit startOffset =
index > 0 ? m_contentRuns[index - 1].breakOffset() : LayoutUnit(0);
Ditto

[mstensho (USE GERRIT), 2014-02-10 13:37:12]

On 2014/02/08 02:26:11, ojan wrote:
> The latest patch looks like it didn't upload correctly. I reviewed the first
> patchset. lgtm assuming you're just merging to tip of tree.

Yes, pretty much, but there were conflicts, because someone else had refactored
multipass layout a little bit in the meantime. That someone renamed the
MultiColumnBlock method relayoutForPagination() to
shouldRelayoutMultiColumnBlock(). So I resolved conflicts, but also made the
method non-const, since it really isn't const. Let me know if this is a problem.
I realized that perhaps shouldRelayoutMultiColumnBlock() is the wrong name for
something non-const.

[mstensho (USE GERRIT), 2014-02-10 13:50:25]

Review issues addressed.
@ojan, could you indicate whether you agree with my
shouldRelayoutMultiColumnBlock() changes or not?

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

https://codereview.chromium.org/143383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:124:
*offsetBreakAdjustment = pageLogicalHeightForOffset(offset) ?
pageRemainingLogicalHeightForOffset(offset, IncludePageBoundary) :
LayoutUnit(0);
On 2014/02/08 02:26:12, ojan wrote:
> Nit: LayoutUnit() is cheaper because it doesn't do the bound checks.

Done.

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

https://codereview.chromium.org/143383002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:132: LayoutUnit startOffset =
index > 0 ? m_contentRuns[index - 1].breakOffset() : LayoutUnit(0);
On 2014/02/08 02:26:12, ojan wrote:
> Ditto

Done.

[abucur, 2014-02-10 16:11:52]

Informal LGTM from me :). You may consider the removal comments like "We're
done" from the code. Those are usually just noisy.

[ojan, 2014-02-10 18:52:19]

lgtm

[commit-bot: I haz the power, 2014-02-10 18:52:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/143383002/520001

[commit-bot: I haz the power, 2014-02-10 20:20:26]

Change committed as 166859