Clean-up RenderTableSection::calcRowLogicalHeight

https://bugs.webkit.org/show_bug.cgi?id=77705

Reviewed by Nikolas Zimmermann.

Refactoring / simplication of the code.

This change removes some variables that were unneeded and were
hiding what the code was really doing. Also some of the code was
split and moved down to RenderTableCell.

* rendering/RenderTableCell.cpp:
(WebCore::RenderTableCell::logicalHeightForRowSizing):
* rendering/RenderTableCell.h:
(RenderTableCell):
Added the previous helper function to calculate the cell's
adjusted logical height.

* rendering/RenderTableSection.cpp:
(WebCore::RenderTableSection::calcRowLogicalHeight):
Removed some variables, simplified the rowspan logic to be clearer
(and added a comment about how we handle rowspans).


Canonical link: https://commits.webkit.org/96684@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@108914 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,28 @@
+2012-02-25  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Clean-up RenderTableSection::calcRowLogicalHeight
+        https://bugs.webkit.org/show_bug.cgi?id=77705
+
+        Reviewed by Nikolas Zimmermann.
+
+        Refactoring / simplication of the code.
+
+        This change removes some variables that were unneeded and were
+        hiding what the code was really doing. Also some of the code was
+        split and moved down to RenderTableCell.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::logicalHeightForRowSizing):
+        * rendering/RenderTableCell.h:
+        (RenderTableCell):
+        Added the previous helper function to calculate the cell's
+        adjusted logical height.
+
+        * rendering/RenderTableSection.cpp:
+        (WebCore::RenderTableSection::calcRowLogicalHeight):
+        Removed some variables, simplified the rowspan logic to be clearer
+        (and added a comment about how we handle rowspans).
+
 2012-02-25  Benjamin Poulain  <benjamin@webkit.org>
 
         Add an empty skeleton of KURL for WTFURL

Source/WebCore/rendering/RenderTableCell.cpp

@@ -91,6 +91,25 @@ void RenderTableCell::colSpanOrRowSpanChanged()
         section()->setNeedsCellRecalc();
 }
 
