Avoid pathological layout on nested percent height tables

Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(), layoutRows()) 
during the layout of the section - and this cascades down the tree with the lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.


BUG=687061
Review-Url: https://codereview.chromium.org/2670603002
Cr-Commit-Position: refs/heads/master@{#449101}
Committed: https://chromium.googlesource.com/chromium/src/+/d3ceb0147ad9f4d4a9b41cea0362d6221cb72302

[rhogan, 2017-02-01 22:16:44]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 22:17:12]

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, 2017-02-01 23:51:10]

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

[commit-bot: I haz the power, 2017-02-01 23:51:11]

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_...)

[rhogan, 2017-02-05 19:03:18]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-05 19:03:25]

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, 2017-02-05 20:32:44]

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

[commit-bot: I haz the power, 2017-02-05 20:32:45]

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_...)

[rhogan, 2017-02-06 20:32:23]

Description was changed from

==========
Avoid pathological layout on nested percent height tables

BUG=687061
==========

to

==========
Avoid unnecessary cell layout on nested percent height tables

When computing the height of the section based on row sizes
(calcRowLogicalHeight()),
don't bother forcing layout on cells that depend on the height of their rows
unless the section itself needed a layout, indicating that the cell's intrinsic
height may need to be recalc'd.




BUG=687061
==========

[rhogan, 2017-02-06 20:37:08]

Description was changed from

==========
Avoid unnecessary cell layout on nested percent height tables

When computing the height of the section based on row sizes
(calcRowLogicalHeight()),
don't bother forcing layout on cells that depend on the height of their rows
unless the section itself needed a layout, indicating that the cell's intrinsic
height may need to be recalc'd.




BUG=687061
==========

to

==========
Avoid unnecessary cell layout on nested percent height tables

Don't bother computing the height of the section based on row sizes
(calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will
do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.


BUG=687061
==========

[rhogan, 2017-02-06 20:45:13]

Description was changed from

==========
Avoid unnecessary cell layout on nested percent height tables

Don't bother computing the height of the section based on row sizes
(calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will
do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.


BUG=687061
==========

to

==========
Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(),
layoutRows()) 
during the layout of the section - and this cascades down the tree with the
lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row
sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will
do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.


BUG=687061
==========

[rhogan, 2017-02-06 20:47:07]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[rhogan, 2017-02-06 20:47:22]

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

[commit-bot: I haz the power, 2017-02-06 20:47:36]

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

[eae, 2017-02-06 20:55:23]

LGTM

Thank you Rob!

[mstensho (USE GERRIT), 2017-02-06 21:11:07]

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
(right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html:4:
table { background-color: lightblue; height: 99px; }
This seems wrong. The table in the test really is 100px tall, isn't it? I don't
think this is an acceptable regression.

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Perf...
File
third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html
(right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html:1:
<!DOCTYPE HTML>
It's always awesome to land the performance test separately before landing the
fix, to observe the performance improvement and pat yourself on the back. :) But
I'm fine either way.

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Perf...
third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html:85:
PerfTestRunner.forceLayout();
Can you be sure that nothing has already been laid out here? Also: this
operation potentially includes computing style and creating layout objects,
which you don't really want to measure.

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:454: bool neededLayout =
section.needsLayout();
Might as well do:

if (section.needsLayout()) {
  section.layout();
  section.setLogicalHeight(LayoutUnit(section.calcRowLogicalHeight()));
}

[rhogan, 2017-02-06 21:21:09]

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:454: bool neededLayout =
section.needsLayout();
On 2017/02/06 at 21:11:07, mstensho wrote:
> Might as well do:
> 
> if (section.needsLayout()) {
>   section.layout();
>   section.setLogicalHeight(LayoutUnit(section.calcRowLogicalHeight()));
> }

section.layout() is private, should I just make it public?

[mstensho (USE GERRIT), 2017-02-06 21:25:27]

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:454: bool neededLayout =
section.needsLayout();
On 2017/02/06 21:21:08, rhogan wrote:
> On 2017/02/06 at 21:11:07, mstensho wrote:
> > Might as well do:
> > 
> > if (section.needsLayout()) {
> >   section.layout();
> >   section.setLogicalHeight(LayoutUnit(section.calcRowLogicalHeight()));
> > }
> 
> section.layout() is private, should I just make it public?

Oh. Yes. This is exactly why it's problematic to override a public method as
private. :)

[commit-bot: I haz the power, 2017-02-06 22:16:55]

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

[commit-bot: I haz the power, 2017-02-06 22:16:59]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-02-07 19:20:54]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[rhogan, 2017-02-07 19:20:54]

