Update our treatment of repeating headers in tables

Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page - respecting this rule requires us
to reintroduce allowing nested repeated headers, something that both Edge and
FF do and which is now supported by the spec.

BUG=720620, 675904
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2882043002
Cr-Commit-Position: refs/heads/master@{#476212}
Committed: https://chromium.googlesource.com/chromium/src/+/9261f26075fab250385db2abde6c63deec1961ed

[rhogan, 2017-05-15 20:07:26]

Description was changed from

==========
bug 720620

BUG=720620
==========

to

==========
bug 720620

BUG=720620
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rhogan, 2017-05-15 20:07:55]

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

[commit-bot: I haz the power, 2017-05-15 20:08:49]

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-05-15 21:27:58]

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

[commit-bot: I haz the power, 2017-05-15 21:27:59]

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

[rhogan, 2017-05-20 10:59:34]

Description was changed from

==========
bug 720620

BUG=720620
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page.

BUG=720620
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rhogan, 2017-05-20 11:00:43]

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

[commit-bot: I haz the power, 2017-05-20 11:00:53]

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-05-20 11:03:09]

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

[commit-bot: I haz the power, 2017-05-20 11:03:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-05-20 15:22:17]

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

[rhogan, 2017-05-20 15:22:22]

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

[commit-bot: I haz the power, 2017-05-20 15:22:23]

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-05-20 17:03:11]

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

[commit-bot: I haz the power, 2017-05-20 17:03:12]

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), 2017-05-22 10:40:44]

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html
(left):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html:1:
<!DOCTYPE html>
:-/

No way to still do this as a reftest?

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutState.h (right):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutState.h:2: * Copyright (C) 2007
Apple Inc.  All rights reserved.
This looks like a revert of https://codereview.chromium.org/2545243002/ , at
least to some extent. Could you elaborate a little on that in the commit
message?

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutState.h:88: };
Drop the semicolon here, perhaps?

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutState.h:91: };
And here

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

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:669:
offset_for_table_headers += section->LogicalHeight();
So here we are re-introducing support for nested repeated headers, while
previously we only repeated the header of the innermost table? Is that really
something we want? I mean, it's fine, but only if the alternative (NOT
re-introducing it) would lead to more complex code. Or is the spec super-clear
on how this is supposed to be handled now? I couldn't find anything.

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (left):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:342: // is clarified.
Tracked by crbug.com/675904
That bug resolved by this CL?

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:307: void
SetHeaderGroupShouldRepeat() {
DetermineIfHeaderGroupShouldRepeat()?

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:66:
LayoutPoint(LayoutUnit(), table->RowOffsetFromRepeatingHeader() -
If we really want to re-introduce nesting:

Thanks to the current coding style, I think this would look prettier if you
instead did this as part of a separate LayoutPoint::Move(LayoutUnit, LayoutUnit)
step. Maybe even a separate LayoutUnit declaration step for the y thing.

[rhogan, 2017-05-22 17:35:00]

Description was changed from

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page.

BUG=720620
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page.

BUG=720620, 675904

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rhogan, 2017-05-22 18:04:35]

Description was changed from

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page.

BUG=720620, 675904

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page - respecting this rule requires us
to reintroduce allowing nested repeated headers, something that both Edge and
FF do and which is now supported by the spec.

BUG=720620, 675904

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rhogan, 2017-05-22 18:05:48]

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

[rhogan, 2017-05-22 18:06:04]

Thanks mstensho - applied your comments.

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html
(left):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-large-cell-with-header-expected.html:1:
<!DOCTYPE html>
On 2017/05/22 at 10:40:44, mstensho wrote:
> :-/
> 
> No way to still do this as a reftest?

I couldn't find the magic. :)

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutState.h (right):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutState.h:88: };
On 2017/05/22 at 10:40:44, mstensho wrote:
> Drop the semicolon here, perhaps?

Done.

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutState.h:91: };
On 2017/05/22 at 10:40:44, mstensho wrote:
> And here

