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

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 234 matching lines @@
             if (inColSpan)
                 c.inColSpan = true;
         }
         m_cCol++;
         cSpan -= currentSpan;
         inColSpan = true;
     }
     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 rowSpanCellHeight = cell->logicalHeightForRowSizing();
+    int totalRowsHeight = 0;
 
-    int totalRowsHeight = 0;
-    int rowSpanCellHeight = cell->logicalHeightForRowSizing();
-    Vector<int> rowsHeight(rowSpan);
-
-    // 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);
         totalRowsHeight += rowsHeight[row];
         rowSpanCellHeight -= borderSpacingForRow(actualRow);
     }
     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;
+
+    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++) {
+        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;
+    }
+
+    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.
-    int remainingHeight = rowSpanCellHeight;
-
-    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);
-    }
-    // Remaining height added in the last row under rowSpan cell
-    m_rowPos[rowIndex + rowSpan] += remainingHeight;
+    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();
 
         // Apply changed height by rowSpan cells to rows present at the end of the table
         for (unsigned row = rowIndex + rowSpan + 1; row <= totalRows; row++)
             m_rowPos[row] += changedHeight;
     }
 }
 
@@ skipping 1214 matching lines @@
     info.addMember(logicalHeight, "logicalHeight");
 }
 
 void RenderTableSection::CellStruct::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) const
 {
     MemoryClassInfo info(memoryObjectInfo, this, PlatformMemoryTypes::Rendering);
     info.addMember(cells, "cells");
 }
 
 } // namespace WebCore