Row with only spanning cells have a height of 0.

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 da4b53a56e5c1fb5c6908e7bd47bd46eca864cab..66e6a14035176ef5bff245b9f6d3739678a657ec 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -253,6 +253,32 @@ void RenderTableSection::addCell(RenderTableCell* cell, RenderTableRow* row)
     cell->setCol(table()->effColToCol(col));
 }
 
+// Check weather row has only spanning cells

[Julien - ping for review, 2013/09/09 21:32:55]

s/weather/whether/ :(

This is yet another what comment that duplicates the name.

[a.suchit, 2013/09/10 13:48:00]

This comment is not needed so removed it.

+unsigned RenderTableSection::hasRowOnlySpanningCells(unsigned row)

[Julien - ping for review, 2013/09/09 21:32:55]

rowHasOnlySpanningCells is more English.

[a.suchit, 2013/09/10 13:48:00]

Done.

+{
+    ASSERT(row < m_grid.size());
+
+    unsigned totalCols = m_grid[row].row.size();
+    bool spanningCellStart = false, emptyCell = false;

[Julien - ping for review, 2013/09/09 21:32:55]

Style violation: we define 1 variable per line.

Why do we need those 2 variables that are never used in practice?

[a.suchit, 2013/09/10 13:48:00]

Not needed. Removed

+
+    if (!totalCols)
+        return false;
+
+    for (unsigned col = 0; col < totalCols; col++) {
+        CellStruct rowSpanCell = cellAt(row, col);
+        if (rowSpanCell.cells.size()) {
+            if (rowSpanCell.cells[0]->rowSpan() == 1)
+                return false;
+            if (row == rowSpanCell.cells[0]->rowIndex())
+                spanningCellStart = true;
+        } else {
+            return false;

[Julien - ping for review, 2013/09/09 21:32:55]

An empty cell emplacement makes the result which is weird. Could you explain the
need for this?

[a.suchit, 2013/09/10 13:48:00]

Empty cell does not say any thing and we are checking here for all row spanning
cells in the row.
In actuality, Empty cell do not have any roll in layout and render and even if
empty cell considers in layout and render then it would be 1 span cell.

+        }
+    }
+
+    return true;
+}
+
 void RenderTableSection::populateSpanningRowsHeightFromCell(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight)
 {
     const unsigned rowSpan = cell->rowSpan();
@@ -265,6 +291,11 @@ void RenderTableSection::populateSpanningRowsHeightFromCell(RenderTableCell* cel
     for (unsigned row = 0; row < rowSpan; row++) {
         unsigned actualRow = row + rowIndex;
         spanningRowsHeight.rowHeight[row] = m_rowPos[actualRow + 1] - m_rowPos[actualRow] - borderSpacingForRow(actualRow);
+        if (!spanningRowsHeight.rowHeight[row]) {
+            if (hasRowOnlySpanningCells(actualRow)) {
+                spanningRowsHeight.rowsCountWithOnlySpanningCells++;
+            }
+        }
         spanningRowsHeight.totalRowsHeight += spanningRowsHeight.rowHeight[row];
         spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing -= borderSpacingForRow(actualRow);
     }
@@ -395,6 +426,48 @@ static bool compareRowSpanCellsInHeightDistributionOrder(const RenderTableCell*
     return false;
 }
 
+unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)

[Julien - ping for review, 2013/09/09 21:32:55]

Note that the name makes people believe this only works on rows with only
spanning cells which is not the case at all.

[a.suchit, 2013/09/10 13:48:00]

Here is considered that row passed here have only spanning cells.

Added assert to check for row with only spanning cells.

+{
+    ASSERT(row < m_grid.size());
+
+    unsigned totalCols = m_grid[row].row.size();
+
+    if (!totalCols)
+        return 0;
+
+    unsigned rowHeight = 0;
+
+    for (unsigned col = 0; col < totalCols; col++) {
+        CellStruct rowSpanCell = cellAt(row, col);
+        if (rowSpanCell.cells.size() && rowSpanCell.cells[0]->rowSpan() > 1)
+            rowHeight = max(rowHeight, rowSpanCell.cells[0]->logicalHeightForRowSizing() / rowSpanCell.cells[0]->rowSpan());
+    }
+
+    return rowHeight;
+}
+
+void RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight)

[Julien - ping for review, 2013/09/09 21:32:55]

Ditto.

[a.suchit, 2013/09/10 13:48:00]

Down we are checking for row with only spanning cells than we are updating the
height of the rows.

+{
+    ASSERT(spanningRowsHeight.rowHeight.size());
+
+    int accumulatedPositionIncrease = 0;
+    const unsigned rowSpan = cell->rowSpan();
+    const unsigned rowIndex = cell->rowIndex();
+
+    ASSERT(rowSpan == spanningRowsHeight.rowHeight.size());
+
+    for (unsigned row = 0; row < spanningRowsHeight.rowHeight.size(); row++) {
+        unsigned actualRow = row + rowIndex;
+        if (!spanningRowsHeight.rowHeight[row] && hasRowOnlySpanningCells(actualRow)) {

[Julien - ping for review, 2013/09/09 21:32:55]

This is now O(columns) as hasRowOnlySpanningCells needs to check all columns. We
should probably put a comment about it, mentioning that we would want to have
this cached somehow.

[a.suchit, 2013/09/10 13:48:00]

We are checking only rows those have 0 height. In most of the cases, row height
would be 0 only when row with only spanning cells.

+            spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow);
+            accumulatedPositionIncrease += spanningRowsHeight.rowHeight[row];
+        }
+        m_rowPos[actualRow + 1] += accumulatedPositionIncrease;
+    }
+
+    spanningRowsHeight.totalRowsHeight += accumulatedPositionIncrease;
+}
+
 // Distribute rowSpan cell height in rows those comes in rowSpan cell based on the ratio of row's height if
 // 1. RowSpan cell height is greater then the total height of rows in rowSpan cell
 void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells& rowSpanCells)
@@ -444,8 +517,13 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
-        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight)
+        if (spanningRowsHeight.rowsCountWithOnlySpanningCells)
+            updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);

[Julien - ping for review, 2013/09/09 21:32:55]

Probably dumb question but why do we need all this *new* logic instead of
reusing the existing logic?

[a.suchit, 2013/09/10 13:48:00]

Existing logic works for only when row have height.
Row with only spanning cells have 0 height so with existing logic, height will
remain 0.
First, we are giving a some valid height to these row and than applying existing
logic so these rows will get proper height.

+
+        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+            extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
             continue;
+        }
 
         int totalPercent = 0;
         int totalAutoRowsHeight = 0;