Done.

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

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:669:
offset_for_table_headers += section->LogicalHeight();
On 2017/05/22 at 10:40:44, mstensho wrote:
> So here we are re-introducing support for nested repeated headers, while
previously we only repeated the header of the innermost table? Is that really
something we want? I mean, it's fine, but only if the alternative (NOT
re-introducing it) would lead to more complex code. Or is the spec super-clear
on how this is supposed to be handled now? I couldn't find anything.

I think the spec implies it now (by allowing you to repeat a header regardless
of whether one content row fits entirely on the page or not) and I know Francois
the spec editor wants to maintain the majority browser behaviour in most cases,
which is to repeat nested headers. I don't think it makes things simpler to
support it - but I think it will be hard to support repeating headers even where
one content row doesn't fit but not support nested headers.

Relevant discussion re 'one content row':

https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html

I can raise it but given that FF and Edge currently nest and the spec supports
that implicitly I don't think I'll get a response that supports removing the
nested case.

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (left):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:342: // is clarified.
Tracked by crbug.com/675904
On 2017/05/22 at 10:40:44, mstensho wrote:
> That bug resolved by this CL?

Yes, updated.

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:307: void
SetHeaderGroupShouldRepeat() {
On 2017/05/22 at 10:40:44, mstensho wrote:
> DetermineIfHeaderGroupShouldRepeat()?

Done.

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:66:
LayoutPoint(LayoutUnit(), table->RowOffsetFromRepeatingHeader() -
On 2017/05/22 at 10:40:44, mstensho wrote:
> If we really want to re-introduce nesting:
> 
> Thanks to the current coding style, I think this would look prettier if you
instead did this as part of a separate LayoutPoint::Move(LayoutUnit, LayoutUnit)
step. Maybe even a separate LayoutUnit declaration step for the y thing.

Done.

[commit-bot: I haz the power, 2017-05-22 18:06:40]

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), 2017-05-22 19:17:34]

Sorry, forgot to review the changes in LayoutBlockFlow.cpp!

I added some nits there, but my main concern is that this header offset isn't
applied at every location in the code where we insert breaks. What about
AdjustBlockChildForPagination() or ApplyForcedBreak(), for instance?

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

https://codereview.chromium.org/2882043002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:669:
offset_for_table_headers += section->LogicalHeight();
On 2017/05/22 18:06:04, rhogan wrote:
> On 2017/05/22 at 10:40:44, mstensho wrote:
> > So here we are re-introducing support for nested repeated headers, while
> previously we only repeated the header of the innermost table? Is that really
> something we want? I mean, it's fine, but only if the alternative (NOT
> re-introducing it) would lead to more complex code. Or is the spec super-clear
> on how this is supposed to be handled now? I couldn't find anything.
> 
> I think the spec implies it now (by allowing you to repeat a header regardless
> of whether one content row fits entirely on the page or not) and I know
Francois
> the spec editor wants to maintain the majority browser behaviour in most
cases,
> which is to repeat nested headers. I don't think it makes things simpler to
> support it - but I think it will be hard to support repeating headers even
where
> one content row doesn't fit but not support nested headers.
> 
> Relevant discussion re 'one content row':
> 
> https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html
> 
> I can raise it but given that FF and Edge currently nest and the spec supports
> that implicitly I don't think I'll get a response that supports removing the
> nested case.
> 

