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 237 matching lines @@
             if (inColSpan)
                 c.inColSpan = true;
         }
         m_cCol++;
         cSpan -= currentSpan;
         inColSpan = true;
     }
     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)
 {
     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);
+    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();
 
         // 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 1234 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