+LayoutUnit RenderTableCell::logicalHeightForRowSizing() const
+{
+    LayoutUnit adjustedLogicalHeight = logicalHeight() - (intrinsicPaddingBefore() + intrinsicPaddingAfter());
+
+    LayoutUnit styleLogicalHeight = style()->logicalHeight().calcValue(0);
+    if (document()->inQuirksMode() || style()->boxSizing() == BORDER_BOX) {
+        // Explicit heights use the border box in quirks mode.
+        // Don't adjust height.
+    } else {
+        // In strict mode, box-sizing: content-box do the right
+        // thing and actually add in the border and padding.
+        LayoutUnit adjustedPaddingBefore = paddingBefore() - intrinsicPaddingBefore();
+        LayoutUnit adjustedPaddingAfter = paddingAfter() - intrinsicPaddingAfter();
+        styleLogicalHeight += adjustedPaddingBefore + adjustedPaddingAfter + borderBefore() + borderAfter();
+    }
+
+    return max(styleLogicalHeight, adjustedLogicalHeight);
+}
+
 Length RenderTableCell::styleOrColLogicalWidth() const
 {
     Length w = style()->logicalWidth();

Source/WebCore/rendering/RenderTableCell.h

@@ -85,6 +85,8 @@ class RenderTableCell : public RenderBlock {
 
     Length styleOrColLogicalWidth() const;
 
+    LayoutUnit logicalHeightForRowSizing() const;
+
     virtual void computePreferredLogicalWidths();
 
     void updateLogicalWidth(LayoutUnit);

Source/WebCore/rendering/RenderTableSection.cpp

@@ -334,14 +334,11 @@ int RenderTableSection::calcRowLogicalHeight()
     m_rowPos[0] = spacing;
 
     for (unsigned r = 0; r < m_grid.size(); r++) {
-        m_rowPos[r + 1] = 0;
         m_grid[r].baseline = 0;
-        LayoutUnit baseline = 0;
-        int bdesc = 0;
-        int ch = m_grid[r].logicalHeight.calcMinValue(0);
-        int pos = m_rowPos[r] + ch + (m_grid[r].rowRenderer ? spacing : 0);
+        LayoutUnit baselineDescent = 0;
 
-        m_rowPos[r + 1] = max(m_rowPos[r + 1], pos);
+        // Our base size is the biggest logical height from our cells' styles (excluding row spanning cells).
+        m_rowPos[r + 1] = max(m_rowPos[r] + m_grid[r].logicalHeight.calcMinValue(0), 0);
 
         Row& row = m_grid[r].row;
         unsigned totalCols = row.size();
@@ -353,10 +350,13 @@ int RenderTableSection::calcRowLogicalHeight()
             if (!cell || current.inColSpan)
                 continue;
 
-            if ((cell->row() + cell->rowSpan() - 1) > r)
+            // FIXME: We are always adding the height of a rowspan to the last rows which doesn't match
+            // other browsers. See webkit.org/b/52185 for example.
+            if ((cell->row() + cell->rowSpan() - 1) != r)
                 continue;
 
-            unsigned indx = max(r - cell->rowSpan() + 1, 0u);
+            // For row spanning cells, |r| is the last row in the span.
+            unsigned cellStartRow = cell->row();
 
             if (cell->hasOverrideHeight()) {
                 if (!statePusher.didPush()) {
@@ -370,43 +370,27 @@ int RenderTableSection::calcRowLogicalHeight()
                 cell->layoutIfNeeded();
             }
 
-            int adjustedLogicalHeight = cell->logicalHeight() - (cell->intrinsicPaddingBefore() + cell->intrinsicPaddingAfter());
-
-            ch = cell->style()->logicalHeight().calcValue(0);
-            if (document()->inQuirksMode() || cell->style()->boxSizing() == BORDER_BOX) {
-                // Explicit heights use the border box in quirks mode.
-                // Don't adjust height.
-            } else {
-                // In strict mode, box-sizing: content-box do the right
-                // thing and actually add in the border and padding.
-                int adjustedPaddingBefore = cell->paddingBefore() - cell->intrinsicPaddingBefore();
-                int adjustedPaddingAfter = cell->paddingAfter() - cell->intrinsicPaddingAfter();
-                ch += adjustedPaddingBefore + adjustedPaddingAfter + cell->borderBefore() + cell->borderAfter();
-            }
-            ch = max(ch, adjustedLogicalHeight);
-
-            pos = m_rowPos[indx] + ch + (m_grid[r].rowRenderer ? spacing : 0);
-
-            m_rowPos[r + 1] = max(m_rowPos[r + 1], pos);
+            int cellLogicalHeight = cell->logicalHeightForRowSizing();
+            m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[cellStartRow] + cellLogicalHeight);
 
             // find out the baseline
             EVerticalAlign va = cell->style()->verticalAlign();
             if (va == BASELINE || va == TEXT_BOTTOM || va == TEXT_TOP || va == SUPER || va == SUB) {
-                int b = cell->cellBaselinePosition();
-                if (b > cell->borderBefore() + cell->paddingBefore()) {
-                    baseline = max<LayoutUnit>(baseline, b - cell->intrinsicPaddingBefore());
-                    bdesc = max(bdesc, m_rowPos[indx] + ch - (b - cell->intrinsicPaddingBefore()));
+                int baselinePosition = cell->cellBaselinePosition();
+                if (baselinePosition > cell->borderBefore() + cell->paddingBefore()) {
+                    m_grid[r].baseline = max(m_grid[r].baseline, baselinePosition - cell->intrinsicPaddingBefore());
+                    baselineDescent = max(baselineDescent, m_rowPos[cellStartRow] + cellLogicalHeight - (baselinePosition - cell->intrinsicPaddingBefore()));
                 }
             }
         }
 
         // do we have baseline aligned elements?
-        if (baseline) {
+        if (m_grid[r].baseline)
             // increase rowheight if baseline requires
-            m_rowPos[r + 1] = max(m_rowPos[r + 1], baseline + bdesc + (m_grid[r].rowRenderer ? spacing : 0));
-            m_grid[r].baseline = baseline;
-        }
+            m_rowPos[r + 1] = max(m_rowPos[r + 1], m_grid[r].baseline + baselineDescent);
 
+        // Add the border-spacing to our final position.
+        m_rowPos[r + 1] += m_grid[r].rowRenderer ? spacing : 0;
         m_rowPos[r + 1] = max(m_rowPos[r + 1], m_rowPos[r]);
     }