Repeat footers in paginated context

Repeat footers in paginated context

BUG=656232, 620223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2584143003
Cr-Commit-Position: refs/heads/master@{#492243}
Committed: https://chromium.googlesource.com/chromium/src/+/a098f68e0de0ba6a40f2524e0f4b437f76c6e9a3

[rhogan, 2016-12-17 14:19:30]

Description was changed from

==========
Repeat footers in paginated context

BUG=656232
==========

to

==========
Repeat footers in paginated context

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

[rhogan, 2016-12-17 14:19:37]

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

[commit-bot: I haz the power, 2016-12-17 14:19:41]

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-12-17 16:03:19]

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

[commit-bot: I haz the power, 2016-12-17 16:03:20]

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-04-20 20:05:58]

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

[commit-bot: I haz the power, 2017-04-20 20:07:10]

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-04-20 21:36:49]

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

[commit-bot: I haz the power, 2017-04-20 21:36:50]

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

[rhogan, 2017-04-22 11:12:51]

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

[commit-bot: I haz the power, 2017-04-22 11:12:58]

Dry run: CQ is trying da patch.

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

[rhogan, 2017-04-22 11:17:31]

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

[rhogan, 2017-04-22 11:17:32]

HI mstensho - can you have a look when you get the chance?

[commit-bot: I haz the power, 2017-04-22 12:54:03]

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

[commit-bot: I haz the power, 2017-04-22 12:54:04]

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

[mstensho (USE GERRIT), 2017-04-24 09:50:04]

On 2017/04/22 11:17:32, rhogan wrote:
> HI mstensho - can you have a look when you get the chance?

Can you use another bug number? The one you used is marked as duplicate.

[mstensho (USE GERRIT), 2017-04-24 11:57:17]

Haven't reviewed the tests yet.

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

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:649: // Lay out table
footer.
Is it necessary to do this out-of-order? We should first lay out top captions,
then the header, then the bodies, then the footer, and then the bottom captions.
Otherwise fragmentation (pagination struts) will be wrong.

Even if break-inside:avoid is set, it doesn't necessarily mean that there won't
be any breaks inside.

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

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.h:257: LayoutUnit position)
const {
Doesn't really look like a const method to me.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTable.h:261: const LayoutUnit
GetPositionOfRepeatingFooterGroupOnPage(int page) const {
The previous method uses unsigned. This one uses int. I suppose we can be
consistent? Using int is probably better, even if negative values are not to be
used here.

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

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1236: bool&
can_repeat_footer) const {
I think calculating can_repeat_footer should be omitted from this method; it
already does enough work.

Since there's only one call site, we could just do it there, rather than
introducing a utility method for it.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1252:
Table()->RowOffsetFromRepeatingHeader() && row->RowIndex();
RowOffsetFromRepeatingHeader() suggests that we depend on a repeating header for
a repeating footer to work, but that can't be right, can it?

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1254: if
(row_logical_height + footer->LogicalHeight() <= page_logical_height) {
... however, if there *is* a repeating header, we need to add in its height here
in the comparison, don't we?

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2109:
Table()->RowOffsetFromRepeatingHeader() && can_repeat_footer) {
Check can_repeat_footer first.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2111:
PageLogicalHeightForOffset(LayoutUnit()));
PageLogicalHeightForOffset() may be 0 (initial column balancing pass), so better
check that before doing any of this.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2113: page,
LogicalTop() + row_object.LogicalTop());
Then again, do we really need to allow individual positions for the footer on
each page? Why not just bottom-align it (unless we're at the last page for the
table)? Edge seems to do that.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2136: if
(GetPaginationBreakability() == LayoutBox::kAllowAnyBreaks)
LayoutBox:: scope unnecessary.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2138: //
TODO(rhogan): Should we paint a header repeatedly if it's self-painting?
I don't think it's currently possible to have a self painting layer for a table
section. If I'm right, you might as well DCHECK.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2141: LayoutUnit
page_height = Table()->PageLogicalHeightForOffset(LayoutUnit());
If "page" height isn't uniform (e.g. a multicol container nested inside another
fragmentation context), you end up with incorrect heights here. Need to specify
the correct offset instead of LayoutUnit().

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2192:
rect.SetHeight(Table()->LogicalHeight());
Introducing a lot of duplicated code here (shared with the code below for
repeating headers). Is there a better way?

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2194: bool mapped =
Table()->MapToVisualRectInAncestorSpaceInternal(
We here skip the table section, ignoring its position. Why don't we do the same
below, for repeated table headers?

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

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:21: void
TableSectionPainter::PaintRepeatingFooterGroup(
Please match the order of declaration. There PaintRepeatingFooterGroup() comes
after PaintRepeatingHeaderGroup().

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:30:
table->SubtractCaptionRect(sections_rect, true);
Why do we want to keep the top captions?

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:32:
sections_rect.Height() - table->OuterBorderAfter();
Mixing physical (Height()) and logical (OuterBorderAfter()) dimensions. Might as
well use OuterBorderBottom() perhaps, since this code already seems to assume
that writing mode isn't twisted.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:34:
layout_table_section_.FirstRow()->PaginationStrut());
Will there ever be a pagination strut here? Seems wrong. That would mean that
the first row of the repeating header has been pushed to the next fragmentainer.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:96:
(paint_offset.Y() - first_row_offset) + total_height_of_rows);
No need for the parentheses.

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:118: if
(table->Footer() == layout_table_section_) {
else if

https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:179: if
(table->Footer() == layout_table_section_) {
else if

[rhogan, 2017-05-06 10:36:49]

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

[commit-bot: I haz the power, 2017-05-06 10:37:21]

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-06 11:41:16]

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

[commit-bot: I haz the power, 2017-05-06 11:41:17]

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-06 12:46:22]

