Height of fixed height cell is not proper when cell's row is under row spanning cell.

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 3bcff2dbf6d5d9c45795c5cf47efbd7cf47e71d3..1bc0fffc5a5d9d9cd5f1a514fd3bf70bcf165a7e 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -252,24 +252,16 @@ void RenderTableSection::addCell(RenderTableCell* cell, RenderTableRow* row)
     cell->setCol(table()->effColToCol(col));
 }
 
-// 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)
+// Getting height of rows in current rowSpan cell, getting total height of rows and adjusting rowSpan cell height with border spacing.
+int RenderTableSection::getRowsHeightInRowSpan(RenderTableCell* cell, int& expectedTotalRowsHeight, Vector<int>& rowsHeight)
 {
-    ASSERT(rowSpanCells.size());
-
-    // FIXME: For now, we handle the first rowspan cell in the table but this is wrong.
-    RenderTableCell* cell = rowSpanCells[0];
+    ASSERT(cell);
 
     unsigned rowSpan = cell->rowSpan();
     unsigned rowIndex = cell->rowIndex();
-    int initialPos = m_rowPos[rowIndex + rowSpan];
-
-    int totalRowsHeight = 0;
     int rowSpanCellHeight = cell->logicalHeightForRowSizing();
-    Vector<int> rowsHeight(rowSpan);
+    int totalRowsHeight = 0;
 
-    // Getting height of rows in current rowSpan cell, getting total height of rows and adjusting rowSpan cell height with border spacing.
     for (unsigned row = 0; row < rowSpan; row++) {
         unsigned actualRow = row + rowIndex;
         rowsHeight[row] = m_rowPos[actualRow + 1] - m_rowPos[actualRow] - borderSpacingForRow(actualRow);
@@ -278,22 +270,126 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
     }
     rowSpanCellHeight += borderSpacingForRow(rowIndex + rowSpan - 1);
 
-    if (!totalRowsHeight || rowSpanCellHeight <= totalRowsHeight)
+    expectedTotalRowsHeight = rowSpanCellHeight;
+
+    return totalRowsHeight;
+}
+
+// Calculate new logical height for the rows those comes in row spanning cell
+// First we distribute the extra height in percentage rows
+// Remaining extra height distributes to auto rows
+// If auro rows are not present in the table then remaining extra height distributes in all rows except percentage rows
+void RenderTableSection::recalcRowsHeightInRowSpanningCell(RenderTableCell* cell, int& expectedTotalRowsHeight, int& currTotalRowsHeight, Vector<int>& rowsHeight)

[Julien - ping for review, 2013/06/27 22:09:25]

You pass |currTotalRowsHeight| as a reference, yet you never update it.

The name of the function also gives no direction as to what this giant function
is supposed to do.

