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)

[Julien - ping for review, 2013/07/16 17:22:13]

Not a huge fan of the name (calc is an abbreviation that doesn't save us much),
how about populateSpanningRowsHeightFromCell?

[a.suchit, 2013/07/18 17:05:32]

Done.

+{
+    const unsigned rowSpan = cell->rowSpan();
+    const unsigned rowIndex = cell->rowIndex();
+
+    spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing = cell->logicalHeightForRowSizing();
+
+    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.spanningCellHeightIgnoringBorderSpacing -= borderSpacingForRow(actualRow);
+    }
+    spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing += borderSpacingForRow(rowIndex + rowSpan - 1);

[Julien - ping for review, 2013/07/16 17:22:13]

This is typically the line of code that would require some explanations for the
reader's sake. Based on what it's supposed to do here is a canvas:

// We don't span the following row so its border-spacing (if any) should be
included.

[a.suchit, 2013/07/18 17:05:32]

Done.

+}
+
+void RenderTableSection::distributeExtraRowSpanHeightToPercentRows(RenderTableCell* cell, int tableHeight, int totalPercent, int& extraRowSpanningHeight)
+{
+    if (!extraRowSpanningHeight || !totalPercent)
+        return;
+
+    const unsigned rowSpan = cell->rowSpan();
+    const unsigned rowIndex = cell->rowIndex();
+    int percent = min(totalPercent, 100);
+    int remainingRowSpanningHeight = extraRowSpanningHeight;

[Julien - ping for review, 2013/07/16 17:22:13]

tableHeight + extraRowSpanningHeight is the only reason you need
|remainingRowSpanningHeight|. It's also a constant that you can store upfront.

[a.suchit, 2013/07/18 17:05:32]

Done.

+
+    int accumulatedPositionIncrease = 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;

[Julien - ping for review, 2013/07/16 17:22:13]

The whole algorithm really need some explanations, like saying why is it correct
for totalPercent > 100 would definitely help.

You stripped all comments but the comments I was against were "what" comment
describing what the code was doing in a bad way. Explaining why we do it this
way is very valuable (e.g. why did we chose this way of spreading the logical
height? ...)

+
+                toAdd = min(toAdd, remainingRowSpanningHeight);
+                accumulatedPositionIncrease += toAdd;
+                remainingRowSpanningHeight -= toAdd;
+                percent -= m_grid[row].logicalHeight.percent();
+            }
+        }
+        m_rowPos[row + 1] += accumulatedPositionIncrease;
+    }
+
+    extraRowSpanningHeight = remainingRowSpanningHeight;
+}
+
+void RenderTableSection::distributeExtraRowSpanHeightToAutoRows(RenderTableCell* cell, int totalAutoRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalAutoRowsHeight)
+        return;
+
+    const unsigned rowSpan = cell->rowSpan();
+    const unsigned rowIndex = cell->rowIndex();
+    int accumulatedPositionIncrease = 0;
+
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (m_grid[row].logicalHeight.isAuto())
+            accumulatedPositionIncrease += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalAutoRowsHeight;
+        m_rowPos[row + 1] += accumulatedPositionIncrease;
+    }
+
+    extraRowSpanningHeight -= accumulatedPositionIncrease;
+
+    // FIXME: In some cases extraRowSpanning height is remain which is very small and cannot be again distributed in
+    // spanning rows. So this remaining height added to last spanning row to maintain the spanning cell height and
+    // set extra row spanning height to 0.
+    // Value should be round properly in the algorithm.
+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight;
+    extraRowSpanningHeight = 0;
+}
+
+void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight)
+{
+    if (!extraRowSpanningHeight || !totalRemainingRowsHeight)
+        return;
+
+    const unsigned rowSpan = cell->rowSpan();
+    const unsigned rowIndex = cell->rowIndex();
+    int accumulatedPositionIncrease = 0;
+
+    for (unsigned row = rowIndex; row < (rowIndex + rowSpan); row++) {
+        if (!m_grid[row].logicalHeight.isPercent())
+            accumulatedPositionIncrease += (extraRowSpanningHeight * rowsHeight[row - rowIndex]) / totalRemainingRowsHeight;
+        m_rowPos[row + 1] += accumulatedPositionIncrease;
+    }
+
+    extraRowSpanningHeight -= accumulatedPositionIncrease;
+
+    // FIXME: In some cases extraRowSpanning height is remain which is very small and cannot be again distributed in
+    // spanning rows. So this remaining height added to last spanning row to maintain the spanning cell height and
+    // set extra row spanning height to 0.
+    // Value should be round properly in the algorithm.

[Julien - ping for review, 2013/07/16 17:22:13]

Can't we do the rounding properly instead of swiping the dust under the carpet
here?

[a.suchit, 2013/07/18 17:05:32]

Done.

+    m_rowPos[rowIndex + rowSpan] += extraRowSpanningHeight;
+    extraRowSpanningHeight = 0;
+}
+
 // 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.spanningCellHeightIgnoringBorderSpacing <= 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.spanningCellHeightIgnoringBorderSpacing - spanningRowsHeight.totalRowsHeight;
+
+    distributeExtraRowSpanHeightToPercentRows(cell, m_rowPos[m_grid.size()], totalPercent, extraRowSpanningHeight);
+    distributeExtraRowSpanHeightToAutoRows(cell, totalAutoRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
+    distributeExtraRowSpanHeightToRemainingRows(cell, totalRemainingRowsHeight, extraRowSpanningHeight, spanningRowsHeight.rowHeight);
 

[Julien - ping for review, 2013/07/16 17:22:13]

The ASSERT was really important, what I wasn't happy with is overriding
extraRowSpanningHeight to 0 (which you are still doing :(), which is just
weakening it.

[a.suchit, 2013/07/18 17:05:32]

Done.

     // 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