On 2017/04/24 at 11:57:17, mstensho wrote:
>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):
> 
>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTable.cpp:649: // Lay out table
footer.
> Is it necessary to do this out-of-order? We should first lay out top captions,
then the header, then the bodies, then the footer, and then the bottom captions.
Otherwise fragmentation (pagination struts) will be wrong.
> 
> Even if break-inside:avoid is set, it doesn't necessarily mean that there
won't be any breaks inside.
I explain this in a comment now.

> 
>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1254: if
(row_logical_height + footer->LogicalHeight() <= page_logical_height) {
> ... however, if there *is* a repeating header, we need to add in its height
here in the comparison, don't we?
Right enough, however this check is no longer necessary as a footer will now
only repeat on the page if its <= 25% of the page height.

>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2111:
PageLogicalHeightForOffset(LayoutUnit()));
> PageLogicalHeightForOffset() may be 0 (initial column balancing pass), so
better check that before doing any of this.
Introduced a DCHECK for this.

>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2113: page,
LogicalTop() + row_object.LogicalTop());
> Then again, do we really need to allow individual positions for the footer on
each page? Why not just bottom-align it (unless we're at the last page for the
table)? Edge seems to do that.
Haven't done this. I think I prefer to align them with the bottom row on each
page.

>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2138: //
TODO(rhogan): Should we paint a header repeatedly if it's self-painting?
> I don't think it's currently possible to have a self painting layer for a
table section. If I'm right, you might as well DCHECK.
They can be alright. I've amended the TODO to address this.

>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2194: bool mapped
= Table()->MapToVisualRectInAncestorSpaceInternal(
> We here skip the table section, ignoring its position. Why don't we do the
same below, for repeated table headers?
Seems simplest for both cases just to return the table's rect - in either case
we want to paint all the headers/footers in the table. 

>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:30:
table->SubtractCaptionRect(sections_rect, true);
> Why do we want to keep the top captions?
Addressed in new comment.

>
https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:34:
layout_table_section_.FirstRow()->PaginationStrut());
> Will there ever be a pagination strut here? Seems wrong. That would mean that
the first row of the repeating header has been pushed to the next fragmentainer.
Not sure why that would be wrong. It's possible for the footer to be at the top
of the page and to have used a strut to move its first row of content there no?

[rhogan, 2017-05-06 13:01:57]

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

[commit-bot: I haz the power, 2017-05-06 13:02:39]

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-06 14:53:37]

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

[commit-bot: I haz the power, 2017-05-06 14:53:38]

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

[rhogan, 2017-05-07 21:14:23]

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

[commit-bot: I haz the power, 2017-05-07 21:14:46]

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-07 22:32:45]

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

[commit-bot: I haz the power, 2017-05-07 22:32:46]

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

[mstensho (USE GERRIT), 2017-05-08 13:56:00]

Still not done with the tests.

