Avoid pathological layout on nested percent height tables

Avoid pathological layout on nested percent height tables

A second go at https://codereview.chromium.org/2670603002.

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/2701163002
Cr-Commit-Position: refs/heads/master@{#451662}
Committed: https://chromium.googlesource.com/chromium/src/+/a4b3b0ba32356230afd307830442db9a807008ad

[rhogan, 2017-02-18 10:29:52]

Description was changed from

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

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

to

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

A second go at https://codereview.chromium.org/2670603002.

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-18 10:29:53]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[rhogan, 2017-02-18 10:30:28]

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

[commit-bot: I haz the power, 2017-02-18 10:30:32]

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-18 12:08:54]

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

[commit-bot: I haz the power, 2017-02-18 12:08:55]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-02-20 13:01:31]

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes-2.html
(right):

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes-2.html:9:
<script src="../../resources/check-layout.js"></script>
Better use check-layout-th.js, to eliminate the -expected.txt counterpart.

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes-3.html
(right):

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes-3.html:22:
<script src="../../resources/check-layout.js"></script>
Better use check-layout-th.js, to eliminate the -expected.txt counterpart.

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html
(right):

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html:13:
<col style="width: 1161px;">
Is the insane width necessary? Are the COLGROUP and COL elements necessary at
all?

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html:23:
<script src="../../resources/check-layout.js"></script>
Time to switch to check-layout-th.js , to avoid the -expected* files.

See LayoutTests/fast/multicol/balance-line-underflow-1.html for an example.

Note that you also need to (at least to my knowledge) include
testharnessreport.js and testharness.js, just like that test does.

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:70:
m_oldAvailableLogicalHeight(0) {
No need (actually surprisingly expensive if you do) to initialize LayoutUnit.

[rhogan, 2017-02-20 18:38:16]

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html
(right):

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html:13:
<col style="width: 1161px;">
On 2017/02/20 at 13:01:31, mstensho wrote:
> Is the insane width necessary? Are the COLGROUP and COL elements necessary at
all?

Yes, otherwise m_columnLogicalWidthChanged gets set and the test lays out
correctly regardless.

[rhogan, 2017-02-20 18:38:39]

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

[commit-bot: I haz the power, 2017-02-20 18:38:45]

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

[rhogan, 2017-02-20 18:39:11]

On 2017/02/20 at 13:01:31, mstensho wrote:
ew.chromium.org/2701163002/diff/1/third_party/WebKit/Source/core/layout/LayoutTable.cpp#newcode70
> third_party/WebKit/Source/core/layout/LayoutTable.cpp:70:
m_oldAvailableLogicalHeight(0) {
> No need (actually surprisingly expensive if you do) to initialize LayoutUnit.

Thanks for the review mstensho - actioned all your comments I think.

[mstensho (USE GERRIT), 2017-02-20 19:02:32]

lgtm

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html
(right):

https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html:13:
<col style="width: 1161px;">
On 2017/02/20 18:38:16, rhogan wrote:
> On 2017/02/20 at 13:01:31, mstensho wrote:
> > Is the insane width necessary? Are the COLGROUP and COL elements necessary
at
> all?
> 
> Yes, otherwise m_columnLogicalWidthChanged gets set and the test lays out
> correctly regardless.

OK, thanks for explaining. I think I used border-spacing:0 to work around the
m_columnLogicalWidthChanged false positives, for the record.

[rhogan, 2017-02-20 20:49:47]

The CQ bit was unchecked by robhogan@gmail.com

[rhogan, 2017-02-20 20:49:50]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2017-02-20 20:50:03]

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

[rhogan, 2017-02-20 21:13:35]

On 2017/02/20 at 19:02:32, mstensho wrote:
> lgtm
> 
>
https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html
(right):
> 
>
https://codereview.chromium.org/2701163002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/table/layout-section-when-table-height-changes.html:13:
<col style="width: 1161px;">
> On 2017/02/20 18:38:16, rhogan wrote:
> > On 2017/02/20 at 13:01:31, mstensho wrote:
> > > Is the insane width necessary? Are the COLGROUP and COL elements necessary
at
> > all?
> > 
> > Yes, otherwise m_columnLogicalWidthChanged gets set and the test lays out
> > correctly regardless.
> 
> OK, thanks for explaining. I think I used border-spacing:0 to work around the
m_columnLogicalWidthChanged false positives, for the record.

Thanks for the review! :)

[commit-bot: I haz the power, 2017-02-20 22:10:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487623790678470,
"parent_rev": "9cff5cc970dd2b93d59ee9ebf3374b94f039409e", "commit_rev":
"a4b3b0ba32356230afd307830442db9a807008ad"}

[commit-bot: I haz the power, 2017-02-20 22:10:48]

Description was changed from

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

A second go at https://codereview.chromium.org/2670603002.

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 pathological layout on nested percent height tables

A second go at https://codereview.chromium.org/2670603002.

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/2701163002
Cr-Commit-Position: refs/heads/master@{#451662}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4b3b0ba32356230afd307830442...
==========

[commit-bot: I haz the power, 2017-02-20 22:10:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a4b3b0ba32356230afd307830442...