Tables with specific merge cell configuration render with extra height to tr elements.

Source/core/rendering/RenderTableSection.cpp

Index: Source/core/rendering/RenderTableSection.cpp
diff --git a/Source/core/rendering/RenderTableSection.cpp b/Source/core/rendering/RenderTableSection.cpp
index f1983f60442b97de4e318913bdadcee70a486cf4..54633ea6c945732daad01c2d0241c7bf84cf8b4e 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -484,57 +484,68 @@ static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell*
     return false;
 }
 
-bool RenderTableSection::isHeightNeededForRowHavingOnlySpanningCells(unsigned row)
+unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells)

[dsinclair, 2014/12/03 19:25:23]

If I'm reading this correctly effectedRowByExtraHeight is the index of the row
which will receive the height?

I think a better name would be rowToApplyExtraHeight

[dsinclair, 2014/12/03 19:25:23]

To clarify this, can we rename extraHeightToPropgate to
extraTableHeightToPropgate and accumulatedPositionIncrease to
accumulatedCellPositionIncrease?

[a.suchit, 2014/12/10 04:27:42]

Done.

[a.suchit, 2014/12/10 04:27:42]

Done.

 {
+    ASSERT(rowHasOnlySpanningCells(row));
+
     unsigned totalCols = m_grid[row].row.size();
 
     if (!totalCols)
-        return false;
+        return 0;
+
+    unsigned rowHeight = 0;
 
     for (unsigned col = 0; col < totalCols; col++) {
         const CellStruct& rowSpanCell = cellAt(row, col);
 
-        if (rowSpanCell.cells.size()) {
-            RenderTableCell* cell = rowSpanCell.cells[0];
-            const unsigned rowIndex = cell->rowIndex();
-            const unsigned rowSpan = cell->rowSpan();
-            int totalRowSpanCellHeight = 0;
+        if (!rowSpanCell.cells.size())
+            continue;
+
+        RenderTableCell* cell = rowSpanCell.cells[0];
 
-            for (unsigned row = 0; row < rowSpan; row++) {
-                unsigned actualRow = row + rowIndex;
-                totalRowSpanCellHeight += m_rowPos[actualRow + 1] - m_rowPos[actualRow];
-            }
-            totalRowSpanCellHeight -= borderSpacingForRow(rowIndex + rowSpan - 1);
+        if (cell->rowSpan() <= 1)
+            continue;
+
+        const unsigned cellRowIndex = cell->rowIndex();
+        const unsigned cellRowSpan = cell->rowSpan();
 
-            if (totalRowSpanCellHeight < cell->logicalHeightForRowSizing())
-                return true;
+        // As we are going from the top of the table to the bottom to calculate the row
+        // heights for rows that only contain spanning cells and all previous rows are
+        // processed we only need to find the number of rows with spanning cells from the
+        // current cell to the end of the current cells spanning height.
+        unsigned startRowForSpanningCellCount = max(cellRowIndex, row);
+        unsigned endRow = cellRowIndex + cellRowSpan;
+        unsigned spanningCellsRowsCountHavingZeroHeight = 0;
+
+        if (startRowForSpanningCellCount) {
+            spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRowForSpanningCellCount - 1];
+        } else {
+            spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow - 1];
         }

[dsinclair, 2014/12/03 19:25:22]

I think this can be simplified too:

unsigned spanningCellsRowsCountHavingZeroHeight =
rowsCountWithOnlySpanningCells[endRow - 1];
if (startRowForSpanningCellCount)
  spanningCellsRowsCountHavingZeroHeight -=
rowsCountWithOnlySpanningCells[startRowForSpanningCellCount - 1];

[a.suchit, 2014/12/10 04:27:42]

Done.

-    }
 
-    return false;
-}
+        int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[cellRowIndex];

[dsinclair, 2014/12/03 19:25:23]

Is totalRowspanCellHeight the total height that is available for the cell?

If that's the case, I think neededCellHeight might be a better name.

[a.suchit, 2014/12/10 04:27:42]

