Display table header groups at the top of each page

Display table header groups at the top of each page

FF, IE and Edge all repeat a table header group at the top of each printed page.

This area is underspecified but after some discussion in the bug we agreed to
repeat the header group if it has break-inside:avoid. We make this the default
style for theads when printing.

BUG=24826
Committed: https://crrev.com/81c0fc6d4e08e4e2bb4eb8a42747f21b13303616
Cr-Commit-Position: refs/heads/master@{#397915}

[rhogan, 2016-05-29 11:45:31]

Description was changed from

==========
Display table header groups at the top of each page

BUG=24826
==========

to

==========
Display table header groups at the top of each page

FF, IE and Edge all repeat a table header group at the top of each printed page.

This area is underspecified but after some discussion in the bug we agreed to
repeat the header group if it has break-inside:avoid. We make this the default
style for theads when printing.

BUG=24826
==========

[rhogan, 2016-05-29 12:20:13]

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

[rhogan, 2016-05-30 18:12:30]

@wangxianzhu: can you take a look at what I'm doing with the painting? Does it
look right to you?

Not sure who can cover the pagination/table part of the change in mstensho's
absence. @eae: any thoughts?

[Xianzhu, 2016-05-31 02:11:34]

I can review the paint/ part. I'm not familiar with pagination strut in
LayoutTableSection.

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

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:21: void
TablePainter::paintRepeatingHeaderGroup(const PaintInfo& paintInfo, const
LayoutPoint& paintOffset, LayoutBox* child, const CollapsedBorderValue&
currentBorderValue, ItemToPaint itemToPaint)
Pass paintInfoForDescendants from caller, then you can avoid
paintInfo.forDescendants()) in this function.

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:25: if
(child->getPaginationBreakability() == LayoutTableBoxComponent::AllowAnyBreaks)
Use LayoutBox::AllowAnyBreaks.

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:44: }
We paint each page as a layer fragment in PaintLayerPainter in a loop, so the
above loop is redundant. Here you should paint for one page only.

[eae, 2016-05-31 18:00:08]

LGTM for layout/

[rhogan, 2016-06-01 18:39:46]

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

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:44: }
On 2016/05/31 at 02:11:34, Xianzhu wrote:
> We paint each page as a layer fragment in PaintLayerPainter in a loop, so the
above loop is redundant. Here you should paint for one page only.

Right, but table headers don't have a layer - so the nearest object that will
paint a layer is the flow thread and it won't be able to inspect for table
header groups when collecting fragments. So this is the only place I can think
of for repeating the table headers across each page.

Maybe I'm missing something?

[Xianzhu, 2016-06-01 19:19:39]

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

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:21: void
TablePainter::paintRepeatingHeaderGroup(const PaintInfo& paintInfo, const
LayoutPoint& paintOffset, LayoutBox* child, const CollapsedBorderValue&
currentBorderValue, ItemToPaint itemToPaint)
s/LayoutBox* child/const LayoutTableSection& headerSection/

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:39:
TableSectionPainter(*section).paintCollapsedBorders(paintInfo.forDescendants(),
childPoint, currentBorderValue);
Move the common code (LayoutTableSection* section = ...; TableSectionPainter
sectionPainter(*section)) out of the loop.

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:44: }
On 2016/06/01 18:39:46, rhogan wrote:
> On 2016/05/31 at 02:11:34, Xianzhu wrote:
> > We paint each page as a layer fragment in PaintLayerPainter in a loop, so
the
> above loop is redundant. Here you should paint for one page only.
> 
> Right, but table headers don't have a layer - so the nearest object that will
> paint a layer is the flow thread and it won't be able to inspect for table
> header groups when collecting fragments. So this is the only place I can think
> of for repeating the table headers across each page.
> 
> Maybe I'm missing something?

I think this is the right place (and TableSectionPainter is more appropriate,
see comments below).

My comment was about performance. In the above loop, normally only 1 iteration
will actually paint into the cull rect of paintInfo because the cull rect is for
one page/fragment.

However, to make the code not strictly coupled to PaintLayerPainter, you can
keep the loop, but with modified starting and ending value according to the cull
rect to avoid unnecessary iterations.

https://codereview.chromium.org/2021703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePainter.cpp:69:
paintRepeatingHeaderGroup(paintInfo, paintOffset, toLayoutBox(child),
CollapsedBorderValue(), PaintChild);
This should be in TableSectionPainter::paint(). With this patch you'll miss the
case that the header section has self-painting layer. It may look like:

   TableSectionPainter::paint()
   {
       orignalPaint();
       if (m_layoutTableSection == m_layoutTableSection.table()->header())
           paintRepeatingHeaderGroup();
   }


(Fwiw, TableSectionPainter::paint has two callsites:
1. From TablePainter if the section has no self-painting layer;
2. From PaintLayerPainter if the section has a self-painting layer.)

[rhogan, 2016-06-02 21:42:50]

On 2016/06/01 at 19:19:39, wangxianzhu wrote:
> 
> However, to make the code not strictly coupled to PaintLayerPainter, you can
keep the loop, but with modified starting and ending value according to the cull
rect to avoid unnecessary iterations.

Applied your comments - in the way that seemed best to me!