I think I agree now that individual footer positions per fragmentainer may be
better and give more predictable behavior, so that it's worth the extra code.
Firefox does it too (while Edge doesn't seem to do it, which I've already
pointed out).

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html
(right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html:19:
</tr>
Why changes here? No repeating footers here.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:72:
position_of_repeating_footer_group_on_page_.Fill(LayoutUnit(), 1);
Seems unnecessary if we're not paginating.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:658: // if we can repeat
it in each page/column.
I must say that I'm still a bit concerned about this. We're performing layout
before knowing the position. But maybe it's okay. Having break-inside:avoid,
we're only going to break inside this section if it's taller than the height of
one page, or if there are forced breaks inside. That would disqualify it from
becoming repeated, I suppose, since its logical height will be taller than that
of one page.

Since I cannot think of anything in particular that could go wrong, I suppose I
just have to hold my peace. :)

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:256: void
SetPositionOfRepeatingFooterGroupOnPage(int page, LayoutUnit position) {
Could you document here that |page| is the page number relatively to the page
where the table starts - NOT the overall page number?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:257:
position_of_repeating_footer_group_on_page_.Grow(page + 1);
Grow() requires that you're actually requesting growth, and it seems that
there's no guarantee that page+1 is larger than the current size of the vector,
since nobody resets it at the beginning of layout, so that a subsequent layout
is bound to hit the DCHECK_GE failure in Vector::Grow().

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:569: mutable
Vector<LayoutUnit> position_of_repeating_footer_group_on_page_;
Are you sure you shouldn't use <int> here? Tables don't really deal a lot with
subpixels. Also, this should probably be a typedef, since you need it for
PositionOfRepeatingFooterGroupOnPage() as well.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1257:
LayoutTableSection* footer = Table()->Footer();
This one can be const, I guess?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1258: bool
footer_wants_to_repeat =
make_room_for_repeating_footer?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1260:
footer->IsRepeatingFooterGroup(const_cast<LayoutTableSection*>(this)) &&
Ugh. But is there really any reason not to let the parameter be const?
IsRepeatingFooterGroup() only seems to call PageLogicalHeightForOffset() and
LogicalTop() on the object passed.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1277: 
Back out this?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2075:
LayoutTableSection* footer = Table()->Footer();
Can you push it a few lines down, next to where it's actually used?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2109: if (footer &&
footer != this && header != this &&
If the header isn't repeating, there should be no reason to exclude it here?

Also, it seems kind of harder than necessary to exercise this code. You either
need a repeating header to help, or table rows with break-inside:avoid. If
there's neither, we don't get repeated footers.

Consider this:

I'd expect the footer to be repeated here:

<!DOCTYPE html>
<p>There should be a blue <em>square</em> below.</p>
<div style="columns:2; width:20px; column-gap:0; column-fill:auto; height:90px;
line-height:20px;">
    <table style="width:100%;" cellspacing="0" cellpadding="0">
        <tbody>
            <tr>
                <td><br></td>
            </tr>
            <tr>
                <td><br></td>
            </tr>
            <tr>
                <td><br></td>
            </tr>
            <tr>
                <td><br></td>
            </tr>
            <tr>
                <td><br></td>
            </tr>
            <tr>
                <td><br></td>
            </tr>
        </tbody>
        <tfoot style="break-inside:avoid;">
            <tr>
                <td style="background:blue;">
                    <br>
                </td>
            </tr>
        </tfoot>
    </table>
</div>

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2114:
FloorToInt(offset_from_top / PageLogicalHeightForOffset(LayoutUnit()));
To cope with non-uniform page heights, you should consider passing the correct
offset to PageLogicalHeightForOffset().

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2116:
row_object.LogicalTop(), LayoutBlock::kAssociateWithLatterPage);
LayoutBlock:: scope should be unnecessary. This one is defined in LayoutBox.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2141: bool
LayoutTableSection::IsRepeatingFooterGroup(
This belongs AFTER IsRepeatingHeaderGroup().

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2146: //
TODO(rhogan): Sections can be self-painting.
Can you tell me how they can be self-painting?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2151: ?
section_for_page->PageLogicalHeightForOffset(
This is to avoid assertion failures and the bugs that follow? (since you can't
call PageLogicalHeightForOffset() for any other object than the one you're
currently laying out)

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2157: if
(LogicalHeight() > 0 && page_height / LogicalHeight() < 4)
The code looks fine, but I'd like a comment that mumbles something about about
25%...

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2175: if
(LogicalHeight() > 0 && page_height / LogicalHeight() < 4)
The code looks fine, but I'd like a comment that mumbles something about 25%...

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2213: return
Table()->MapToVisualRectInAncestorSpaceInternal(
How can we just skip one ancestor in the chain like this and ignore the location
of the table section?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:354:
LayoutTableSection* section_for_page = nullptr) const;
This parameter needs some documentation.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:36: LayoutUnit
first_row_offset;
Could you make "pagination" or at least "strut" part of that variable name?

What does this have to do with repeated footers (this CL), BTW?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:83: // Include the
top captions as we need top_offset to be
Include, as in exclude from subtraction? How about "Keep"? :)

OK, I think I want something other than bool to express this.

How about enum class CaptionSelector { TopCaptionsOnly, BottomCaptionsOnly,
AllCaptions } (should probably remove TopCaptionsOnly, though, since it's not
used)

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:99:
table->GetPositionOfRepeatingFooterGroupOnPage(page).ToInt();
Since the whole vector has already been made accessible via
table->PositionOfRepeatingFooterGroupOnPage() anyway, why not use it directly?
Alternatively, maybe remove PositionOfRepeatingFooterGroupOnPage(), and add
whatever functionality is required to encapsulate the vector completely.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:100: if
(!footer_position)
When is it 0?

[rhogan, 2017-05-08 18:30:56]

Description was changed from

==========
Repeat footers in paginated context

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

to

==========
Repeat footers in paginated context

BUG=656232, 620223

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

[rhogan, 2017-05-08 18:31:54]

Description was changed from

==========
Repeat footers in paginated context

BUG=656232, 620223

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

to

==========
Repeat footers in paginated context

BUG=656232,620223

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

[rhogan, 2017-05-09 19:56:50]

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

[rhogan, 2017-05-09 19:57:17]

Thanks mstensho for that super review - addressed all your comments I hope.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html
(right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html:19:
</tr>
On 2017/05/08 at 13:55:58, mstensho wrote:
> Why changes here? No repeating footers here.

This is so that the header is < 25% of the table.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:72:
position_of_repeating_footer_group_on_page_.Fill(LayoutUnit(), 1);
On 2017/05/08 at 13:55:59, mstensho wrote:
> Seems unnecessary if we're not paginating.

I'm not sure I can zap this and rebuild it each time I layout the table, because
I won't necessarily lay out each section. So this is why I allocate it here and
grow it as/if necessary. Seems like the simplest approach and not too expensive.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:658: // if we can repeat
it in each page/column.
On 2017/05/08 at 13:55:58, mstensho wrote:
> I must say that I'm still a bit concerned about this. We're performing layout
before knowing the position. But maybe it's okay. Having break-inside:avoid,
we're only going to break inside this section if it's taller than the height of
one page, or if there are forced breaks inside. That would disqualify it from
becoming repeated, I suppose, since its logical height will be taller than that
of one page.
> 
> Since I cannot think of anything in particular that could go wrong, I suppose
I just have to hold my peace. :)

Great. :)

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:256: void
SetPositionOfRepeatingFooterGroupOnPage(int page, LayoutUnit position) {
On 2017/05/08 at 13:55:59, mstensho wrote:
> Could you document here that |page| is the page number relatively to the page
where the table starts - NOT the overall page number?

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:257:
position_of_repeating_footer_group_on_page_.Grow(page + 1);
On 2017/05/08 at 13:55:59, mstensho wrote:
> Grow() requires that you're actually requesting growth, and it seems that
there's no guarantee that page+1 is larger than the current size of the vector,
since nobody resets it at the beginning of layout, so that a subsequent layout
is bound to hit the DCHECK_GE failure in Vector::Grow().

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:569: mutable
Vector<LayoutUnit> position_of_repeating_footer_group_on_page_;
On 2017/05/08 at 13:55:59, mstensho wrote:
> Are you sure you shouldn't use <int> here? Tables don't really deal a lot with
subpixels. Also, this should probably be a typedef, since you need it for
PositionOfRepeatingFooterGroupOnPage() as well.

The row's logical top is stored as a LayoutUnit so it makes sense to me to store
it without converting it.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1257:
LayoutTableSection* footer = Table()->Footer();
On 2017/05/08 at 13:55:59, mstensho wrote:
> This one can be const, I guess?

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1258: bool
footer_wants_to_repeat =
On 2017/05/08 at 13:55:59, mstensho wrote:
> make_room_for_repeating_footer?

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1260:
footer->IsRepeatingFooterGroup(const_cast<LayoutTableSection*>(this)) &&
On 2017/05/08 at 13:55:59, mstensho wrote:
> Ugh. But is there really any reason not to let the parameter be const?
IsRepeatingFooterGroup() only seems to call PageLogicalHeightForOffset() and
LogicalTop() on the object passed.

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1277: 
On 2017/05/08 at 13:55:59, mstensho wrote:
> Back out this?

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2075:
LayoutTableSection* footer = Table()->Footer();
On 2017/05/08 at 13:55:59, mstensho wrote:
> Can you push it a few lines down, next to where it's actually used?

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2109: if (footer &&
footer != this && header != this &&
On 2017/05/08 at 13:55:59, mstensho wrote:
> If the header isn't repeating, there should be no reason to exclude it here?
> 
> Also, it seems kind of harder than necessary to exercise this code. You either
need a repeating header to help, or table rows with break-inside:avoid. If
there's neither, we don't get repeated footers.
> 
> Consider this:
> 
> I'd expect the footer to be repeated here:
> 
> <!DOCTYPE html>
> <p>There should be a blue <em>square</em> below.</p>
> <div style="columns:2; width:20px; column-gap:0; column-fill:auto;
height:90px; line-height:20px;">
>     <table style="width:100%;" cellspacing="0" cellpadding="0">
>         <tbody>
>             <tr>
>                 <td><br></td>
>             </tr>
>             <tr>
>                 <td><br></td>
>             </tr>
>             <tr>
>                 <td><br></td>
>             </tr>
>             <tr>
>                 <td><br></td>
>             </tr>
>             <tr>
>                 <td><br></td>
>             </tr>
>             <tr>
>                 <td><br></td>
>             </tr>
>         </tbody>
>         <tfoot style="break-inside:avoid;">
>             <tr>
>                 <td style="background:blue;">
>                     <br>
>                 </td>
>             </tr>
>         </tfoot>
>     </table>
> </div>

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2114:
FloorToInt(offset_from_top / PageLogicalHeightForOffset(LayoutUnit()));
On 2017/05/08 at 13:55:59, mstensho wrote:
> To cope with non-uniform page heights, you should consider passing the correct
offset to PageLogicalHeightForOffset().

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2116:
row_object.LogicalTop(), LayoutBlock::kAssociateWithLatterPage);
On 2017/05/08 at 13:55:59, mstensho wrote:
> LayoutBlock:: scope should be unnecessary. This one is defined in LayoutBox.

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2141: bool
LayoutTableSection::IsRepeatingFooterGroup(
On 2017/05/08 at 13:55:59, mstensho wrote:
> This belongs AFTER IsRepeatingHeaderGroup().

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2146: //
TODO(rhogan): Sections can be self-painting.
On 2017/05/08 at 13:55:59, mstensho wrote:
> Can you tell me how they can be self-painting?

Yes, if you put an opacity: 0.5 on them for example. See
fast/table/backgr_layers-opacity.html for example.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2151: ?
section_for_page->PageLogicalHeightForOffset(
On 2017/05/08 at 13:55:59, mstensho wrote:
> This is to avoid assertion failures and the bugs that follow? (since you can't
call PageLogicalHeightForOffset() for any other object than the one you're
currently laying out)

Correct.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2157: if
(LogicalHeight() > 0 && page_height / LogicalHeight() < 4)
On 2017/05/08 at 13:55:59, mstensho wrote:
> The code looks fine, but I'd like a comment that mumbles something about about
25%...

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2175: if
(LogicalHeight() > 0 && page_height / LogicalHeight() < 4)
On 2017/05/08 at 13:55:59, mstensho wrote:
> The code looks fine, but I'd like a comment that mumbles something about
25%...

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2213: return
Table()->MapToVisualRectInAncestorSpaceInternal(
On 2017/05/08 at 13:55:59, mstensho wrote:
> How can we just skip one ancestor in the chain like this and ignore the
location of the table section?

I've updated the comment to explain this. For both headers and footers we really
need to return the table quads because the headers/footers are painted
throughout it. That's what this achieves. The alternative would be to dive
further into LayoutBox::MapToVisualRectInAncestorSpaceInternal and special case
repeating footers to make sure the top/left and top/right points are at least
above the first place we paint a footer. This wouldn't be the top of the table
obviously, so the quads we come up with here are overgenerous for sure.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:354:
LayoutTableSection* section_for_page = nullptr) const;
On 2017/05/08 at 13:56:00, mstensho wrote:
> This parameter needs some documentation.

Done in the definition.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:36: LayoutUnit
first_row_offset;
On 2017/05/08 at 13:56:00, mstensho wrote:
> Could you make "pagination" or at least "strut" part of that variable name?
> 
> What does this have to do with repeated footers (this CL), BTW?

This isn't necessary anymore for some reason. ¯\_(ツ)_/¯

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:83: // Include the
top captions as we need top_offset to be
On 2017/05/08 at 13:56:00, mstensho wrote:
> Include, as in exclude from subtraction? How about "Keep"? :)
> 
> OK, I think I want something other than bool to express this.
> 
> How about enum class CaptionSelector { TopCaptionsOnly, BottomCaptionsOnly,
AllCaptions } (should probably remove TopCaptionsOnly, though, since it's not
used)

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:99:
table->GetPositionOfRepeatingFooterGroupOnPage(page).ToInt();
On 2017/05/08 at 13:56:00, mstensho wrote:
> Since the whole vector has already been made accessible via
table->PositionOfRepeatingFooterGroupOnPage() anyway, why not use it directly?
Alternatively, maybe remove PositionOfRepeatingFooterGroupOnPage(), and add
whatever functionality is required to encapsulate the vector completely.

Done.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:100: if
(!footer_position)
On 2017/05/08 at 13:56:00, mstensho wrote:
> When is it 0?

We don't repeat the footer on every page. For example it might not fit on the
first page, e.g.
fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid.html

[commit-bot: I haz the power, 2017-05-09 19:57:33]

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-09 20:23:12]

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

[commit-bot: I haz the power, 2017-05-09 20:23:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mstensho (USE GERRIT), 2017-05-10 10:59:04]

I'm still not through the tests.

Also, I'd like you to break the changes for repeated headers out as a separate
CL. Adding support for repeated footers is task heavy enough on its own.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html
(right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html:19:
</tr>
On 2017/05/09 19:57:15, rhogan wrote:
> On 2017/05/08 at 13:55:58, mstensho wrote:
> > Why changes here? No repeating footers here.
> 
> This is so that the header is < 25% of the table.

If it's not feasible to break out this change from this CL (I'd prefer that),
please mention this change in the commit comment.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:72:
position_of_repeating_footer_group_on_page_.Fill(LayoutUnit(), 1);
On 2017/05/09 19:57:15, rhogan wrote:
> On 2017/05/08 at 13:55:59, mstensho wrote:
> > Seems unnecessary if we're not paginating.
> 
> I'm not sure I can zap this and rebuild it each time I layout the table,
because
> I won't necessarily lay out each section. So this is why I allocate it here
and
> grow it as/if necessary. Seems like the simplest approach and not too
expensive.

How about moving initialization to LayoutTableSection::UpdateLayout() or
something? You already populate the vector from LayoutTableSection. Probably a
good idea to shrink the vector size to fit when done with layout too, so that we
don't keep unused memory around.

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:658: // if we can repeat
it in each page/column.
On 2017/05/09 19:57:15, rhogan wrote:
> On 2017/05/08 at 13:55:58, mstensho wrote:
> > I must say that I'm still a bit concerned about this. We're performing
layout
> before knowing the position. But maybe it's okay. Having break-inside:avoid,
> we're only going to break inside this section if it's taller than the height
of
> one page, or if there are forced breaks inside. That would disqualify it from
> becoming repeated, I suppose, since its logical height will be taller than
that
> of one page.
> > 
> > Since I cannot think of anything in particular that could go wrong, I
suppose
> I just have to hold my peace. :)
> 
> Great. :)