On 2017/02/06 at 21:25:27, mstensho wrote:
>
https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):
> 
>
https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTable.cpp:454: bool neededLayout =
section.needsLayout();
> On 2017/02/06 21:21:08, rhogan wrote:
> > On 2017/02/06 at 21:11:07, mstensho wrote:
> > > Might as well do:
> > > 
> > > if (section.needsLayout()) {
> > >   section.layout();
> > >   section.setLogicalHeight(LayoutUnit(section.calcRowLogicalHeight()));
> > > }
> > 
> > section.layout() is private, should I just make it public?
> 
> Oh. Yes. This is exactly why it's problematic to override a public method as
private. :)

Applied your comments - and fixed the performance test, thanks!

[mstensho (USE GERRIT), 2017-02-07 19:52:33]

Code changes look fine to me, but there seem to be serious regressions
introduced by this CL? See other comment.

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
(right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html:4:
table { background-color: lightblue; height: 99px; }
On 2017/02/06 21:11:07, mstensho wrote:
> This seems wrong. The table in the test really is 100px tall, isn't it? I
don't
> think this is an acceptable regression.

This is still unresolved.

[rhogan, 2017-02-07 20:27:42]

Whoops, completely missed that.

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
(right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html:4:
table { background-color: lightblue; height: 99px; }
On 2017/02/07 at 19:52:33, mstensho wrote:
> On 2017/02/06 21:11:07, mstensho wrote:
> > This seems wrong. The table in the test really is 100px tall, isn't it? I
don't
> > think this is an acceptable regression.
> 
> This is still unresolved.

Without my CL the rendered height on the table in
table-two-pass-layout-overpaint-expected.html file is 98px. The extra 2px get
discarded because distributeExtraLogicalHeight() fails to allocate them from the
'extra height available' of 46px left over by calcRowLogicalHeight(). Likewise
for LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html.

With my CL (and leaving table-two-pass-layout-overpaint-expected.html unchanged)
the rendered height on the table in
table-two-pass-layout-overpaint-expected.html is 99px. This is because we are
using the 98px we arrived at the last time we laid out the section and trying to
redistribute the 2px. We manage to allocate 1px and are left with 1px left over.

On LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html we are
still left with 2px left over because we're doing a single layout pass as a
result of the style change, so visiting calcRowLogicalHeight(), getting 46px to
reallocate, and getting left with 2px.

So on the one hand we're not rendering the 100px table as 100px in the first
place, but we are left with an 1px inconsistency in the height depending on how
many layouts we trigger.

Taking on the FIXME in distributeExtraLogicalHeight() as a follow-up seems like
the way to address this? I suspect it will be a tricky one.

[mstensho (USE GERRIT), 2017-02-07 21:09:10]

lgtm, with some boring paperwork left, that I hope you have time to do before
landing.

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html
(right):

https://codereview.chromium.org/2670603002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html:4:
table { background-color: lightblue; height: 99px; }
On 2017/02/07 20:27:42, rhogan wrote:
> On 2017/02/07 at 19:52:33, mstensho wrote:
> > On 2017/02/06 21:11:07, mstensho wrote:
> > > This seems wrong. The table in the test really is 100px tall, isn't it? I
> don't
> > > think this is an acceptable regression.
> > 
> > This is still unresolved.
> 
> Without my CL the rendered height on the table in
> table-two-pass-layout-overpaint-expected.html file is 98px. The extra 2px get
> discarded because distributeExtraLogicalHeight() fails to allocate them from
the
> 'extra height available' of 46px left over by calcRowLogicalHeight(). Likewise
> for LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html.
> 
> With my CL (and leaving table-two-pass-layout-overpaint-expected.html
unchanged)
> the rendered height on the table in
> table-two-pass-layout-overpaint-expected.html is 99px. This is because we are
> using the 98px we arrived at the last time we laid out the section and trying
to
> redistribute the 2px. We manage to allocate 1px and are left with 1px left
over.
> 
> On LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html we are
> still left with 2px left over because we're doing a single layout pass as a
> result of the style change, so visiting calcRowLogicalHeight(), getting 46px
to
> reallocate, and getting left with 2px.
> 
> So on the one hand we're not rendering the 100px table as 100px in the first
> place, but we are left with an 1px inconsistency in the height depending on
how
> many layouts we trigger.
> 
> Taking on the FIXME in distributeExtraLogicalHeight() as a follow-up seems
like
> the way to address this? I suspect it will be a tricky one. 
> 

Oh dear. I had no idea it was this broken. Thanks for the explanation. Yeah,
attacking that FIXME sounds like the proper way to fix this (and it might become
more urgent with this fix landed... then again, nobody has been complaining
loudly about us losing a couple of pixels either, so...). So the test used to
pass by accident. It's confusing that the test and the ref don't use the same
table height, though.

I think you should either:
A: Keep the ref at 100px and change it to also use two-pass layout (by changing
the table width, or by setting display:none on some new sibling of #target, or
whatever), and document why you do this.
or:
B: document why it has to be 99px.

A is silly, but, on the other hand, the specified height mismatch between the
test and the ref is hacky.

We probably need a bug report for the FIXME at this point, so that we can refer
to it.

https://codereview.chromium.org/2670603002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html
(right):

https://codereview.chromium.org/2670603002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html:32:
<div class="table container" style="display: block; float: left; height: 99px;">
This deserves an comment, with a reference to the bug about the table height
distribution machinery having a couple of screws or pixels loose.

(yeah, 98px wasn't right either, but I still think this deserves an explanation)

[rhogan, 2017-02-08 18:32:41]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 18:33:16]

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, 2017-02-08 21:33:50]

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

[commit-bot: I haz the power, 2017-02-08 21:33:54]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-02-08 22:00:09]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-02-08 22:00:09]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/2670603002/#ps80001
(title: "bug 687061")

