Improve how the column balancer handles top margins on floats.

Improve how the column balancer handles top margins on floats.

Float margins do not collapse with column boundaries, so we should make room
for them after the break, if the border box of the float starts in the next
column.

Let the balancer work on the margin box of the float (and the border box for
all other objects). For floats, we want to insert breaks before the
margin-before edge, not the border-before edge. This lets us remove
some special-code for unbreakable floats in InitialColumnHeightFinder, which
was the only place that previously bothered about this.

Changed how we determine which objects to process. We used to include the
overflow both before and after the border box, but we really don't have to
bother with content preceding it, since that shouldn't undergo fragmentation
anyway.

Discovered (one test regressed) that logicalHeightIncludingOverflow() also
included clipped overflow, which certainly wasn't the intention. This didn't
make much of a difference as long as the method was only called to check if we
could skip re-layout. But now we also use it to determine the column height.
Fixed it to only include visible overflow and renamed it to
logicalHeightWithVisibleOverflow().

Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
Cr-Commit-Position: refs/heads/master@{#429245}

[mstensho (USE GERRIT), 2016-11-01 18:25:21]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 18:25:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 19:56:08]

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

[commit-bot: I haz the power, 2016-11-01 19:56:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mstensho (USE GERRIT), 2016-11-01 20:39:53]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 20:40:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mstensho (USE GERRIT), 2016-11-01 21:42:33]

mstensho@opera.com changed reviewers:
+ eae@chromium.org

[eae, 2016-11-01 21:46:30]

Overall this looks great but I do have one (optional) suggestion.

LGTM

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

https://codereview.chromium.org/2465363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ColumnBalancer.cpp:113: m_flowThreadOffset
+= borderBoxOffset;
This looks a little fishy, instead of inflating the offset by the
borderBoxOffset why not pass the borderBoxOffset as a param to traverseSubtree?

[commit-bot: I haz the power, 2016-11-01 21:58:13]

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

[commit-bot: I haz the power, 2016-11-01 21:58:15]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2016-11-01 22:09:38]

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

https://codereview.chromium.org/2465363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ColumnBalancer.cpp:113: m_flowThreadOffset
+= borderBoxOffset;
On 2016/11/01 21:46:30, eae wrote:
> This looks a little fishy, instead of inflating the offset by the
> borderBoxOffset why not pass the borderBoxOffset as a param to
traverseSubtree?

I'm not sure if that would be cleaner, since we already modify
m_flowThreadOffset here (inflate by offsetForThisChild a few lines further up),
and this is the only method where we change m_flowThreadOffset.

Maybe there should be no m_flowThreadOffset at all, and instead add some offset
parameter to most of the methods? That would be for a different CL though.

[eae, 2016-11-01 22:10:44]

OK, please at least add a comment explaining why.

[mstensho (USE GERRIT), 2016-11-01 22:26:26]

On 2016/11/01 22:10:44, eae wrote:
> OK, please at least add a comment explaining why.

Done.

[mstensho (USE GERRIT), 2016-11-01 22:26:35]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-11-01 22:26:35]

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/2465363003/#ps40001
(title: "Rename to borderEdgeOffset and moar documentation.")

[commit-bot: I haz the power, 2016-11-01 22:27:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eae, 2016-11-01 22:27:43]

Thanks!

(still LGTM)

[commit-bot: I haz the power, 2016-11-01 23:00:59]

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

[commit-bot: I haz the power, 2016-11-01 23:01:01]

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

[mstensho (USE GERRIT), 2016-11-02 08:51:31]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2016-11-02 08:51:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 09:34:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-02 09:36:52]

Description was changed from

==========
Improve how the column balancer handles top margins on floats.

Float margins do not collapse with column boundaries, so we should make room
for them after the break, if the border box of the float starts in the next
column.

Let the balancer work on the margin box of the float (and the border box for
all other objects). For floats, we want to insert breaks before the
margin-before edge, not the border-before edge. This lets us remove
some special-code for unbreakable floats in InitialColumnHeightFinder, which
was the only place that previously bothered about this.

Changed how we determine which objects to process. We used to include the
overflow both before and after the border box, but we really don't have to
bother with content preceding it, since that shouldn't undergo fragmentation
anyway.

Discovered (one test regressed) that logicalHeightIncludingOverflow() also
included clipped overflow, which certainly wasn't the intention. This didn't
make much of a difference as long as the method was only called to check if we
could skip re-layout. But now we also use it to determine the column height.
Fixed it to only include visible overflow and renamed it to
logicalHeightWithVisibleOverflow().
==========

to

==========
Improve how the column balancer handles top margins on floats.

Float margins do not collapse with column boundaries, so we should make room
for them after the break, if the border box of the float starts in the next
column.

Let the balancer work on the margin box of the float (and the border box for
all other objects). For floats, we want to insert breaks before the
margin-before edge, not the border-before edge. This lets us remove
some special-code for unbreakable floats in InitialColumnHeightFinder, which
was the only place that previously bothered about this.

Changed how we determine which objects to process. We used to include the
overflow both before and after the border box, but we really don't have to
bother with content preceding it, since that shouldn't undergo fragmentation
anyway.

Discovered (one test regressed) that logicalHeightIncludingOverflow() also
included clipped overflow, which certainly wasn't the intention. This didn't
make much of a difference as long as the method was only called to check if we
could skip re-layout. But now we also use it to determine the column height.
Fixed it to only include visible overflow and renamed it to
logicalHeightWithVisibleOverflow().

Committed: https://crrev.com/7c82da727f64121aa34aa1decf82452c37ef7a2d
Cr-Commit-Position: refs/heads/master@{#429245}
==========

[commit-bot: I haz the power, 2016-11-02 09:36:53]

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

[please use gerrit instead, 2016-11-02 13:43:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2468193002/ by rouslan@chromium.org.

The reason for reverting is: Speculative revert to fix
csspaint/invalidation-background-image.html failure on "WebKit Win7 (dbg)" bot.

Failed build:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%....

[mstensho (USE GERRIT), 2016-11-03 08:42:58]

Relanded. csspaint/invalidation-background-image.html was flaky before this CL
landed.

[mstensho (USE GERRIT), 2016-11-03 08:43:23]

This also fixes bug 624784.