But, while I think it's safe, could you please remark in the comment that we're
actually laying out objects out-of-order here?

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2146: //
TODO(rhogan): Sections can be self-painting.
On 2017/05/09 19:57:16, rhogan wrote:
> On 2017/05/08 at 13:55:59, mstensho wrote:
> > Can you tell me how they can be self-painting?
> 
> Yes, if you put an opacity: 0.5 on them for example. See
> fast/table/backgr_layers-opacity.html for example.

Ah, thanks!

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-nested-repeating-tfoot.html
(right):

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-nested-repeating-tfoot.html:1:
<!DOCTYPE html>
This test lacks an expectation. I assume you want it to be a reftest?

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-2.html
(right):

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-2.html:15:
<div style="columns:3; line-height: 18px; column-fill: auto; height:105px;
background-color: yellow;">
One more instance of the new quarter rule for repeating headers, I guess. Yeah,
I really think you should break this out as a separate CL.

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-2.html
(left):

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-2.html:7:
background-color: #ddd;
This test lacks an expectation. I assume you want it to be a reftest?

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:266: unsigned
PagesWithRepeatingFooters() const {
Could you squeeze "Count" (or some synonym) into that method name?

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

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2186:
section_for_page->LogicalTop())
First of all, you should probably pass LayoutUnit() here, since the logical
offset parameter in PageLogicalHeightForOffset() is relative to |this|.