[commit-bot: I haz the power, 2017-02-08 22:00:51]

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, 2017-02-08 22:07:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486591209218510,
"parent_rev": "1dfa16b2126e9776a3e26f00b4c767b83f2645b5", "commit_rev":
"d3ceb0147ad9f4d4a9b41cea0362d6221cb72302"}

[commit-bot: I haz the power, 2017-02-08 22:07:55]

Description was changed from

==========
Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(),
layoutRows()) 
during the layout of the section - and this cascades down the tree with the
lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row
sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will
do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.


BUG=687061
==========

to

==========
Avoid unnecessary cell layout on nested percent height tables

In the test, the nested percent height tables mean that each cell will get at
least three layouts (LayoutTableRow::layout(),calcRowLogicalHeight(),
layoutRows()) 
during the layout of the section - and this cascades down the tree with the
lowest
children getting hundreds of repeated layouts.

This inefficiency has always been there, we've just introduced it for the
percent height situation with crrev.com/2441373002.

To avoid this don't bother computing the height of the section based on row
sizes (calcRowLogicalHeight()),
unless the section itself needed a layout - the previously calculated value will
do.

The only complication this introduces is: any extra logical height from the
table that we previously discarded when distributing to the rows we will now
try to distribute again. This can change the size of the section and cause extra
paints.


BUG=687061

Review-Url: https://codereview.chromium.org/2670603002
Cr-Commit-Position: refs/heads/master@{#449101}
Committed:
https://chromium.googlesource.com/chromium/src/+/d3ceb0147ad9f4d4a9b41cea0362...
==========

[commit-bot: I haz the power, 2017-02-08 22:07:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d3ceb0147ad9f4d4a9b41cea0362...

[eae, 2017-02-17 23:01:05]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2700183003/ by eae@chromium.org.

The reason for reverting is: Broke Google Hangouts.

BUG=693722.