+{
+    ASSERT(cell);

[Julien - ping for review, 2013/06/27 22:09:25]

If you need those type of ASSERT, it means that you should pass a reference and
not a pointer to the function.

+    ASSERT(currTotalRowsHeight);
+    ASSERT(expectedTotalRowsHeight > currTotalRowsHeight);
+
+    if (expectedTotalRowsHeight <= currTotalRowsHeight)
         return;
 
-    // Recalculating the height of rows based on rowSpan cell height if rowSpan cell height is more than total height of rows.
-    int remainingHeight = rowSpanCellHeight;
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+    int totalPercent = 0;
+    int totalAutoRowsHeight = 0;
+    int extraHeight = expectedTotalRowsHeight - currTotalRowsHeight;

[Julien - ping for review, 2013/06/27 22:09:25]

I am sorry but I can't say what |expectedTotalRowHeight| could mean, same for
|currTotalRowsHeight|.

+    int totalRowsHeightExceptPercentRows = 0;
+
+    ASSERT(rowsHeight.size() == rowSpan);
 
+    // Calculate total percentage, total auto rows height and total rows height except percent rows.
+    totalRowsHeightExceptPercentRows = currTotalRowsHeight;

[Julien - ping for review, 2013/06/27 22:09:25]

Why do we need 2 initialization of this variable?

     for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
-        int rowHeight = (rowSpanCellHeight * rowsHeight[row - rowIndex]) / totalRowsHeight;
-        remainingHeight -= rowHeight;
-        m_rowPos[row + 1] = m_rowPos[row] + rowHeight + borderSpacingForRow(row);
+        if (m_grid[row].logicalHeight.isPercent()) {
+            totalPercent += m_grid[row].logicalHeight.percent();
+            totalRowsHeightExceptPercentRows -= rowsHeight[row - rowIndex];
+        } else if (m_grid[row].logicalHeight.isAuto()) {
+            totalAutoRowsHeight += rowsHeight[row - rowIndex];
+        }
+    }
+
+    // If percent is 100 or more then whole extra height would be distribute in percent rows based on their percent ratios so that
+    // all percent rows would get its own share from extra height based on their percentages.
+    // Else total percent share would go to percent rows and remaining would be distributed in other rows.
+    if (100 <= totalPercent) {
+        int changedPosBy = 0;
+        for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+            if (m_grid[row].logicalHeight.isPercent())
+                changedPosBy += extraHeight * m_grid[row].logicalHeight.percent() / totalPercent;
+            m_rowPos[row + 1] += changedPosBy;
+        }
+
+        // Remaining height added in the last row under rowSpan cell
+        m_rowPos[rowIndex + rowSpan] += extraHeight - changedPosBy;
+        extraHeight = 0;
+    } else {
+        int changedPosBy = 0;
+        for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+            if (m_grid[row].logicalHeight.isPercent())
+                changedPosBy += extraHeight * m_grid[row].logicalHeight.percent() / 100;
+            m_rowPos[row + 1] += changedPosBy;
+        }
+
+        extraHeight = extraHeight - changedPosBy;
     }
-    // Remaining height added in the last row under rowSpan cell
-    m_rowPos[rowIndex + rowSpan] += remainingHeight;
+
+    if (extraHeight) {
+        ASSERT(totalRowsHeightExceptPercentRows);
+
+        // If auto rows present in the table then remaining extra height would be distribute in auto rows based on their height ratios so that
+        // all auto rows would get its own share from remaining extra height based on their heights.
+        // Else remaining extra height would be distribute in all rows (except percent rows) based on their height ratios so that
+        // all rows would get its own share from remaining extra height based on their heights.
+        if (totalAutoRowsHeight) {
+            int changedPosBy = 0;
+
+            for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+                if (m_grid[row].logicalHeight.isAuto())
+                    changedPosBy += (extraHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight;
+                m_rowPos[row + 1] += changedPosBy;
+            }
+            // Remaining height added in the last row under rowSpan cell
+            m_rowPos[rowIndex + rowSpan] += extraHeight - changedPosBy;
+        } else if (totalRowsHeightExceptPercentRows) {
+            int changedPosBy = 0;
+
+            for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+                if (!m_grid[row].logicalHeight.isPercent())
+                    changedPosBy += (extraHeight * rowsHeight[row - rowIndex]) / totalRowsHeightExceptPercentRows;
+                m_rowPos[row + 1] += changedPosBy;
+            }
+            // Remaining height added in the last row under rowSpan cell
+            m_rowPos[rowIndex + rowSpan] += extraHeight - changedPosBy;
+        }
+    }
+}
+
+// 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());
+
+    // FIXME: For now, we handle the first rowspan cell in the table but this is wrong.
+    RenderTableCell* cell = rowSpanCells[0];
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+    int initialPos = m_rowPos[rowIndex + rowSpan];
+
+    int expectedTotalRowsHeight = 0;
+    Vector<int> rowsHeight(rowSpan);
+
+    int totalRowsHeight = getRowsHeightInRowSpan(cell, expectedTotalRowsHeight, rowsHeight);
+
+    if (!totalRowsHeight || expectedTotalRowsHeight <= totalRowsHeight)
+        return;
+
+    // Recalculating the height of rows based on rowSpan cell height if rowSpan cell height is more than total height of rows.
+    recalcRowsHeightInRowSpanningCell(cell, expectedTotalRowsHeight, totalRowsHeight, rowsHeight);
 
     // Getting total changed height in the table
-    unsigned changedHeight = changedHeight = m_rowPos[rowIndex + rowSpan] - initialPos;
+    unsigned changedHeight = m_rowPos[rowIndex + rowSpan] - initialPos;
 
     if (changedHeight) {
         unsigned totalRows = m_grid.size();