It's actually still wrong, though, since you really need the logical offset that
maps to the exact column that you're currently considering. That's only an issue
when we have non-uniform column heights, though (e.g. when printing a multicol
container that crosses page boundaries). I don't think it's super-important to
handle this. Repeated headers and footers are first and foremost useful when
paginating, and not so much inside multicol.

But since this won't work correctly anyway, wouldn't it be better to calculate
(at the beginning of layout) and store a bool is_repeating_footer_group_, so
that we give the same answer every time? The way it works right now, the answer
depends on whether section_for_page is specified or not. It is specified during
layout, but not during painting. I can't see why we'd want this discrepancy.

https://codereview.chromium.org/2584143003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2187: :
PageLogicalHeightForOffset(LogicalTop());
Ditto - LayoutUnit() is better. Getting rid of this code altogether would be
even better. :)

[rhogan, 2017-05-31 21:26:02]

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

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 22:39:02]

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

[commit-bot: I haz the power, 2017-05-31 22:39:03]

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-06-13 21:34:38]

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

[commit-bot: I haz the power, 2017-06-13 21:35:28]

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-13 23:53:25]

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

[commit-bot: I haz the power, 2017-06-13 23:53:27]

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

[rhogan, 2017-06-14 21:42:17]

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

[commit-bot: I haz the power, 2017-06-14 21:42:37]

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-14 23:46:01]

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

[commit-bot: I haz the power, 2017-06-14 23:46:03]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-06-15 17:37:20]

On 2017/05/10 at 10:59:04, mstensho wrote:
> I'm still not through the tests.
> 
> Also, I'd like you to break the changes for repeated headers out as a separate
CL. Adding support for repeated footers is task heavy enough on its own.
> 

