Don't lose the pagination strut when a line is re-created.

Don't lose the pagination strut when a line is re-created.

If there's not enough room for a line in a column, so that it gets pushed to
the next one, and there's a float at the top of the next column, the available
line width changes, and we need to re-create the line at the new position. Do
not lose the pagination strut in the process, or the column balancer might
over-stretch the columns, since it might fail to find the lowest possible space
shortage for the next layout pass. So store the strut so that we can re-apply
it when the new line has been created.

Since we now store the strut of such lines while they're being re-created, this
can be used as a flag to skip positioning of out-of-flow objects and floats. In
other words, we can retire the logicalWidthIsAvailable flag (which was a really
confusing name anyway).

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

[mstensho (USE GERRIT), 2016-04-21 16:31:28]

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

[eae, 2016-04-21 16:44:21]

LGTM w/suggestions

https://codereview.chromium.org/1905923002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/multicol/pushed-line-affected-by-float.html
(right):

https://codereview.chromium.org/1905923002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/multicol/pushed-line-affected-by-float.html:8:
<div data-expected-height="40" id="multicol">
This might be better as a reftest as it currently only asserts the height of the
container and y position of the child.

[mstensho (USE GERRIT), 2016-04-21 18:56:59]

https://codereview.chromium.org/1905923002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/multicol/pushed-line-affected-by-float.html
(right):

https://codereview.chromium.org/1905923002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/multicol/pushed-line-affected-by-float.html:8:
<div data-expected-height="40" id="multicol">
On 2016/04/21 16:44:21, eae wrote:
> This might be better as a reftest as it currently only asserts the height of
the
> container and y position of the child.

Yeah, that's the weakness about check-layout tests. You can add as many
assertions as you like, but there may still be unforeseen ways of breaking the
layout that will only show up on a reftest.

In this case I wanted to make sure that the balancer ended up with a 40px tall
multicol container, so that the float would end up at the exact top of the
second column. The requirement about being right-aligned as well was just to
make the test human-friendly (since "at the top of the second column" is a bit
harder to observe than "top-right corner").

Anyway, I tend to like reftests, although they are slower. So, since that's what
you suggest, I'll just do that. :)

Done.

[mstensho (USE GERRIT), 2016-04-21 18:57:09]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-04-21 18:57:10]

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/1905923002/#ps40001
(title: "Make a reftest instead.")

[commit-bot: I haz the power, 2016-04-21 18:57:33]

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

[wkorman, 2016-04-21 19:21:14]

lgtm

No need to stop CQ but if it doesn't go straight in and there's a BUG= worth
adding before reattempt. I realize this may just be refinement/improvement
without a specific bug.

[mstensho (USE GERRIT), 2016-04-21 19:24:26]

On 2016/04/21 19:21:14, wkorman wrote:
> lgtm
> 
> No need to stop CQ but if it doesn't go straight in and there's a BUG= worth
> adding before reattempt. I realize this may just be refinement/improvement
> without a specific bug.

I'm not aware of any bug report for this.

I usually don't bother to spam the BTS with bug reports for things that I
discover and fix right away, but I can change my policy on that if you like. :)

[wkorman, 2016-04-21 19:28:51]

On 2016/04/21 at 19:24:26, mstensho wrote:
> On 2016/04/21 19:21:14, wkorman wrote:
> > lgtm
> > 
> > No need to stop CQ but if it doesn't go straight in and there's a BUG= worth
> > adding before reattempt. I realize this may just be refinement/improvement
> > without a specific bug.
> 
> I'm not aware of any bug report for this.
> 
> I usually don't bother to spam the BTS with bug reports for things that I
discover and fix right away, but I can change my policy on that if you like. :)

No need, I agree, don't need to create bug just to submit and close again...

[commit-bot: I haz the power, 2016-04-21 20:37:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:38:54]

Description was changed from

==========
Don't lose the pagination strut when a line is re-created.

If there's not enough room for a line in a column, so that it gets pushed to
the next one, and there's a float at the top of the next column, the available
line width changes, and we need to re-create the line at the new position. Do
not lose the pagination strut in the process, or the column balancer might
over-stretch the columns, since it might fail to find the lowest possible space
shortage for the next layout pass. So store the strut so that we can re-apply
it when the new line has been created.

Since we now store the strut of such lines while they're being re-created, this
can be used as a flag to skip positioning of out-of-flow objects and floats. In
other words, we can retire the logicalWidthIsAvailable flag (which was a really
confusing name anyway).
==========

to

==========
Don't lose the pagination strut when a line is re-created.

If there's not enough room for a line in a column, so that it gets pushed to
the next one, and there's a float at the top of the next column, the available
line width changes, and we need to re-create the line at the new position. Do
not lose the pagination strut in the process, or the column balancer might
over-stretch the columns, since it might fail to find the lowest possible space
shortage for the next layout pass. So store the strut so that we can re-apply
it when the new line has been created.

Since we now store the strut of such lines while they're being re-created, this
can be used as a flag to skip positioning of out-of-flow objects and floats. In
other words, we can retire the logicalWidthIsAvailable flag (which was a really
confusing name anyway).

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

[commit-bot: I haz the power, 2016-04-22 19:38:55]

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