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 41ad27591186675bdfe59ef7837d5c6d0c22ba4a..c1bd949510e24989635a85fbf3b9b63d904079b3 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -255,6 +255,95 @@ void RenderTableSection::addCell(RenderTableCell* cell, RenderTableRow* row)
     cell->setCol(table()->effColToCol(col));
 }
 
+void RenderTableSection::calcRowsHeightInRowSpan(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight)
+{
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+
+    spanningRowsHeight.expectedTotalRowsHeight = cell->logicalHeightForRowSizing();

[Julien - ping for review, 2013/07/13 00:56:36]

I have a hard time with this variable |expectedTotalRowsHeight|. In a way, it is
expected but the context feels weird.

How about spanningCellHeightIgnoringBorderSpacing?

[a.suchit, 2013/07/15 12:06:19]

Done.

+
+    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);
+        spanningRowsHeight.totalRowsHeight += spanningRowsHeight.rowHeight[row];
+        spanningRowsHeight.expectedTotalRowsHeight -= borderSpacingForRow(actualRow);
+    }
+    spanningRowsHeight.expectedTotalRowsHeight += borderSpacingForRow(rowIndex + rowSpan - 1);
+}
+
+void RenderTableSection::distributeExtraRowSpanHeightToPrecentRows(RenderTableCell* cell, int tableHeight, int totalPercent, int& extraRowSpanningHeight)

[Julien - ping for review, 2013/07/13 00:56:36]

typo: PrecentRows

[a.suchit, 2013/07/15 12:06:19]

Done.

+{
+    if (!extraRowSpanningHeight || !totalPercent)
+        return;
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();

[Julien - ping for review, 2013/07/13 00:56:36]

const?

[a.suchit, 2013/07/15 12:06:19]

Done.

+    int percent = min(totalPercent, 100);
+    int remainingRowSpanningHeight = extraRowSpanningHeight;
+
+    int changedPosBy = 0;
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (percent > 0 && remainingRowSpanningHeight > 0) {
+            if (m_grid[row].logicalHeight.isPercent()) {
+                int toAdd = (tableHeight + extraRowSpanningHeight) * m_grid[row].logicalHeight.percent() / 100;
+
+                changedPosBy += min(toAdd, remainingRowSpanningHeight);
+                remainingRowSpanningHeight -= toAdd;
+                percent -= m_grid[row].logicalHeight.percent();
+            }
+        }
+        m_rowPos[row + 1] += changedPosBy;
+    }
+
+    extraRowSpanningHeight = remainingRowSpanningHeight > 0 ? remainingRowSpanningHeight : 0;

[Julien - ping for review, 2013/07/13 00:56:36]

If you need to do a max, please just use one instead of re-inventing the wheel.

I wouldn't expect remainingRowSpanningHeight to be negative btw or that would
mean that you over-extended (and/or have rounding errors).

[a.suchit, 2013/07/15 12:06:19]

changedPosBy += min(toAdd, remainingRowSpanningHeight);
remainingRowSpanningHeight -= toAdd;

|ChangedPosBy| is increasing by value min(toAdd, remainingRowSpanningHeight) and
|remainingRowspanningHeight| should be decremented by same value. But it was not
done just because of saving min called twice. So in some cases
|remainingRowSpanningHeight| was going in negative and handled it after the
loop.

Now it is changed. min(toAdd, remainingRowSpanningHeight) value took in |toAdd|
and |toAdd| used to update |ChangedPosBy| and |remainingRowSpanningHeight|.

+}
+
+void RenderTableSection::distributeExtraRowSpanHeightToAutoRows(RenderTableCell* cell, int totalAutoRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalAutoRowsHeight)
+        return;
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+    int changedPosBy = 0;

[Julien - ping for review, 2013/07/13 00:56:36]

Not a fan of |changedPosBy|, how about |accumulatedPositionIncrease|.

[a.suchit, 2013/07/15 12:06:19]

Done.

+
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (m_grid[row].logicalHeight.isAuto())
+            changedPosBy += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight;
+        m_rowPos[row + 1] += changedPosBy;
+    }
+    // FIXME: Remaining height is very less which can not be distributed in rows so added it in the last spanning row
+    // and it save from assert used in caller.
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+
+    // Full height is distributed in auto rows so make it 0.

[Julien - ping for review, 2013/07/13 00:56:36]

Remember my asking always about *why*, that's another case of a what comment
that people can make from the code without help.

Btw, you code will probably be off in most cases as you are doing an integer
division above to split the extra space. That's the reason why
RenderTableSection::distributeExtraLogicalHeightToAutoRows updates the
|extraLogicalHeight| in the for loop.

[a.suchit, 2013/07/15 12:06:19]

Done.

+    extraRowSpanningHeight = 0;
+}
+
+void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalRemainingRowsHeight)
+        return;
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+    int changedPosBy = 0;
+
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (!m_grid[row].logicalHeight.isPercent())
+            changedPosBy += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalRemainingRowsHeight;
+        m_rowPos[row + 1] += changedPosBy;
+    }
+    // FIXME: Remaining height is very less which can not be distributed in rows so added it in the last spanning row
+    // and it save from assert used in caller.
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+
+    // Full height is distributed in rows so make it 0.
+    extraRowSpanningHeight = 0;

[Julien - ping for review, 2013/07/13 00:56:36]

What guarantees that?

[a.suchit, 2013/07/15 12:06:19]

We are distributing the whole extra row spanning height in spanning rows based
on rows weightage.
I some cases, very small amount of height will remain which is added in last
spanning row.

+}
+
 // 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)
@@ -265,38 +354,40 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
     RenderTableCell* cell = rowSpanCells[0];
 
     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);
+    struct SpanningRowsHeight spanningRowsHeight;
 
-    // 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);
+    calcRowsHeightInRowSpan(cell, spanningRowsHeight);
 
-    if (!totalRowsHeight || rowSpanCellHeight <= totalRowsHeight)
+    if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.expectedTotalRowsHeight <= spanningRowsHeight.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;
+    unsigned rowIndex = cell->rowIndex();
+    int totalPercent = 0;
+    int totalAutoRowsHeight = 0;
+    int totalRemainingRowsHeight = spanningRowsHeight.totalRowsHeight;
 
+    // Calculate total percentage, total auto rows height and total rows height except percent rows.
     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();
+            totalRemainingRowsHeight -= spanningRowsHeight.rowHeight[row - rowIndex];
+        } else if (m_grid[row].logicalHeight.isAuto()) {
+            totalAutoRowsHeight += spanningRowsHeight.rowHeight[row - rowIndex];
+        }
     }
-    // Remaining height added in the last row under rowSpan cell
-    m_rowPos[rowIndex + rowSpan] += remainingHeight;
+
+    int initialPos = m_rowPos[rowIndex + rowSpan];
+    int extraRowSpanningHeight = spanningRowsHeight.expectedTotalRowsHeight - spanningRowsHeight.totalRowsHeight;
+
+    distributeExtraRowSpanHeightToPrecentRows(cell, m_rowPos[m_grid.size()], totalPercent, extraRowSpanningHeight);
+    distributeExtraRowSpanHeightToAutoRows(cell, totalAutoRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
+    distributeExtraRowSpanHeightToRemainingRows(cell, totalRemainingRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
+
+    ASSERT(!extraRowSpanningHeight);

[Julien - ping for review, 2013/07/13 00:56:36]

This ASSERT is neutered by distributeExtraRowSpanHeightToRemainingRows :(

[a.suchit, 2013/07/15 12:06:19]

Full height is distributed in the spanning rows by adding remaining very small
height in the last row spanning cell. So Assert is removed.

 
     // 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();