Can you take a look? I've moved back to mimicking Edge by placing the footer at
the bottom of each page because it seemed easier to do that and deal with cells
straddle multiple pages). Still a couple of things to work out (I need to
account for border spacing correctly, I'm not doing that right now) but I've
looked at this as much as I can without further inspiration!

[mstensho (USE GERRIT), 2017-06-16 08:43:06]

Is this the inspiration you were looking for? :)

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

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1142:
remaining_logical_height - layout_state->HeightOffsetForTableFooters();
Isn't this something that belongs inside PageRemainingLogicalHeightForOffset()?
Who would want to ignore footers?

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutState.h (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutState.h:134: // The offset we need
to include when placing repeating table footers in
Would it make sense to make the text here more similar to the one documenting
height_offset_for_table_headers_?

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:663:
state.SetHeightOffsetForTableFooters(offset_for_table_footers);
What if we delay setting this until right before laying out the bodies? Then we
could get rid of the temporary resetting below, couldn't we?

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:716:
state.SetHeightOffsetForTableFooters(LayoutUnit());
No need to temporarily reset this, if you just move it a bit further up. We're
going to set it to LayoutUnit() unconditionally below anyway.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:723:
state.SetHeightOffsetForTableHeaders(LayoutUnit());
I don't think I understand why we need to reset this here. I realize that
LayoutRows() won't do much work if the table crosses fragmentainer boundaries
(and it's only then that we need to worry about repeated headers and footers).
So it won't make much of a difference. But do we HAVE TO reset it?

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:216:
DCHECK(!NeedsSectionRecalc());
Better leave this alone in this CL. I don't even know if it's possible to do
this yet.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1230: if
(row_logical_height > page_logical_height) {
No need for the curls.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1234: LayoutState*
layout_state = View()->GetLayoutState();
Please move it inside the "if (make_room_for_repeating_footer)" block. Might as
well do View()->GetLayoutState()->HeightOffsetForTableFooters() directly there,
unless the code formatter is going to turn that into soup.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1959: if
(GetPaginationBreakability() == kAllowAnyBreaks)
I wonder if you should add a DCHECK at the beginning of this method, to document
that we only expect headers and footers here.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2000: // Repeating
table headers and footers are painted once per fragmentation
Instead of "fragmentation page/column", I suggest "fragmentainer" or
"page/column".

[rhogan, 2017-06-20 18:46:02]

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

[rhogan, 2017-06-20 18:46:06]

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

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1142:
remaining_logical_height - layout_state->HeightOffsetForTableFooters();
On 2017/06/16 at 08:43:05, mstensho wrote:
> Isn't this something that belongs inside
PageRemainingLogicalHeightForOffset()? Who would want to ignore footers?

I've explained why I think this is the way to do it in a new comment.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutState.h (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutState.h:134: // The offset we need
to include when placing repeating table footers in
On 2017/06/16 at 08:43:05, mstensho wrote:
> Would it make sense to make the text here more similar to the one documenting
height_offset_for_table_headers_?

OK.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:663:
state.SetHeightOffsetForTableFooters(offset_for_table_footers);
On 2017/06/16 at 08:43:06, mstensho wrote:
> What if we delay setting this until right before laying out the bodies? Then
we could get rid of the temporary resetting below, couldn't we?

Yup, reorganized this.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:716:
state.SetHeightOffsetForTableFooters(LayoutUnit());
On 2017/06/16 at 08:43:06, mstensho wrote:
> No need to temporarily reset this, if you just move it a bit further up. We're
going to set it to LayoutUnit() unconditionally below anyway.

ditto.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.cpp:723:
state.SetHeightOffsetForTableHeaders(LayoutUnit());
On 2017/06/16 at 08:43:06, mstensho wrote:
> I don't think I understand why we need to reset this here. I realize that
LayoutRows() won't do much work if the table crosses fragmentainer boundaries
(and it's only then that we need to worry about repeated headers and footers).
So it won't make much of a difference. But do we HAVE TO reset it?

We don't want layout of the footer sections or captions to know about either of
these which is why we reset them.
single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-4.html
is a test that covers the need to reset the header value,
single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-2.html
(among quite a few) that covers the footer case.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTable.h (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTable.h:216:
DCHECK(!NeedsSectionRecalc());
On 2017/06/16 at 08:43:06, mstensho wrote:
> Better leave this alone in this CL. I don't even know if it's possible to do
this yet.

OK.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1230: if
(row_logical_height > page_logical_height) {
On 2017/06/16 at 08:43:06, mstensho wrote:
> No need for the curls.

OK.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1234: LayoutState*
layout_state = View()->GetLayoutState();
On 2017/06/16 at 08:43:06, mstensho wrote:
> Please move it inside the "if (make_room_for_repeating_footer)" block. Might
as well do View()->GetLayoutState()->HeightOffsetForTableFooters() directly
there, unless the code formatter is going to turn that into soup.

Sure.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1959: if
(GetPaginationBreakability() == kAllowAnyBreaks)
On 2017/06/16 at 08:43:06, mstensho wrote:
> I wonder if you should add a DCHECK at the beginning of this method, to
document that we only expect headers and footers here.

Done.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2000: // Repeating
table headers and footers are painted once per fragmentation
On 2017/06/16 at 08:43:06, mstensho wrote:
> Instead of "fragmentation page/column", I suggest "fragmentainer" or
"page/column".

Indeed.

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2014: ancestor,
transform_state, flags);
This is the place which used to trigger the DCHECK I removed above. Pretty sure
it's safe to remove now.

[commit-bot: I haz the power, 2017-06-20 18:46:27]

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-20 20:59:54]

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

[commit-bot: I haz the power, 2017-06-20 20:59:55]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-06-20 21:39:05]

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2014: ancestor,
transform_state, flags);
On 2017/06/20 18:46:06, rhogan wrote:
> This is the place which used to trigger the DCHECK I removed above. Pretty
sure
> it's safe to remove now.

I see no DCHECK removed.

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

https://codereview.chromium.org/2584143003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1143: // full
remaining logical height as our strut to the next page.
But why wouldn't you want to do the same for other types of content that we
don't want to break inside? Why is it limited to lines? What about blocks with
break-inside:avoid? They should also steer clear of repeatables, no?

[rhogan, 2017-06-22 18:08:25]

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

[commit-bot: I haz the power, 2017-06-22 18:08:45]

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-22 18:12:21]

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

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

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[rhogan, 2017-06-23 18:26:12]

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

[commit-bot: I haz the power, 2017-06-23 18:26:30]

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-23 19:42:13]

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