Thanks for the clarification. I agree that support for nested tables is now
implied. I definitely think you did the right thing by bringing it back.

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1171: if
(!IsTableCell()) {
Table cells should never accept struts, so this check is unnecessary. I suggest
that you change it to a DCHECK.

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1203: if
(!IsTableCell()) {
Ditto.

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1205:
View()->GetLayoutState()->HeightOffsetForTableHeaders();
I think this is now the third time we do View()->GetLayoutState() in this
method. How about declaring a const layout_state variable instead?

[commit-bot: I haz the power, 2017-05-22 20:20:26]

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

[commit-bot: I haz the power, 2017-05-22 20:20:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2017-05-25 19:43:01]

On 2017/05/22 at 19:17:34, mstensho wrote:
> Sorry, forgot to review the changes in LayoutBlockFlow.cpp!
> 
> I added some nits there, but my main concern is that this header offset isn't
applied at every location in the code where we insert breaks. What about
AdjustBlockChildForPagination() or ApplyForcedBreak(), for instance?

When we have repeated headers we always force a break on any row that doesn't
fit at the end of the page so that the header can start at the top of the next
page and the row can appear below it. So I couldn't come up with a case that hit
either of these.

[mstensho (USE GERRIT), 2017-05-26 10:56:50]

On 2017/05/25 19:43:01, rhogan wrote:
> On 2017/05/22 at 19:17:34, mstensho wrote:
> > Sorry, forgot to review the changes in LayoutBlockFlow.cpp!
> > 
> > I added some nits there, but my main concern is that this header offset
isn't
> applied at every location in the code where we insert breaks. What about
> AdjustBlockChildForPagination() or ApplyForcedBreak(), for instance?
> 
> When we have repeated headers we always force a break on any row that doesn't
> fit at the end of the page so that the header can start at the top of the next
> page and the row can appear below it. So I couldn't come up with a case that
hit
> either of these.

<p>There should be no text overlap below.</p>
<div style="columns:2; line-height:20px; column-fill:auto; height:100px;
orphans:1; widows:1; background:yellow;">
    <table cellspacing="0" cellpadding="0" style="background:pink;">
        <thead style="break-inside:avoid;">
            <tr>
                <th>repeated header</th>
            </tr>
        </thead>
        <tbody>
            <tr>
                <td>
                    <div>
                        line1<br>
                        line2<br>
                        line3<br>
                        <div style="break-inside:avoid;">
                            line4<br>
                            line5<br>
                            line6<br>
                        </div>
                    </div>
                </td>
            </tr>
        </tbody>
    </table>
</div>

Maybe we could exercise the code in CalculatePaginationStrutToFitContent() that
deals with making room for repeated headers in more cases than merely if
(IsTableCell()). Couldn't we just add LayoutState's
HeightOffsetForTableHeaders() regardless of object type?

[rhogan, 2017-05-27 12:47:41]

On 2017/05/26 at 10:56:50, mstensho wrote:
> 
> Maybe we could exercise the code in CalculatePaginationStrutToFitContent()
that deals with making room for repeated headers in more cases than merely if
(IsTableCell()). Couldn't we just add LayoutState's
HeightOffsetForTableHeaders() regardless of object type?

Nice one, thanks! Added the test and your suggestion.

Found crbug.com/727016 which I think I'll need to look at separately.

[rhogan, 2017-05-27 12:47:55]

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

[commit-bot: I haz the power, 2017-05-27 12:48:02]

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-05-27 14:36:28]

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

[commit-bot: I haz the power, 2017-05-27 14:36:29]

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), 2017-05-29 10:22:00]

https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1173: pagination_strut
+= layout_state->HeightOffsetForTableHeaders();
I was actually expecting this one to go away, now that
CalculatePaginationStrutToFitContent() always adds in this.

https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1203: logical_offset +
layout_state->HeightOffsetForTableHeaders();
This one we need to keep, though, since CalculatePaginationStrutToFitContent()
isn't used.

https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1209: LayoutUnit strut
=
How about here? Looks like we're going to end up pushing the block to the very
top of some fragmentainer, overlapping any repeated table header.

[rhogan, 2017-05-30 19:07:31]

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

[commit-bot: I haz the power, 2017-05-30 19:08:07]

Dry run: CQ is trying da patch.

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

[rhogan, 2017-05-30 19:08:42]

