Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2584143003: Repeat footers in paginated context

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months ago by rhogan
Modified:
4 days, 18 hours ago
Reviewers:
mstensho
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dglazkov+blink, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Repeat footers in paginated context BUG=656232, 620223 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Patch Set 1 #

Patch Set 2 : bug 656232 #

Patch Set 3 : bug 656232 #

Total comments: 21

Patch Set 4 : bug 656232 #

Patch Set 5 : bug 656232 #

Patch Set 6 : bug 656232 #

Total comments: 54

Patch Set 7 : bug 656232 #

Total comments: 6

Patch Set 8 : bug 656232 #

Patch Set 9 : bug 656232 #

Patch Set 10 : bug 656232 #

Total comments: 22

Patch Set 11 : bug 656232 #

Total comments: 1

Patch Set 12 : bug 656232 #

Patch Set 13 : bug 656232 #

Patch Set 14 : bug 656232 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+1989 lines, -48 lines) Patch
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row.html View 1 2 3 4 5 6 7 1 chunk +36 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-2.html View 1 2 3 4 5 6 7 8 1 chunk +52 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-2-expected.html View 1 2 3 4 5 6 7 8 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-3.html View 1 2 3 1 chunk +52 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-3-expected.html View 1 2 3 4 5 6 7 8 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-4.html View 1 2 3 4 5 6 7 1 chunk +35 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-4-expected.html View 1 2 3 4 5 6 7 8 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-multiple-tables-caption-repeating-thead-tfoot-with-border-spacing-at-top-of-row-expected.html View 1 2 3 4 5 6 7 8 1 chunk +41 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-nested-repeating-tfoot.html View 1 2 3 4 5 6 7 8 1 chunk +68 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-nested-repeating-tfoot-expected.html View 1 2 3 4 5 6 7 8 9 1 chunk +74 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot.html View 1 2 3 4 5 6 7 8 1 chunk +57 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-2.html View 1 2 3 4 5 6 7 8 1 chunk +61 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-2-expected.html View 1 2 3 4 5 6 7 8 1 chunk +69 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-3.html View 1 2 3 4 5 6 7 8 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-3-expected.html View 1 2 3 4 5 6 7 8 1 chunk +60 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-4.html View 1 2 3 4 5 6 7 8 1 chunk +56 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-4-expected.html View 1 2 3 4 5 6 7 8 1 chunk +60 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-nested-repeating-thead-tfoot-expected.html View 1 2 3 4 5 6 7 8 1 chunk +61 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-tfoot-rows-allowing-break.html View 1 2 3 4 5 6 7 8 9 1 chunk +33 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-tfoot-rows-allowing-break-expected.html View 1 2 3 4 5 6 7 8 9 1 chunk +36 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-cell-straddles-page.html View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-cell-straddles-page-expected.html View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -5 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-cell-straddles-page-unsplittable-div.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +50 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-cell-straddles-page-unsplittable-div-expected.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +44 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot.html View 1 2 3 1 chunk +55 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-expected.html View 1 2 3 4 5 6 7 8 1 chunk +58 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page.html View 1 2 3 4 5 6 7 8 1 chunk +51 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid.html View 1 2 3 4 5 6 7 1 chunk +55 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-2.html View 1 2 3 4 5 6 7 8 9 1 chunk +37 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-2-expected.html View 1 2 3 4 5 6 7 8 9 1 chunk +34 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-3.html View 1 2 3 1 chunk +38 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-3-expected.html View 1 2 3 4 5 6 7 8 1 chunk +36 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-break-after-avoid-expected.html View 1 2 3 4 5 6 7 8 1 chunk +49 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-starts-middle-of-page-expected.html View 1 2 3 4 5 6 7 8 1 chunk +50 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-with-border-spacing-at-top-of-row.html View 1 2 3 1 chunk +36 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-with-border-spacing-at-top-of-row-expected.html View 1 2 3 4 5 6 7 8 1 chunk +54 lines, -0 lines 0 comments Download
A + third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-with-caption.html View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -19 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-with-caption-expected.html View 1 2 3 4 5 6 7 8 1 chunk +38 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-with-two-captions.html View 1 2 3 4 5 6 7 8 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-tfoot-with-two-captions-expected.html View 1 2 3 4 5 6 7 8 1 chunk +37 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/html.css View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +9 lines, -2 lines 1 comment Download
M third_party/WebKit/Source/core/layout/LayoutBox.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -2 lines 1 comment Download
M third_party/WebKit/Source/core/layout/LayoutState.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +11 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutState.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTable.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTable.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +22 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +34 lines, -8 lines 1 comment Download
M third_party/WebKit/Source/core/paint/CollapsedBorderPainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +9 lines, -3 lines 2 comments Download
M third_party/WebKit/Source/core/paint/TableSectionPainter.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/TableSectionPainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +77 lines, -4 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 73 (60 generated)
rhogan
HI mstensho - can you have a look when you get the chance?
3 months ago (2017-04-22 11:17:32 UTC) #13
mstensho
On 2017/04/22 11:17:32, rhogan wrote: > HI mstensho - can you have a look when ...
2 months, 4 weeks ago (2017-04-24 09:50:04 UTC) #16
mstensho
Haven't reviewed the tests yet. https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Source/core/layout/LayoutTable.cpp File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Source/core/layout/LayoutTable.cpp#newcode649 third_party/WebKit/Source/core/layout/LayoutTable.cpp:649: // Lay out table ...
2 months, 4 weeks ago (2017-04-24 11:57:17 UTC) #17
rhogan
On 2017/04/24 at 11:57:17, mstensho wrote: > https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Source/core/layout/LayoutTable.cpp > File third_party/WebKit/Source/core/layout/LayoutTable.cpp (right): > > https://codereview.chromium.org/2584143003/diff/40001/third_party/WebKit/Source/core/layout/LayoutTable.cpp#newcode649 ...
2 months, 2 weeks ago (2017-05-06 12:46:22 UTC) #22
mstensho
Still not done with the tests. I think I agree now that individual footer positions ...
2 months, 2 weeks ago (2017-05-08 13:56:00 UTC) #31
rhogan
Thanks mstensho for that super review - addressed all your comments I hope. https://codereview.chromium.org/2584143003/diff/100001/third_party/WebKit/LayoutTests/fast/multicol/balance-repeating-table-headers.html File ...
2 months, 1 week ago (2017-05-09 19:57:17 UTC) #35
mstensho
I'm still not through the tests. Also, I'd like you to break the changes for ...
2 months, 1 week ago (2017-05-10 10:59:04 UTC) #39
rhogan
On 2017/05/10 at 10:59:04, mstensho wrote: > I'm still not through the tests. > > ...
1 month ago (2017-06-15 17:37:20 UTC) #52
mstensho
Is this the inspiration you were looking for? :) https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode1142 third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1142: ...
1 month ago (2017-06-16 08:43:06 UTC) #53
rhogan
https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp#newcode1142 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: ...
1 month ago (2017-06-20 18:46:06 UTC) #55
mstensho
https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode2014 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2014: ancestor, transform_state, flags); On 2017/06/20 18:46:06, rhogan wrote: > ...
1 month ago (2017-06-20 21:39:05 UTC) #59
rhogan
On 2017/06/20 at 21:39:05, mstensho wrote: > https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): > > https://codereview.chromium.org/2584143003/diff/180001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode2014 ...
2 weeks, 6 days ago (2017-07-01 16:41:16 UTC) #70
mstensho
4 days, 18 hours ago (2017-07-17 10:38:08 UTC) #73
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973