[Xianzhu, 2016-06-02 23:03:53]

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

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:42: //
TODO(rhogan): We don't paint a header repeatedly if it's self-painting.
Nit: This sentence actually describes a bug, but gives me a sense that we should
do it like this. Please describe why we are doing this and what we should do
instead.

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:43: if
(m_layoutTableSection.layer() &&
m_layoutTableSection.layer()->isSelfPaintingLayer())
Use hasSelfPaintingLayer().

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:45: LayoutUnit
printedPageLogicalHeight = m_layoutTableSection.view() ?
m_layoutTableSection.view()->pageLogicalHeight() : LayoutUnit();
Nit: During painting, we can be sure that any layout object has a view, so you
can omit the check.

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:47: LayoutUnit
pageHeight = std::max(printedPageLogicalHeight,
table->flowThreadContainingBlock() ?
table->pageLogicalHeightForOffset(LayoutUnit()) : LayoutUnit());
Can you try
  LayoutUnit pageHeight = table->pageLogicalHeightForOffset(LayoutUnit());
instead of line 45-47

after modifying LayoutBox::pageLogicalHeightForOffset() not to depend on
layoutState()?

I think during layout, view()->layoutState()->pageLogicalHeight should equal
view()->pageLogicalHeight(), but the latter is also available after layout.

You can create another patch for LayoutBox::pageLogicalHeightForOffset() and let
eae@ review it, or include it in this patch.

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:54: for (int i = 1;
i < pages; i++) {
You can reduce number of iterations of the loop like the following:

    // Skip pages that are above the cull rect.
    // Just sample. Not sure if it's fully correct.
    if (paintInfo.cullRect().m_rect.y() > paginationOffset.y())
        paginationOffset.move(0, pageHeight *
static_cast<int>((paintInfo.cullRect().m_rect.y() - paginationOffset.y()) /
pageHeight));

    LayoutUnit bottomBound = std::min(paintInfo.cullRect().m_rect.maxY(),
paintOffset.y() + table->logicalHeight());
    while (paginationOffset.y() < bottomBound) {
        ... (intersectsVerticalRange is no longer needed)
    }

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:61:
paginationOffset += LayoutPoint(0, pageHeight);
Nit: (optional) pageinationOffset.move(0, pageHeight);

https://codereview.chromium.org/2021703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:73: void
TableSectionPainter::paintChild(const PaintInfo& paintInfo, const LayoutPoint&
paintOffset)
The name looks not appropriate because it paints the whole section, not "child".
Perhaps "paintSection"?

[rhogan, 2016-06-03 18:35:03]

On 2016/06/02 at 23:03:53, wangxianzhu wrote:

Applied all your comments - very nice feedback!

[rhogan, 2016-06-03 18:36:55]

On 2016/06/03 at 18:35:03, rhogan wrote:
> On 2016/06/02 at 23:03:53, wangxianzhu wrote:
> 
> Applied all your comments - very nice feedback!

eae@ could you take a look at the update to LayoutBox too?

[Xianzhu, 2016-06-03 18:45:41]

paint/ lgtm with several comments.

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

https://codereview.chromium.org/2021703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:59:
paintCollapsedSectionBorders(paintInfo.forDescendants(), paginationOffset,
currentBorderValue);
Remove .forDescendants() because the method is still expecting paintInfo for the
table section.

https://codereview.chromium.org/2021703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:61:
paintSection(paintInfo.forDescendants(), paginationOffset);
Ditto.

https://codereview.chromium.org/2021703002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.h (right):

https://codereview.chromium.org/2021703002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.h:44: PaintChild
Nit: Also rename this to PaintSection.

[eae, 2016-06-03 20:13:32]

OK

LGTM

[rhogan, 2016-06-04 10:29:57]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-06-04 10:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2021703002/#ps150001
(title: "Updated")

[commit-bot: I haz the power, 2016-06-04 10:30:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021703002/150001

[commit-bot: I haz the power, 2016-06-04 11:40:07]

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

[commit-bot: I haz the power, 2016-06-04 11:40:09]

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

[rhogan, 2016-06-04 11:50:11]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-06-04 11:50:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2021703002/150001

[commit-bot: I haz the power, 2016-06-04 12:14:09]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-06-04 12:15:42]

Description was changed from

==========
Display table header groups at the top of each page

FF, IE and Edge all repeat a table header group at the top of each printed page.

This area is underspecified but after some discussion in the bug we agreed to
repeat the header group if it has break-inside:avoid. We make this the default
style for theads when printing.

BUG=24826
==========

to

==========
Display table header groups at the top of each page

FF, IE and Edge all repeat a table header group at the top of each printed page.

This area is underspecified but after some discussion in the bug we agreed to
repeat the header group if it has break-inside:avoid. We make this the default
style for theads when printing.

BUG=24826

Committed: https://crrev.com/81c0fc6d4e08e4e2bb4eb8a42747f21b13303616
Cr-Commit-Position: refs/heads/master@{#397915}
==========

[commit-bot: I haz the power, 2016-06-04 12:15:43]

Patchset 9 (id:??) landed as
https://crrev.com/81c0fc6d4e08e4e2bb4eb8a42747f21b13303616
Cr-Commit-Position: refs/heads/master@{#397915}