On 2017/05/29 at 10:22:00, mstensho wrote:
>
https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1173:
pagination_strut += layout_state->HeightOffsetForTableHeaders();
> I was actually expecting this one to go away, now that
CalculatePaginationStrutToFitContent() always adds in this.
> 
> 
>
https://codereview.chromium.org/2882043002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1209: LayoutUnit
strut =
> How about here? Looks like we're going to end up pushing the block to the very
top of some fragmentainer, overlapping any repeated table header.

Applied these two comments and added a couple of tests to cover them:

LayoutTests/fragmentation/single-cell-repeating-thead-break-inside-always-content-first-line.html
LayoutTests/fragmentation/single-cell-repeating-thead-break-inside-always-content.html

[rhogan, 2017-05-30 19:39:25]

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

[commit-bot: I haz the power, 2017-05-30 19:40:00]

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), 2017-05-30 19:43:55]

lgtm

https://codereview.chromium.org/2882043002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fragmentation/single-cell-repeating-thead-break-inside-always-content-first-line.html
(right):

https://codereview.chromium.org/2882043002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fragmentation/single-cell-repeating-thead-break-inside-always-content-first-line.html:22:
<div style="break-inside:always; border-top: 40px solid black;">
No such thing as break-inside:always. Guess you should rename these tests as
well. :)

https://codereview.chromium.org/2882043002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2882043002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1206:
line_box.SetIsFirstAfterPageBreak(true);
Already taken care of above (line 1192).

[commit-bot: I haz the power, 2017-05-30 19:49:09]

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

[commit-bot: I haz the power, 2017-05-30 19:49:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_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_...)

[rhogan, 2017-05-31 18:04:29]

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

[commit-bot: I haz the power, 2017-05-31 18:05:06]

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-05-31 18:24:14]

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

[commit-bot: I haz the power, 2017-05-31 18:24:16]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[rhogan, 2017-05-31 21:10:55]

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

[commit-bot: I haz the power, 2017-05-31 21:11:52]

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-06-01 02:05:14]

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

[commit-bot: I haz the power, 2017-06-01 02:05:16]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-06-01 06:38:22]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-06-01 06:38:23]

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

[commit-bot: I haz the power, 2017-06-01 06:38:40]

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-06-01 06:43:34]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496299102850860,
"parent_rev": "e1396f8adefd2f71ed33732f5ffe013bcf87990a", "commit_rev":
"9261f26075fab250385db2abde6c63deec1961ed"}

[commit-bot: I haz the power, 2017-06-01 06:43:48]

Description was changed from

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page - respecting this rule requires us
to reintroduce allowing nested repeated headers, something that both Edge and
FF do and which is now supported by the spec.

BUG=720620, 675904

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update our treatment of repeating headers in tables

https://drafts.csswg.org/css-tables-3/#repeated-headers has been updated since
we first implemented them. Respect the new rules such as only repeating headers
if they fit on at most a quarter of the page. We also need to repeat headers
even if only part of a row fits on the page - respecting this rule requires us
to reintroduce allowing nested repeated headers, something that both Edge and
FF do and which is now supported by the spec.

BUG=720620, 675904

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2882043002
Cr-Commit-Position: refs/heads/master@{#476212}
Committed:
https://chromium.googlesource.com/chromium/src/+/9261f26075fab250385db2abde6c...
==========

[commit-bot: I haz the power, 2017-06-01 06:43:51]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/9261f26075fab250385db2abde6c...

[rhogan, 2017-06-01 18:45:25]

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1171: if
(!IsTableCell()) {
On 2017/05/22 at 19:17:34, mstensho wrote:
> Table cells should never accept struts, so this check is unnecessary. I
suggest that you change it to a DCHECK.

Done.

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1203: if
(!IsTableCell()) {
On 2017/05/22 at 19:17:34, mstensho wrote:
> Ditto.

Done.

https://codereview.chromium.org/2882043002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1205:
View()->GetLayoutState()->HeightOffsetForTableHeaders();
On 2017/05/22 at 19:17:34, mstensho wrote:
> I think this is now the third time we do View()->GetLayoutState() in this
method. How about declaring a const layout_state variable instead?

Done.