Table rows are incorrectly collapsed in case of hidden cells and rowspans.

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 bd7863a888193166d5e2e6691dcad701864ba5af..d3cc69da5037fecc910f3e33ec5cfcdd08d9920d 100644
--- a/Source/core/rendering/RenderTableSection.cpp
+++ b/Source/core/rendering/RenderTableSection.cpp
@@ -288,8 +288,8 @@ void RenderTableSection::populateSpanningRowsHeightFromCell(RenderTableCell* cel
         unsigned actualRow = row + rowIndex;
 
         spanningRowsHeight.rowHeight[row] = m_rowPos[actualRow + 1] - m_rowPos[actualRow] - borderSpacingForRow(actualRow);
-        if (!spanningRowsHeight.rowHeight[row])
-            spanningRowsHeight.rowWithOnlySpanningCells |= rowHasOnlySpanningCells(actualRow);
+        if (!spanningRowsHeight.rowWithOnlySpanningCells && !spanningRowsHeight.rowHeight[row])

[Julien - ping for review, 2014/02/06 20:30:45]

You misunderstood what I said: this code is equivalent to the previous one and I
was fine with using a logical OR. I don't see the value in this part of the
change.

If you name this variable rowWithOnlySpanningCells, it means that any row that
*doesn't* have spanning cells should turn it to false but here it's the
opposite. That makes wonder if it's not wrongly named.

[a.suchit, 2014/02/18 13:07:10]

We are checking all the rows of rowspanning cell. If any row is having only
spanning cells and empty cells are considered as non rowspanning cell then
|rowWithOnlySpanningCells| becomes true.

At least one spanning row is having only spanning cells than
|rowWithOnlySpanningCells| becomes true

+            spanningRowsHeight.rowWithOnlySpanningCells = rowHasOnlySpanningCells(actualRow);
 
         spanningRowsHeight.totalRowsHeight += spanningRowsHeight.rowHeight[row];
         spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing -= borderSpacingForRow(actualRow);
@@ -542,14 +542,28 @@ void RenderTableSection::distributeRowSpanHeightToRows(SpanningRenderTableCells&
 
         populateSpanningRowsHeightFromCell(cell, spanningRowsHeight);
 
+        // if only spanning cells are present in spanning rows then apply height based on spanning cells.
+        // Here we are handling only row(s) who have only rowspanning cells and do not have any empty cell.
         if (spanningRowsHeight.rowWithOnlySpanningCells)
             updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
 
-        if (!spanningRowsHeight.totalRowsHeight || spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
+        // else if total height of spanning rows are 0 then apply rowspanning cells height here.
+        // Here we are handling only row(s) who have rowspanning cell(s) and at least one empty cell.

[Julien - ping for review, 2014/02/06 20:30:45]

I tried reading those comments several times but as I keep pointing out, they
are just mirroring the code and thus don't add value. What we need is 'why'
comments (why is it correct to do that?) or comments adding values to our code
('what' comments that go beyond just the code by giving easy to understand
examples or helping the reader follow the high level algorithm).

[a.suchit, 2014/02/18 13:07:10]

Done.

+        if (!spanningRowsHeight.totalRowsHeight) {
+            if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing)
+                m_rowPos[rowIndex + rowSpan] += spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing + borderSpacingForRow(rowIndex + rowSpan - 1);

[Julien - ping for review, 2014/02/06 20:30:45]

This should be spanningCellEndIndex for consistency.

[a.suchit, 2014/02/18 13:07:10]

Done.

+
+            extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;
+            continue;

[Julien - ping for review, 2014/02/06 20:30:45]

My biggest issue with this code is that it will wrongly handle the case where
you only have spanning cells maybe with empty cells and some set row's height.

Also it is allocating the extra space to the last row in a rowspan
(m_rowPos[rowIndex + rowSpan]).

+        }
+
+        if (spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <= spanningRowsHeight.totalRowsHeight) {
             extraHeightToPropagate = m_rowPos[rowIndex + rowSpan] - originalBeforePosition;

[Julien - ping for review, 2014/02/06 20:30:45]

How does this ensures that we allocate all of
spanningCellHeightIgnoringBorderSpacing?

[a.suchit, 2014/02/18 13:07:10]

Spanning rows total height is already more then the spanning cell height with
ignoring border space.
So If spanning cell already have enough height which is equal or more than
specified by user for this spanning cell then we are leaving spanning cell with
same height.

             continue;
         }
 
+        // else distribute extra height in percent rows, auto rows and fixed rows respectively.
+        // Below we are handling only row(s) who have at least one visible cell without rowspan value.
         int totalPercent = 0;
         int totalAutoRowsHeight = 0;
         int totalRemainingRowsHeight = spanningRowsHeight.totalRowsHeight;