Jump to the next outer column when an inner column is too short.

Jump to the next outer column when an inner column is too short.

If an inner multicol ends up near the bottom in a column in an outer multicol
container, we get inner columns that are shorter in the first row than in
subsequent rows. In such cases it may be necessary to break past all inner
columns in the first row, so that we push the content all the way to the next
row (and thus to the next outer column), in order to fit unbreakable content
(such as lines or unbreakable blocks).

BUG=447718
R=leviw@chromium.org

Committed: https://crrev.com/6692bea7c8d9afb6c4f23c6194ecacc3a4bddbe8
Cr-Commit-Position: refs/heads/master@{#361769}

[mstensho (USE GERRIT), 2015-11-24 14:44:18]

I'm pretty sure this isn't the smallest CL you've ever got. :)

It is possible to split out the changes for unbreakable blocks into a separate
CL (but that's just a few lines of related code, and this first CL wouldn't
shrink that much anyway). I'm talking about the changes in
adjustBlockChildForPagination() and adjustForUnsplittableChild(), and one test.
Let me know.

[mstensho (USE GERRIT), 2015-11-24 19:00:35]

The trybots found regressions. I suppose I should figure out what that's about
before you look at this.

[mstensho (USE GERRIT), 2015-11-24 20:58:36]

On 2015/11/24 19:00:35, mstensho wrote:
> The trybots found regressions. I suppose I should figure out what that's about
> before you look at this.

Need to land https://codereview.chromium.org/1471403002/ before this, and then
we should be good.

[leviw_travelin_and_unemployed, 2015-11-24 21:41:40]

On 2015/11/24 at 14:44:18, mstensho wrote:
> I'm pretty sure this isn't the smallest CL you've ever got. :)

Crap ;)

> 
> It is possible to split out the changes for unbreakable blocks into a separate
CL (but that's just a few lines of related code, and this first CL wouldn't
shrink that much anyway). I'm talking about the changes in
adjustBlockChildForPagination() and adjustForUnsplittableChild(), and one test.
Let me know.

I'll give this a fair shake before EOD PST.

I put your dependent CL in the CQ in the meantime.

[leviw_travelin_and_unemployed, 2015-11-25 03:52:38]

Well... I lied. I'll try again tomorrow morning. Don't hate me.

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

On 2015/11/25 03:52:38, leviw wrote:
> Well... I lied. I'll try again tomorrow morning. Don't hate me.

I was getting concerned about your health. You're reviewing way too many
multicol CLs these days! Good to see that you know when enough is enough. :)

[leviw_travelin_and_unemployed, 2015-11-25 19:26:34]

Whew... I made it through this. A couple comments, but I'll let you follow-up on
them on your schedule. LGTM.

FYI: I'll be out from this eve through Monday for American Thanksgiving and will
be blissfully ignoring your diligent efforts in Multicoland.

I also want to tip my hat to your awesome code documenting. It's the only thing
that gets me through some of these patches.

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp (right):

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:102:
ASSERT(pageLogicalTopForOffset(flowThreadOffset) == flowThreadOffset); // That's
all we bother to handle.
Nit: This comment doesn't help me.

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:110: if
(pageLogicalHeightForOffset(flowThreadOffset) >= contentLogicalHeight)
This makes sense, but it appears in the current code it'll never be hit?
LayoutBlock::calculatePaginationStrutToFitContent has the same check.

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:115: // look
beyond the first row here.
http://i.imgur.com/EqQOTR9.jpg

[mstensho (USE GERRIT), 2015-11-25 20:08:17]

Good work! Thanks a lot! Enjoy the turducken!

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp (right):

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:102:
ASSERT(pageLogicalTopForOffset(flowThreadOffset) == flowThreadOffset); // That's
all we bother to handle.
On 2015/11/25 19:26:34, leviw wrote:
> Nit: This comment doesn't help me.

Removed.

"|flowThreadOffset| is expected to be at the exact top of a column." says
LayoutFlowThread::nextLogicalTopForUnbreakableContent(). But that's exactly what
the assertion checks, so the comment is redundant.

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:110: if
(pageLogicalHeightForOffset(flowThreadOffset) >= contentLogicalHeight)
On 2015/11/25 19:26:34, leviw wrote:
> This makes sense, but it appears in the current code it'll never be hit?
> LayoutBlock::calculatePaginationStrutToFitContent has the same check.

Right. No need to be that robust. Let's add an assertion instead.

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:115: // look
beyond the first row here.
On 2015/11/25 19:26:34, leviw wrote:
> http://i.imgur.com/EqQOTR9.jpg

http://renholdsnytt.no/var/askmedia/storage/images/media/gloriapark/diverse/z...

[mstensho (USE GERRIT), 2015-11-25 20:08:27]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-11-25 20:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1472053002/#ps40001
(title: "code review")

[commit-bot: I haz the power, 2015-11-25 20:10:08]

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

[leviw_travelin_and_unemployed, 2015-11-25 22:00:49]

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp (right):

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:110: if
(pageLogicalHeightForOffset(flowThreadOffset) >= contentLogicalHeight)
On 2015/11/25 at 20:08:17, mstensho wrote:
> On 2015/11/25 19:26:34, leviw wrote:
> > This makes sense, but it appears in the current code it'll never be hit?
> > LayoutBlock::calculatePaginationStrutToFitContent has the same check.
> 
> Right. No need to be that robust. Let's add an assertion instead.

I was hoping you'd say that ;)

https://codereview.chromium.org/1472053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp:115: // look
beyond the first row here.
On 2015/11/25 at 20:08:17, mstensho wrote:
> On 2015/11/25 19:26:34, leviw wrote:
> > http://i.imgur.com/EqQOTR9.jpg
> 
>
http://renholdsnytt.no/var/askmedia/storage/images/media/gloriapark/diverse/z...

HA!

[commit-bot: I haz the power, 2015-11-25 22:14:09]

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

[commit-bot: I haz the power, 2015-11-25 22:14:10]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[mstensho (USE GERRIT), 2015-11-25 22:39:59]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-11-25 22:42:02]

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

[commit-bot: I haz the power, 2015-11-26 00:48:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-26 00:49:28]

Description was changed from

==========
Jump to the next outer column when an inner column is too short.

If an inner multicol ends up near the bottom in a column in an outer multicol
container, we get inner columns that are shorter in the first row than in
subsequent rows. In such cases it may be necessary to break past all inner
columns in the first row, so that we push the content all the way to the next
row (and thus to the next outer column), in order to fit unbreakable content
(such as lines or unbreakable blocks).

BUG=447718
R=leviw@chromium.org
==========

to

==========
Jump to the next outer column when an inner column is too short.

If an inner multicol ends up near the bottom in a column in an outer multicol
container, we get inner columns that are shorter in the first row than in
subsequent rows. In such cases it may be necessary to break past all inner
columns in the first row, so that we push the content all the way to the next
row (and thus to the next outer column), in order to fit unbreakable content
(such as lines or unbreakable blocks).

BUG=447718
R=leviw@chromium.org

Committed: https://crrev.com/6692bea7c8d9afb6c4f23c6194ecacc3a4bddbe8
Cr-Commit-Position: refs/heads/master@{#361769}
==========

[commit-bot: I haz the power, 2015-11-26 00:49:29]

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