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 b0264c6f7fd92c939df76fadccf26cf87534875b..fe988609cb0e84c1c72dcb9d74afd1754b6d0267 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -485,57 +485,61 @@ static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell*
     return false;
 }
 
-bool RenderTableSection::isHeightNeededForRowHavingOnlySpanningCells(unsigned row)
+unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, unsigned& extraHeightToPropagate, unsigned r1, int& accumulatedPositionIncrease, unsigned r2, Vector<int>& rowsCountWithOnlySpanningCells)

[Julien - ping for review, 2014/10/08 00:16:08]

r1 and r2 are not good variable names :(

Also this function has 6 arguments, which is a bad smell (and the limit of
craziness).

To top it off, the only caller calls it with r2 == row.

[a.suchit, 2014/10/08 11:30:08]

changed and name of r1 variable

I did not observed about r2 == row. Thanks
Now I removed it.

Now, function has 5 argument, it is also more but I don't have any other option
because I do not want to use structure just for it.

 {
+    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;
 
-            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) {
+                const unsigned rowIndex = cell->rowIndex();
+                const unsigned rowSpan = cell->rowSpan();
 
-            if (totalRowSpanCellHeight < cell->logicalHeightForRowSizing())
-                return true;
-        }
-    }
+                unsigned endRow = rowIndex + rowSpan;
+                unsigned spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow -1] - rowsCountWithOnlySpanningCells[max(rowIndex, row)];

[Julien - ping for review, 2014/10/08 00:16:08]

max(rowIndex, row) is repeated 4 times, it seems like we should have a variable
for it.

It would be nice to give an explanation as to *why* this is the index we're
looking for.

[a.suchit, 2014/10/08 11:30:08]

Done.

 
-    return false;
-}
+                if (max(rowIndex, row)) {
+                    spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[max(rowIndex, row)] - rowsCountWithOnlySpanningCells[max(rowIndex, row) - 1];

[Julien - ping for review, 2014/10/08 00:16:08]

I am probably confused by this line but it seems deeply wrong to use a *table
index* to access a spot into the sparse array rowsCountWithOnlySpanningCells.

[a.suchit, 2014/10/08 11:30:08]

Acknowledged.

+                } else {
+                    spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[0];

[Julien - ping for review, 2014/10/08 00:16:08]

This else is suspicious, I don't see why some cells are different than others.

[a.suchit, 2014/10/08 11:30:08]

Acknowledged.

+                }
 
-unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
-{
-    ASSERT(rowHasOnlySpanningCells(row));
+                int totalRowspanCellHeight = m_rowPos[rowIndex + rowSpan] - m_rowPos[rowIndex];
 
-    unsigned totalCols = m_grid[row].row.size();
+                totalRowspanCellHeight -= borderSpacingForRow(rowIndex + rowSpan - 1);
+                if (r1 > rowIndex && r1 < endRow)
+                    totalRowspanCellHeight += extraHeightToPropagate;
+                if (r2 > rowIndex && r2 < endRow)
+                    totalRowspanCellHeight += accumulatedPositionIncrease;
 
-    if (!totalCols)
-        return 0;
+                if (totalRowspanCellHeight < cell->logicalHeightForRowSizing()) {
+                    unsigned extraHeightRequired = rowSpanCell.cells[0]->logicalHeightForRowSizing() - totalRowspanCellHeight;
+                    int remainder = extraHeightRequired % spanningCellsRowsCountHavingZeroHeight;
 
-    unsigned rowHeight = 0;
+                    if (remainder)
+                        extraHeightRequired += spanningCellsRowsCountHavingZeroHeight - remainder;
 
-    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());
 
@@ -547,8 +551,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, extraHeightToPropagate, rowIndex + rowSpan, accumulatedPositionIncrease, actualRow, rowsCountWithOnlySpanningCells);
             accumulatedPositionIncrease += spanningRowsHeight.rowHeight[row];
         }
         m_rowPos[actualRow + 1] += accumulatedPositionIncrease;
@@ -571,6 +575,16 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
     unsigned lastRowIndex = 0;
     unsigned lastRowSpan = 0;
 
+    Vector<int> rowsCountWithOnlySpanningCells;
+
+    // At this stage, Height of the rows are zero those contains only spanning cells.

[Julien - ping for review, 2014/10/08 00:16:08]

Let's do correct English sentence:

// At this stage, the height of the rows is zero for the one containing only
spanning cells.

[a.suchit, 2014/10/08 11:30:08]

Done.

+    int count = 0;
+    for (unsigned row = 0; row < m_grid.size(); row++) {
+        if (rowHasOnlySpanningCells(row))
+            count++;
+        rowsCountWithOnlySpanningCells.append(count);
+    }

[Julien - ping for review, 2014/10/08 00:16:08]

You've added an unrelated O(N) loop here inside a function that can be called
repeatedly during layout :(

[a.suchit, 2014/10/08 11:30:08]

If it is not done here then repeatedly, need to check it in
calcRowHeightHavingOnlySpanningCells(...) which is too expensive.

+
     for (unsigned i = 0; i < rowSpanCells.size(); i++) {
         RenderTableCell* cell = rowSpanCells[i];
 
@@ -608,7 +622,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