Account for border spacing correctly when repeating header groups

third_party/WebKit/Source/core/layout/LayoutTableSection.cpp

Index: third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
index 25fb70ab90f774205c1b74dae700f9f6112bb61c..cef034176dbc021c948bde730605ed1bd2af24c2 100644
--- a/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp
@@ -1260,14 +1260,6 @@ int LayoutTableSection::paginationStrutForRow(LayoutTableRow* row,
 
   LayoutUnit remainingLogicalHeight = pageRemainingLogicalHeightForOffset(
       logicalOffset, LayoutBlock::AssociateWithLatterPage);
-  LayoutUnit offsetForBorderSpacing =
-      pageLogicalHeight - (remainingLogicalHeight + table()->vBorderSpacing());
-  // 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 of the page and avoid any
-  // repeating header.
-  if (offsetForBorderSpacing < 0)
-    return offsetForBorderSpacing.toInt();
-
   if (remainingLogicalHeight >= rowLogicalHeight)
     return 0;  // It fits fine where it is. No need to break.
   LayoutUnit paginationStrut = calculatePaginationStrutToFitContent(
@@ -1990,12 +1982,18 @@ void LayoutTableSection::adjustRowForPagination(LayoutTableRow& rowObject,
   rowObject.setLogicalHeight(LayoutUnit(logicalHeightForRow(rowObject)));
   int paginationStrut =
       paginationStrutForRow(&rowObject, rowObject.logicalTop());
+  bool rowIsAtTopOfColumn = false;
+  LayoutUnit offsetFromTopOfPage;
   if (!paginationStrut) {
-    bool rowIsAtTopOfColumn =
-        state.heightOffsetForTableHeaders() &&
-        pageLogicalHeightForOffset(rowObject.logicalTop()) ==
-            pageRemainingLogicalHeightForOffset(rowObject.logicalTop(),
-                                                AssociateWithLatterPage);
+    if (state.heightOffsetForTableHeaders()) {

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

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.

+      offsetFromTopOfPage =
+          pageLogicalHeightForOffset(rowObject.logicalTop()) -
+          pageRemainingLogicalHeightForOffset(rowObject.logicalTop(),

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

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.

+                                              AssociateWithLatterPage);
+      rowIsAtTopOfColumn = !offsetFromTopOfPage ||
+                           offsetFromTopOfPage <= table()->vBorderSpacing();
+    }
+
     if (!rowIsAtTopOfColumn)
       return;
   }
@@ -2015,6 +2013,9 @@ void LayoutTableSection::adjustRowForPagination(LayoutTableRow& rowObject,
   }
   // If we have a header group we will paint it at the top of each page,
   // move the rows down to accomodate it.
+  if (rowIsAtTopOfColumn && offsetFromTopOfPage)
+    paginationStrut -= offsetFromTopOfPage.toInt();
+

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

Better delete this blank line, so that it's a bit more obvious that the comment
above also applies to the code below.

   paginationStrut += state.heightOffsetForTableHeaders().toInt();
   rowObject.setPaginationStrut(LayoutUnit(paginationStrut));