[commit-bot: I haz the power, 2017-06-23 19:42:14]

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-07-01 16:40:42]

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

[commit-bot: I haz the power, 2017-07-01 16:40:48]

Dry run: CQ is trying da patch.

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

[rhogan, 2017-07-01 16:41:16]

On 2017/06/20 at 21:39:05, mstensho wrote:
>
https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):
> 
>
https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2014: ancestor,
transform_state, flags);
> On 2017/06/20 18:46:06, rhogan wrote:
> > This is the place which used to trigger the DCHECK I removed above. Pretty
sure
> > it's safe to remove now.
> 
> I see no DCHECK removed.
That's because I reinstated it per your request. :)

> 
>
https://codereview.chromium.org/2584143003/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/2584143003/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1143: // full
remaining logical height as our strut to the next page.
> But why wouldn't you want to do the same for other types of content that we
don't want to break inside? Why is it limited to lines? What about blocks with
break-inside:avoid? They should also steer clear of repeatables, no?

True - added a new test, and also had to update the CL to cope with
wangxianzhu's recent changes in border painting.

Can you take another look? I've been slow to apply your feedback but definitely
want to get this one out of the way now. :)

[commit-bot: I haz the power, 2017-07-01 18:25:39]

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

[commit-bot: I haz the power, 2017-07-01 18:25:41]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-07-17 10:38:08]

