Row with only spanning cells have a height of 0.

Source/core/rendering/RenderTableSection.cpp

 /*
  * Copyright (C) 1997 Martin Jones (mjones@kde.org)
  *           (C) 1997 Torben Weis (weis@kde.org)
  *           (C) 1998 Waldo Bastian (bastian@kde.org)
  *           (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 235 matching lines @@
             if (inColSpan)
                 c.inColSpan = true;
         }
         m_cCol++;
         cSpan -= currentSpan;
         inColSpan = true;
     }
     cell->setCol(table()->effColToCol(col));
 }
 
+unsigned RenderTableSection::rowHasOnlySpanningCells(unsigned row)
+{
+    ASSERT(row < m_grid.size());

[Julien - ping for review, 2013/09/17 16:51:26]

This will be caught by Vector::at() (even in release, out of bounds access are
really nasty) at line 260 as you do m_grid[row].

[a.suchit, 2013/09/18 11:40:38]

returned if row have more than vector size.

+
+    unsigned totalCols = m_grid[row].row.size();
+
+    if (!totalCols)
+        return false;
+
+    for (unsigned col = 0; col < totalCols; col++) {
+        CellStruct rowSpanCell = cellAt(row, col);

[Julien - ping for review, 2013/09/17 16:51:26]

Let's avoid a copy by using a reference here. Also it should probably be const?

[a.suchit, 2013/09/18 11:40:38]

Done.

+        if (rowSpanCell.cells.size()) {
+            if (rowSpanCell.cells[0]->rowSpan() == 1)
+                return false;
+        } else {
+            return false;
+        }

[Julien - ping for review, 2013/09/17 16:51:26]

Let's write this (more readable with the early return):

if (rowSpan.cells.isEmpty()) {
    // Some explanations here for why we consider one empty cell to make the
return false as it's still unclear to me. Please let's make it a why comment.
    return false;
}

if (rowSpanCell.cells[0]->rowSpan() == 1)
    return false;

[a.suchit, 2013/09/18 11:40:38]

Done.

+    }
+
+    return true;
+}
+
 void RenderTableSection::populateSpanningRowsHeightFromCell(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight)
 {
     const unsigned rowSpan = cell->rowSpan();
     const unsigned rowIndex = cell->rowIndex();
 
     spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing = cell->logicalHeightForRowSizing();
 
     spanningRowsHeight.rowHeight.resize(rowSpan);
     spanningRowsHeight.totalRowsHeight = 0;
     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 (rowHasOnlySpanningCells(actualRow)) {
+                spanningRowsHeight.rowWithOnlySpanningCells = true;

[Julien - ping for review, 2013/09/17 16:51:26]

We can remove the inner branch (yay!):

spanningRowsHeight.rowWithOnlySpanningCells |=
rowHasOnlySpanningCells(actualRow);

[a.suchit, 2013/09/18 11:40:38]

Done.

+            }
+        }
         spanningRowsHeight.totalRowsHeight += spanningRowsHeight.rowHeight[row];
         spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing -= borderSpacingForRow(actualRow);
     }
     // We don't span the following row so its border-spacing (if any) should be included.
     spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing += borderSpacingForRow(rowIndex + rowSpan - 1);
 }
 
 void RenderTableSection::distributeExtraRowSpanHeightToPercentRows(RenderTableCell* cell, int totalPercent, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
 {
     if (!extraRowSpanningHeight || !totalPercent)
@@ skipping 110 matching lines @@
     if (cellIsFullyIncludedInOtherCell(cell1, cell2))
         return true;
     // Sorting lower row index first because first we need to apply the extra height of spanning cell which
     // comes first in the table so lower rows's position would increment in sequence.
     if (cellIsFullyIncludedInOtherCell(cell2, cell1))
         return (cell1->rowIndex() < cell2->rowIndex());
 
     return false;
 }
 
+unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)

[Julien - ping for review, 2013/09/17 16:51:26]

This function is never called.

[a.suchit, 2013/09/18 11:40:38]

It is called by function "updateRowsHeightHavingOnlySpanningCells" in line : 459

+{
+    ASSERT(row < m_grid.size());

[Julien - ping for review, 2013/09/17 16:51:26]

Again, duplicated by Vector::at().

[a.suchit, 2013/09/18 11:40:38]

returned if row have more than vector size.

+    ASSERT(rowHasOnlySpanningCells(row));
+
+    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)
+{
+    ASSERT(spanningRowsHeight.rowHeight.size());
+
+    int accumulatedPositionIncrease = 0;
+    const unsigned rowSpan = cell->rowSpan();
+    const unsigned rowIndex = cell->rowIndex();
+
+    ASSERT(rowSpan == spanningRowsHeight.rowHeight.size());

[Julien - ping for review, 2013/09/17 16:51:26]

Nice!

+
+    for (unsigned row = 0; row < spanningRowsHeight.rowHeight.size(); row++) {
+        unsigned actualRow = row + rowIndex;
+        if (!spanningRowsHeight.rowHeight[row] && rowHasOnlySpanningCells(actualRow)) {
+            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)
 {
     ASSERT(rowSpanCells.size());
 
     // 'rowSpanCells' list is already sorted based on the cells rowIndex in ascending order
     // Arrange row spanning cell in the order in which we need to process first.
     std::sort(rowSpanCells.begin(), rowSpanCells.end(), compareRowSpanCellsInHeightDistributionOrder);
 
@@ skipping 29 matching lines @@
                 m_rowPos[row] += extraHeightToPropagate;
         }
 
         lastRowIndex = rowIndex;
         lastRowSpan = rowSpan;
 
         struct SpanningRowsHeight spanningRowsHeight;
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
-        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight)
+        if (spanningRowsHeight.rowWithOnlySpanningCells)
+            updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
+
+        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+            extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
             continue;
+        }
 
         int totalPercent = 0;
         int totalAutoRowsHeight = 0;
         int totalRemainingRowsHeight = spanningRowsHeight.totalRowsHeight;
 
         // FIXME: Inner spanning cell height should not change if it have fixed height when it's parent spanning cell
         // is distributing it's extra height in rows.
 
         // Calculate total percentage, total auto rows height and total rows height except percent rows.
         for (unsigned row = rowIndex; row < spanningCellEndIndex; row++) {
@@ skipping 1238 matching lines @@
     if (!style()->isLeftToRightDirection())
         cellLocation.setX(table()->columnPositions()[table()->numEffCols()] - table()->columnPositions()[table()->colToEffCol(cell->col() + cell->colSpan())] + horizontalBorderSpacing);
     else
         cellLocation.setX(table()->columnPositions()[effectiveColumn] + horizontalBorderSpacing);
 
     cell->setLogicalLocation(cellLocation);
     view()->addLayoutDelta(oldCellLocation - cell->location());
 }
 
 } // namespace WebCore