Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388613002:
Move below headers even when row doesn't require a strut (Closed)
|
Created:
4 years, 2 months ago by rhogan Modified:
4 years, 2 months ago Reviewers:
mstensho (USE GERRIT) CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove below headers even when row doesn't require a strut
When a row doesn't need a strut to appear at the top of a page, ensure it moves
below any repeating header. When the tables uses 'border-spacing' ensure that
the row is offsetting from the top of the page, as the border-spacing can often
straddle the page-break.
BUG=642814
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/0b78ef8444af3b0eba5aa7adcf932fffbb3233f9
Cr-Commit-Position: refs/heads/master@{#423471}
Patch Set 1 #Patch Set 2 : bug 642814 #
Total comments: 9
Patch Set 3 : bug 642814 #
Total comments: 1
Patch Set 4 : bug 642814 #Patch Set 5 : bug 642814 #
Messages
Total messages: 33 (24 generated)
Description was changed from ========== bug 642814 BUG=642814 ========== to ========== bug 642814 BUG=642814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== bug 642814 BUG=642814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move below headers even when row doesn't require a strut When a row doesn't need a strut to appear at the top of a page, ensure it moves below any repeating header. When the tables uses 'border-spacing' ensure that the row is offsetting from the top of the page, as the border-spacing can often straddle the page-break. BUG=642814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
robhogan@gmail.com changed reviewers: + mstensho@opera.com
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Layo... 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/2388613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-2.html:4: font-size: 12pt; Would 16px work too? https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-2.html:14: <p>crbug.com/624814: A table header group should not repeat on each page if we can't fit at least one content row below it.</p> That doesn't seem to be what this test is testing. It's about unwanted overlap in the second column, right? https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-2.html:15: <div style="-webkit-columns:3; line-height: 18px; column-fill: auto; height:91px; background-color: yellow;"> No need for -webkit- prefix. https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-3.html (right): https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-starts-middle-of-page-break-after-avoid-3.html:15: <p>crbug.com/624814: A table header group should not repeat on each page if we can't fit at least one content row below it.</p> This description too seems wrong. https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1276: // Border spacing from the previous row has pushed this row just past the top of the page, so we must reposition it to the top Should wrap this comment at 80 cols. https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:316: bool needsStrutForTableHeaders(const LayoutState& state, I think I'd non-inline this, and skip the LayoutState parameter. https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:319: (pageRemainingLogicalHeightForOffset( Expression doesn't need to be in parentheses. (Man, this 80 cols limit makes code hard to read sometimes) :( https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:320: logicalOffset, LayoutBlock::AssociateWithLatterPage) == AssociateWithLatterPage is defined in LayoutBox, so no need to prefix it with a scope.
https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.h:319: (pageRemainingLogicalHeightForOffset( On 2016/10/04 at 21:02:38, mstensho wrote: > Expression doesn't need to be in parentheses. > > (Man, this 80 cols limit makes code hard to read sometimes) :( God it's awful.
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
On 2016/10/04 at 21:10:45, rhogan wrote: > https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right): > > https://codereview.chromium.org/2388613002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutTableSection.h:319: (pageRemainingLogicalHeightForOffset( > On 2016/10/04 at 21:02:38, mstensho wrote: > > Expression doesn't need to be in parentheses. > > I've applied your comments. Can you take another look?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2388613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2388613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1108: LayoutBlock::AssociateWithLatterPage) == LayoutBlock:: prefix not necessary.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by robhogan@gmail.com
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/2388613002/#ps60001 (title: "bug 642814")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by robhogan@gmail.com to run a CQ dry run
The CQ bit was unchecked by robhogan@gmail.com
The CQ bit was checked by robhogan@gmail.com
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/2388613002/#ps80001 (title: "bug 642814")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Move below headers even when row doesn't require a strut When a row doesn't need a strut to appear at the top of a page, ensure it moves below any repeating header. When the tables uses 'border-spacing' ensure that the row is offsetting from the top of the page, as the border-spacing can often straddle the page-break. BUG=642814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move below headers even when row doesn't require a strut When a row doesn't need a strut to appear at the top of a page, ensure it moves below any repeating header. When the tables uses 'border-spacing' ensure that the row is offsetting from the top of the page, as the border-spacing can often straddle the page-break. BUG=642814 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0b78ef8444af3b0eba5aa7adcf932fffbb3233f9 Cr-Commit-Position: refs/heads/master@{#423471} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0b78ef8444af3b0eba5aa7adcf932fffbb3233f9 Cr-Commit-Position: refs/heads/master@{#423471} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