Just got back from vacation.

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

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1256: if
(remaining_logical_height_including_footer >= child_logical_height)
Why can't PageRemainingLogicalHeightForOffset() do this job? Look at all the
call sites for PageRemainingLogicalHeightForOffset(). And we only care about
footers at two of these sites?

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5879:
DCHECK_EQ(strut_to_next_page, PageRemainingLogicalHeightForOffset(
I think this one is worth keeping.

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1950: bool
LayoutTableSection::HeaderGroupShouldRepeat() const {
Maybe these could/should be inline? It's a one-liner, if you write it like this:

return Table()->Header() == this && GroupShouldRepeat();

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp (right):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp:91: // the
before border separately. This will double-paint the top border of the
That double-painting doesn't seem like a good thing to do? Maybe add some paint
team reviewer.

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp:95: bool
cell_is_top_of_repeating_footer =
Sounds like a rare case, and a lot of other conditions need to be true before we
even need to evaluate this. Maybe move the check inside the following if-body.

[rhogan, 2017-07-22 09:53:40]

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

[commit-bot: I haz the power, 2017-07-22 09:53:47]

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-07-22 11:35:57]

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

[commit-bot: I haz the power, 2017-07-22 11:35:59]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-07-24 18:56:08]

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

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1256: if
(remaining_logical_height_including_footer >= child_logical_height)
On 2017/07/17 at 10:38:08, mstensho wrote:
> Why can't PageRemainingLogicalHeightForOffset() do this job? Look at all the
call sites for PageRemainingLogicalHeightForOffset(). And we only care about
footers at two of these sites?

Because, as well as deciding whether the line or block fits on the page,
PageRemainingLogicalHeightForOffset() is used to figure out the strut we need to
apply to reach the top of the next page. When we have a footer it can be used
for the latter but not the former - we need to adjust its value by the height of
the footer in order to that.

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (left):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5879:
DCHECK_EQ(strut_to_next_page, PageRemainingLogicalHeightForOffset(
On 2017/07/17 at 10:38:08, mstensho wrote:
> I think this one is worth keeping.

Done. This was accidental as you can probably guess!

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1950: bool
LayoutTableSection::HeaderGroupShouldRepeat() const {
On 2017/07/17 at 10:38:08, mstensho wrote:
> Maybe these could/should be inline? It's a one-liner, if you write it like
this:
> 
> return Table()->Header() == this && GroupShouldRepeat();

Done.

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp (right):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp:91: // the
before border separately. This will double-paint the top border of the
On 2017/07/17 at 10:38:08, mstensho wrote:
> That double-painting doesn't seem like a good thing to do? Maybe add some
paint team reviewer.

Will do. wangxianzhu would be ideal but he's off for a while. I think the double
painting is unavoidable for now due to the new way we paint collapsed borders.

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp:95: bool
cell_is_top_of_repeating_footer =
On 2017/07/17 at 10:38:08, mstensho wrote:
> Sounds like a rare case, and a lot of other conditions need to be true before
we even need to evaluate this. Maybe move the check inside the following
if-body.

Done.

[mstensho (USE GERRIT), 2017-07-25 08:59:32]

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

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1256: if
(remaining_logical_height_including_footer >= child_logical_height)
On 2017/07/24 18:56:06, rhogan wrote:
> On 2017/07/17 at 10:38:08, mstensho wrote:
> > Why can't PageRemainingLogicalHeightForOffset() do this job? Look at all the
> call sites for PageRemainingLogicalHeightForOffset(). And we only care about
> footers at two of these sites?
> 
> Because, as well as deciding whether the line or block fits on the page,
> PageRemainingLogicalHeightForOffset() is used to figure out the strut we need
to
> apply to reach the top of the next page. When we have a footer it can be used
> for the latter but not the former - we need to adjust its value by the height
of
> the footer in order to that.

Ah, true.

PageRemainingLogicalHeightForOffset() should definitely take footers into
account. Otherwise it's not really answering how much space we have left.

Maybe CalculatePaginationStrutToFitContent() should always be used to calculate
the strut. Then it probably shouldn't take this |strut_to_next_page| parameter
anymore, since it's going to be impossible for the caller to calculate it.

How about this: https://chromium-review.googlesource.com/c/583619/

Then, afterwards, you can fix PageRemainingLogicalHeightForOffset() and
CalculatePaginationStrutToFitContent() to deal correctly with footers?

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp (right):

https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp:91: // the
before border separately. This will double-paint the top border of the
On 2017/07/24 18:56:07, rhogan wrote:
> On 2017/07/17 at 10:38:08, mstensho wrote:
> > That double-painting doesn't seem like a good thing to do? Maybe add some
> paint team reviewer.
> 
> Will do. wangxianzhu would be ideal but he's off for a while. I think the
double
> painting is unavoidable for now due to the new way we paint collapsed borders.

OK. A TODO, then.

[rhogan, 2017-07-30 11:41:57]

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

[commit-bot: I haz the power, 2017-07-30 11:42:08]

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-07-30 11:45:37]

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

[commit-bot: I haz the power, 2017-07-30 11:45:38]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[rhogan, 2017-07-31 17:47:36]

On 2017/07/25 at 08:59:32, mstensho wrote:
> How about this: https://chromium-review.googlesource.com/c/583619/
> 
> Then, afterwards, you can fix PageRemainingLogicalHeightForOffset() and
CalculatePaginationStrutToFitContent() to deal correctly with footers?
> 
Done, thanks.

>
https://codereview.chromium.org/2584143003/diff/260001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp (right):
> 
> > Will do. wangxianzhu would be ideal but he's off for a while. I think the
double
> > painting is unavoidable for now due to the new way we paint collapsed
borders.
> 
> OK. A TODO, then.

Done, thanks. (In next CL I upload to fix merge issues.)

[rhogan, 2017-07-31 20:48:06]

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

[commit-bot: I haz the power, 2017-07-31 20:48: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-07-31 22:06:48]

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

[commit-bot: I haz the power, 2017-07-31 22:06:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[rhogan, 2017-08-01 06:47:42]

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

[commit-bot: I haz the power, 2017-08-01 06:47:46]

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-08-01 08:44:57]

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

[commit-bot: I haz the power, 2017-08-01 08:44:59]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-08-01 18:36:05]

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

[commit-bot: I haz the power, 2017-08-01 18:36:23]

Dry run: CQ is trying da patch.

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

[rhogan, 2017-08-01 18:46:16]

On 2017/07/31 at 17:47:36, rhogan wrote:
> On 2017/07/25 at 08:59:32, mstensho wrote:
> > OK. A TODO, then.
> 
> Done, thanks. (In next CL I upload to fix merge issues.)

OK to take another look?

[mstensho (USE GERRIT), 2017-08-01 18:58:19]

Sorry about the delay, and thanks for the ping! Forgot to sweep through my list
here.

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

https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1154: // We need to
detect if we overlap a repeating footer and if so take the
Probably don't want this comment anymore.

https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5849: return
remaining_height;
Need to subtract it here too, I guess, or it's not gonna work with printing.

https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5854:
View()->GetLayoutState()->HeightOffsetForTableFooters();
Should we DCHECK that the result isn't negative?

[commit-bot: I haz the power, 2017-08-01 21:07:27]

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

[commit-bot: I haz the power, 2017-08-01 21:07:29]

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

[rhogan, 2017-08-04 07:47:59]

On 2017/08/01 at 18:58:19, mstensho wrote:
>
https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1154: // We need to
detect if we overlap a repeating footer and if so take the
> Probably don't want this comment anymore.
Done.

>
https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:5849: return
remaining_height;
> Need to subtract it here too, I guess, or it's not gonna work with printing.
:/. Done. Added a couple of printing tests too.

> 
>
https://codereview.chromium.org/2584143003/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:5854:
View()->GetLayoutState()->HeightOffsetForTableFooters();
> Should we DCHECK that the result isn't negative?
I need to keep the result negative because I'm netting the footer offset back
into the result in CalculatePaginationStrutForContent.

[mstensho (USE GERRIT), 2017-08-04 08:03:45]

lgtm

[rhogan, 2017-08-04 08:27:18]

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

[commit-bot: I haz the power, 2017-08-04 08:27:27]

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-08-04 09:28:57]

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

[commit-bot: I haz the power, 2017-08-04 09:28:59]

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

[rhogan, 2017-08-05 14:26:42]

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

[commit-bot: I haz the power, 2017-08-05 14:26:58]

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-08-05 16:07:49]

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

[commit-bot: I haz the power, 2017-08-05 16:07:51]

Dry run: This issue passed the CQ dry run.

[rhogan, 2017-08-05 16:38:13]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-08-05 16:38:14]

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/2584143003/#ps420001
(title: "bug 656232")

[commit-bot: I haz the power, 2017-08-05 16:38:20]

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-08-05 16:43:58]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1501951093246890,
"parent_rev": "d9e90afec475d912cca91cd0b42e7b3a564fff59", "commit_rev":
"a098f68e0de0ba6a40f2524e0f4b437f76c6e9a3"}

[commit-bot: I haz the power, 2017-08-05 16:44:23]

Description was changed from

==========
Repeat footers in paginated context

BUG=656232,620223

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

to

==========
Repeat footers in paginated context

BUG=656232,620223

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-08-05 16:44:25]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/a098f68e0de0ba6a40f2524e0f4b...