This is current rowspan cells height.

 
-unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
-{
-    ASSERT(rowHasOnlySpanningCells(row));
+        totalRowspanCellHeight -= borderSpacingForRow(endRow - 1);
 
-    unsigned totalCols = m_grid[row].row.size();
+        totalRowspanCellHeight += accumulatedPositionIncrease;

[dsinclair, 2014/12/03 19:25:23]

Why not do these three calculations at the same time?

[a.suchit, 2014/12/10 04:27:42]

Done.

[a.suchit, 2014/12/10 04:27:42]

Readability and clarification.

+        if (effectedRowByExtraHeight >= cellRowIndex && effectedRowByExtraHeight < endRow)
+            totalRowspanCellHeight += extraHeightToPropagate;
 
-    if (!totalCols)
-        return 0;
+        if (totalRowspanCellHeight < cell->logicalHeightForRowSizing()) {
+            unsigned extraHeightRequired = rowSpanCell.cells[0]->logicalHeightForRowSizing() - totalRowspanCellHeight;

[dsinclair, 2014/12/03 19:25:23]

We use cell above and rowSpanCell.cells[0] here, can you make this one cell so
it's consistent?

[a.suchit, 2014/12/10 04:27:42]

Done.

+            int remainder = extraHeightRequired % spanningCellsRowsCountHavingZeroHeight;
 
-    unsigned rowHeight = 0;
+            if (remainder)
+                extraHeightRequired += spanningCellsRowsCountHavingZeroHeight - remainder;

[dsinclair, 2014/12/03 19:25:22]

Why do rows having zero height effect the height required for our current row?

[a.suchit, 2014/12/10 04:27:42]

Just assume a example here :
spanning cell have 7 rows with only spanning cells and extra height required
here is 11.
If 11 is distributed here without considering the remainder then it would like
below :
1st row height = 11 / 7 = 1
2nd row height = 10 / 6 = 1
3rd row height = 9 / 5 = 1
4th row height8 / 4 = 2
5th row height6 / 3 = 2
6th row height4 / 2 = 2
7th row height2 / 1 = 2

current schenario :
1st row height = 11 / 7 => 14 / 7 = 2
2nd row height = 9 / 6 => 12 / 6 = 2
3rd row height = 7 / 5 => 10 / 5 = 2
4th row height = 5 / 4 => 8 / 4 = 2
5th row height = 3 / 3 = 1
6th row height = 2 / 2 = 1
7th row height = 1 / 1 = 1

Remainder part, we can remove it here.

 
-    for (unsigned col = 0; col < totalCols; col++) {
-        const CellStruct& rowSpanCell = cellAt(row, col);
-        if (rowSpanCell.cells.size() && rowSpanCell.cells[0]->rowSpan() > 1)
-            rowHeight = std::max(rowHeight, rowSpanCell.cells[0]->logicalHeightForRowSizing() / rowSpanCell.cells[0]->rowSpan());
+            rowHeight = std::max(rowHeight, extraHeightRequired / spanningCellsRowsCountHavingZeroHeight);
+        }
     }
 
     return rowHeight;
 }
 
-void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight)
+void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells)
 {
     ASSERT(spanningRowsHeight.rowHeight.size());
 
@@ -546,8 +557,8 @@ void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell
 
     for (unsigned row = 0; row < spanningRowsHeight.rowHeight.size(); row++) {
         unsigned actualRow = row + rowIndex;
-        if (!spanningRowsHeight.rowHeight[row] && rowHasOnlySpanningCells(actualRow) && isHeightNeededForRowHavingOnlySpanningCells(actualRow)) {
-            spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow);
+        if (!spanningRowsHeight.rowHeight[row] && rowHasOnlySpanningCells(actualRow)) {
+            spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow, accumulatedPositionIncrease, rowIndex + rowSpan, extraHeightToPropagate, rowsCountWithOnlySpanningCells);
             accumulatedPositionIncrease += spanningRowsHeight.rowHeight[row];
         }
         m_rowPos[actualRow + 1] += accumulatedPositionIncrease;
@@ -570,6 +581,16 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
     unsigned lastRowIndex = 0;
     unsigned lastRowSpan = 0;
 
+    Vector<int> rowsCountWithOnlySpanningCells;
+
+    // At this stage, Height of the rows are zero for the one containing only spanning cells.
+    int count = 0;
+    for (unsigned row = 0; row < m_grid.size(); row++) {
+        if (rowHasOnlySpanningCells(row))
+            count++;
+        rowsCountWithOnlySpanningCells.append(count);
+    }
+
     for (unsigned i = 0; i < rowSpanCells.size(); i++) {
         RenderTableCell* cell = rowSpanCells[i];
 
@@ -607,7 +628,7 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
 
         // Here we are handling only row(s) who have only rowspanning cells and do not have any empty cell.
         if (spanningRowsHeight.isAnyRowWithOnlySpanningCells)
-            updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
+            updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight, extraHeightToPropagate, rowsCountWithOnlySpanningCells);
 
         // This code handle row(s) that have rowspanning cell(s) and at least one empty cell.
         // Such rows are not handled below and end up having a height of 0. That would mean