Account for border spacing correctly when repeating header groups

Account for border spacing correctly when repeating header groups

BUG=642814
Committed: https://crrev.com/8fdd7b1b8f735ae842949e69e7b37869abf7c705
Cr-Commit-Position: refs/heads/master@{#427044}

[rhogan, 2016-10-17 15:33:13]

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

[commit-bot: I haz the power, 2016-10-17 15:33:29]

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-10-17 17:05:24]

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

[commit-bot: I haz the power, 2016-10-17 17:05:25]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-10-19 09:33:30]

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

[mstensho (USE GERRIT), 2016-10-19 20:29:16]

Now that https://chromiumcodereview.appspot.com/2433473002 has landed, you need
to rebase this patch, since the code has moved around. Sorry about that.

[rhogan, 2016-10-21 10:56:12]

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

[commit-bot: I haz the power, 2016-10-21 10:56:26]

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-10-21 13:31:05]

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

[commit-bot: I haz the power, 2016-10-21 13:31:05]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-10-22 11:40:13]

On 2016/10/19 at 20:29:16, mstensho wrote:
> Now that https://chromiumcodereview.appspot.com/2433473002 has landed, you
need to rebase this patch, since the code has moved around. Sorry about that.

That's rebased now - can you take another look?

[mstensho (USE GERRIT), 2016-10-24 08:39:35]

Code changes are fine. I'll I'd like to change is that blank line. :) The rest
is better dealt with later, since your CL isn't changing any of that anyway.

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-with-border-spacing-at-top-of-row-expected.html
(right):

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-with-border-spacing-at-top-of-row-expected.html:15:
text-align: -internal-center
This class is unused. Good thing too, since it uses "-internal-center", whatever
that is. :)

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-with-border-spacing-at-top-of-row.html
(right):

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-with-border-spacing-at-top-of-row.html:4:
font-size: 16px;
I don't think we need this.

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-with-border-spacing-at-top-of-row.html:14:
<p>crbug.com/652792: A row positioned naturally at the top of a column should
move below any repeating table header.</p>
Wrong bug number. Can you either fix it or remove it?

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

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1988: if
(state.heightOffsetForTableHeaders()) {
Yeah, we used to check for this before your changes too, but is it right?
Shouldn't we always do this, regardless of having repeated headers or not?

We cannot do this right now though, because that will increase the test coverage
of this code, and we'll crash in some existing tests because we sometimes call
pageRemainingLogicalHeightForOffset() when page height is 0.

Should fix this in a separate CL.

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1991:
pageRemainingLogicalHeightForOffset(rowObject.logicalTop(),
It's not allowed to call pageRemainingLogicalHeightForOffset() if page height is
0 (e.g. in the initial layout pass for multicol with auto column height).

Should fix this in a separate CL.

https://codereview.chromium.org/2422163003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:2018: 
Better delete this blank line, so that it's a bit more obvious that the comment
above also applies to the code below.

[rhogan, 2016-10-24 09:35:42]

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

[rhogan, 2016-10-24 09:36:00]

On 2016/10/24 at 08:39:35, mstensho wrote:
> Code changes are fine. I'll I'd like to change is that blank line. :) The rest
is better dealt with later, since your CL isn't changing any of that anyway.

Updated per your comments!

[commit-bot: I haz the power, 2016-10-24 09:36:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mstensho (USE GERRIT), 2016-10-24 09:39:38]

lgtm

[rhogan, 2016-10-24 10:23:40]

The CQ bit was unchecked by robhogan@gmail.com

[rhogan, 2016-10-24 10:23:43]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-10-24 10:23:55]

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-10-24 11:12:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-24 11:15:43]

Description was changed from

==========
Account for border spacing correctly when repeating header groups

BUG=642814
==========

to

==========
Account for border spacing correctly when repeating header groups

BUG=642814

Committed: https://crrev.com/8fdd7b1b8f735ae842949e69e7b37869abf7c705
Cr-Commit-Position: refs/heads/master@{#427044}
==========

[commit-bot: I haz the power, 2016-10-24 11:15:44]

Patchset 3 (id:??) landed as
https://crrev.com/8fdd7b1b8f735ae842949e69e7b37869abf7c705
Cr-Commit-Position: refs/heads/master@{#427044}