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..f3b81a57a2c4370c9c5ffef8445e8213203065b9 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -252,24 +252,14 @@ 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)

[Julien - ping for review, 2013/07/02 02:16:18]

This function starts to smell badly as you now have 2 in-parameters and one
out-parameter. That makes it hard to see what's going on.

[a.suchit, 2013/07/03 12:59:57]

Done.

 {
-    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 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 +268,134 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
     }
     rowSpanCellHeight += borderSpacingForRow(rowIndex + rowSpan - 1);
 
-    if (!totalRowsHeight || rowSpanCellHeight <= totalRowsHeight)
+    expectedTotalRowsHeight = rowSpanCellHeight;
+
+    return totalRowsHeight;
+}
+
+// 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.

[Julien - ping for review, 2013/07/02 02:16:18]

I read this several times and failed to understand it. You should make your code
self-explanatory and remove those comments that just distract people from the
actual code.

+void RenderTableSection::distributeExtraRowSpanHeightToPrecentRows(RenderTableCell* cell, int totalPercent, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalPercent)
+        return;
+
+    unsigned rowSpan = cell->rowSpan();
+    unsigned rowIndex = cell->rowIndex();
+
+    if (100 <= totalPercent) {

[Julien - ping for review, 2013/07/02 02:16:18]

I had a hard time understanding this check and believe it's probably wrong in
the general case: you are making an assumption that you can just handle the
percentage locally at the current cell's level.

If your table is above 100% but not your cell's rows, your percentage will be
off as percentages refers to the whole table's dimension.

[a.suchit, 2013/07/03 12:59:57]

At this point, total table height is not fixed so we are taking percent based on
the current rowspan cell's level.
At the end of table layout, row's height percent are handled saperatly based on
table height if table have extra logical height.

[Julien - ping for review, 2013/07/03 21:08:22]

I am still not totally convinced this is correct to ignore other percentage for
that but I would have to come up with a good counter-argument if I can find one.
That's a separate piece of code and not one that has much bearing on this change
(apart from the similarity).

+        int changedPosBy = 0;
+        for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+            if (m_grid[row].logicalHeight.isPercent())
+                changedPosBy += extraRowSpanningHeight * 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] += extraRowSpanningHeight - changedPosBy;
+        extraRowSpanningHeight = 0;

[Julien - ping for review, 2013/07/02 02:16:18]

Again, it is WRONG to clear extraRowSpanningHeight as you are neutering your
ASSERT. If you have rounding issues that make the ASSERT trigger, they should be
fixed.

[a.suchit, 2013/07/03 12:59:57]

Done.

+    } else {
+        int changedPosBy = 0;
+        for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+            if (m_grid[row].logicalHeight.isPercent())
+                changedPosBy += extraRowSpanningHeight * m_grid[row].logicalHeight.percent() / 100;
+            m_rowPos[row + 1] += changedPosBy;
+        }
+
+        extraRowSpanningHeight -= changedPosBy;
+    }
+}
+
+// If auto rows are present then whole extra height would be distribute in auto rows based on their height ratios so that
+// all auto rows would get its own share from extra height.
+// Else extra height would be distributed in other rows.
+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;
+
+    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;
+    }
+    // Remaining height added in the last row under rowSpan cell
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+
+    extraRowSpanningHeight = 0;

[Julien - ping for review, 2013/07/02 02:16:18]

This is wrong to clear extraRowSpanningHeight unconditionally here.

[a.suchit, 2013/07/03 12:59:57]

Done.

+}
+// Whole extra height would be distribute in  remaining rows based on their height ratios so that
+// all rows would get its own share from extra height.
+void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)

[Julien - ping for review, 2013/07/02 02:16:18]

Because of extraRowSpanningHeight = 0; above, this function is pretty much dead
code.

[a.suchit, 2013/07/03 12:59:57]

If rowspan cell does not contain auto rows then extra height would be
distributed in all rows.

[a.suchit, 2013/07/03 12:59:57]

Done.

+{
+    if (!extraRowSpanningHeight || !totalRemainingRowsHeight)
         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 changedPosBy = 0;
 
     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())
+            changedPosBy += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalRemainingRowsHeight;
+        m_rowPos[row + 1] += changedPosBy;

[Julien - ping for review, 2013/07/02 02:16:18]

The more I see these lines that basically mandates O(n) behavior *all the time*,
the more I think we should have some better data structure for layout. The issue
with rowPos is that it stores the absolute position, not the row's heights which
we manipulate heavily during layout.

[a.suchit, 2013/07/03 12:59:57]

m_rowPos is already handled to store the absolute positions, so if we will go to
change it then it will heavily affect all the existing places and we need to
change all over the places.

[Julien - ping for review, 2013/07/03 21:08:22]

That's an understatement! However this argument shouldn't prevent us from doing
the right thing.

What you don't account for is that it would probably remove a lot of complexity
in your change and the layout code.

     }
     // Remaining height added in the last row under rowSpan cell
-    m_rowPos[rowIndex + rowSpan] += remainingHeight;
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight - changedPosBy;
+
+    extraRowSpanningHeight = 0;

[Julien - ping for review, 2013/07/02 02:16:18]

This is just gross. Nothing guarantees that changedPosBy == 
extraRowSpanningHeight and thus you are making the
ASSERT(!extraRowSpanningHeight) useless because of this change.

[a.suchit, 2013/07/03 12:59:57]

Done.

+}
+
+// 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();
+
+    int expectedTotalRowsHeight = 0;
+    Vector<int> rowsHeight(rowSpan);
+
+    int totalRowsHeight = getRowsHeightInRowSpan(cell, expectedTotalRowsHeight, rowsHeight);
+
+    if (!totalRowsHeight || expectedTotalRowsHeight <= totalRowsHeight)
+        return;
+
+    unsigned rowIndex = cell->rowIndex();
+    int totalPercent = 0;
+    int totalAutoRowsHeight = 0;
+    int totalRemainingRowsHeight = totalRowsHeight;
+
+    // Calculate total percentage, total auto rows height and total rows height except percent rows.
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (m_grid[row].logicalHeight.isPercent()) {
+            totalPercent += m_grid[row].logicalHeight.percent();
+            totalRemainingRowsHeight -= rowsHeight[row - rowIndex];
+        } else if (m_grid[row].logicalHeight.isAuto()) {
+            totalAutoRowsHeight += rowsHeight[row - rowIndex];
+        }
+    }
+
+    int initialPos = m_rowPos[rowIndex + rowSpan];
+    int extraRowSpanningHeight = expectedTotalRowsHeight - totalRowsHeight;
+
+    distributeExtraRowSpanHeightToPrecentRows(cell, totalPercent, extraRowSpanningHeight, rowsHeight);
+    distributeExtraRowSpanHeightToAutoRows(cell, totalAutoRowsHeight, extraRowSpanningHeight, rowsHeight);
+    distributeExtraRowSpanHeightToRemainingRows(cell, totalRemainingRowsHeight, extraRowSpanningHeight, rowsHeight);
+
+    ASSERT(!extraRowSpanningHeight);
 
     // 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();