Split the intrinsic padding update code out of RenderTableSection::layoutRows
authorjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Oct 2012 03:56:17 +0000 (03:56 +0000)
committerjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Oct 2012 03:56:17 +0000 (03:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=98454

Reviewed by Eric Seidel.

RenderTableSection::layoutRows is very long and it's difficult to see what's
going on. This change moves the intrinsic padding update code into RenderTableCell
for clarity. While at it, cleaned up a bit the code (renaming variables, functions).

Change covered by existing table tests.

* rendering/RenderTableCell.cpp:
(WebCore::RenderTableCell::computeIntrinsicPadding):
Added this new function that does the update. Removed the 'default' case, replaced by
the explicit label BASELINE_MIDDLE.

* rendering/RenderTableCell.h:
(WebCore::RenderTableCell::setIntrinsicPaddingBefore):
(WebCore::RenderTableCell::setIntrinsicPaddingAfter):
(WebCore::RenderTableCell::setIntrinsicPadding):
Moved those setters to the private section as we want other classes
to use computeIntrinsicPadding.

* rendering/RenderTableSection.cpp:
(WebCore::RenderTableSection::layoutRows):
Replaced the code with a call to RenderTableCell::computeIntrinsicPadding.
Also moved 2 variables in the loop per our coding style.

* rendering/RenderTableSection.h:
(WebCore::RenderTableSection::rowBaseline):
Renamed to match our coding style.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@130454 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderTableCell.cpp
Source/WebCore/rendering/RenderTableCell.h
Source/WebCore/rendering/RenderTableSection.cpp
Source/WebCore/rendering/RenderTableSection.h

index 95b12c7..ec6eb3d 100644 (file)
@@ -1,3 +1,37 @@
+2012-10-04  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Split the intrinsic padding update code out of RenderTableSection::layoutRows
+        https://bugs.webkit.org/show_bug.cgi?id=98454
+
+        Reviewed by Eric Seidel.
+
+        RenderTableSection::layoutRows is very long and it's difficult to see what's
+        going on. This change moves the intrinsic padding update code into RenderTableCell
+        for clarity. While at it, cleaned up a bit the code (renaming variables, functions).
+
+        Change covered by existing table tests.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::computeIntrinsicPadding):
+        Added this new function that does the update. Removed the 'default' case, replaced by
+        the explicit label BASELINE_MIDDLE.
+
+        * rendering/RenderTableCell.h:
+        (WebCore::RenderTableCell::setIntrinsicPaddingBefore):
+        (WebCore::RenderTableCell::setIntrinsicPaddingAfter):
+        (WebCore::RenderTableCell::setIntrinsicPadding):
+        Moved those setters to the private section as we want other classes
+        to use computeIntrinsicPadding.
+
+        * rendering/RenderTableSection.cpp:
+        (WebCore::RenderTableSection::layoutRows):
+        Replaced the code with a call to RenderTableCell::computeIntrinsicPadding.
+        Also moved 2 variables in the loop per our coding style.
+
+        * rendering/RenderTableSection.h:
+        (WebCore::RenderTableSection::rowBaseline):
+        Renamed to match our coding style.
+
 2012-10-04  Nate Chapin  <japhet@chromium.org>
 
         Crash in EventHandler::mouseMoved().
index 944a90c..0b7a80d 100644 (file)
@@ -198,6 +198,47 @@ void RenderTableCell::computePreferredLogicalWidths()
     }
 }
 
+void RenderTableCell::computeIntrinsicPadding(int rowHeight)
+{
+    int oldIntrinsicPaddingBefore = intrinsicPaddingBefore();
+    int oldIntrinsicPaddingAfter = intrinsicPaddingAfter();
+    int logicalHeightWithoutIntrinsicPadding = pixelSnappedLogicalHeight() - oldIntrinsicPaddingBefore - oldIntrinsicPaddingAfter;
+
+    int intrinsicPaddingBefore = 0;
+    switch (style()->verticalAlign()) {
+    case SUB:
+    case SUPER:
+    case TEXT_TOP:
+    case TEXT_BOTTOM:
+    case LENGTH:
+    case BASELINE: {
+        LayoutUnit baseline = cellBaselinePosition();
+        if (baseline > borderBefore() + paddingBefore())
+            intrinsicPaddingBefore = section()->rowBaseline(rowIndex()) - (baseline - oldIntrinsicPaddingBefore);
+        break;
+    }
+    case TOP:
+        break;
+    case MIDDLE:
+        intrinsicPaddingBefore = (rowHeight - logicalHeightWithoutIntrinsicPadding) / 2;
+        break;
+    case BOTTOM:
+        intrinsicPaddingBefore = rowHeight - logicalHeightWithoutIntrinsicPadding;
+        break;
+    case BASELINE_MIDDLE:
+        break;
+    }
+
+    int intrinsicPaddingAfter = rowHeight - logicalHeightWithoutIntrinsicPadding - intrinsicPaddingBefore;
+    setIntrinsicPaddingBefore(intrinsicPaddingBefore);
+    setIntrinsicPaddingAfter(intrinsicPaddingAfter);
+
+    // FIXME: Changing an intrinsic padding shouldn't trigger a relayout as it only shifts the cell inside the row but
+    // doesn't change the logical height.
+    if (intrinsicPaddingBefore != oldIntrinsicPaddingBefore || intrinsicPaddingAfter != oldIntrinsicPaddingAfter)
+        setNeedsLayout(true, MarkOnlyThis);
+}
+
 void RenderTableCell::updateLogicalWidth()
 {
 }
index 45db7aa..e876419 100644 (file)
@@ -99,9 +99,7 @@ public:
 
     LayoutUnit cellBaselinePosition() const;
 
-    void setIntrinsicPaddingBefore(int p) { m_intrinsicPaddingBefore = p; }
-    void setIntrinsicPaddingAfter(int p) { m_intrinsicPaddingAfter = p; }
-    void setIntrinsicPadding(int before, int after) { setIntrinsicPaddingBefore(before); setIntrinsicPaddingAfter(after); }
+    void computeIntrinsicPadding(int rowHeight);
     void clearIntrinsicPadding() { setIntrinsicPadding(0, 0); }
 
     int intrinsicPaddingBefore() const { return m_intrinsicPaddingBefore; }
@@ -210,6 +208,10 @@ private:
     int borderHalfBefore(bool outer) const;
     int borderHalfAfter(bool outer) const;
 
+    void setIntrinsicPaddingBefore(int p) { m_intrinsicPaddingBefore = p; }
+    void setIntrinsicPaddingAfter(int p) { m_intrinsicPaddingAfter = p; }
+    void setIntrinsicPadding(int before, int after) { setIntrinsicPaddingBefore(before); setIntrinsicPaddingAfter(after); }
+
     CollapsedBorderValue collapsedStartBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
     CollapsedBorderValue collapsedEndBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
     CollapsedBorderValue collapsedBeforeBorder(IncludeBorderColorOrNot = IncludeBorderColor) const;
index eb69489..6c9e335 100644 (file)
@@ -516,8 +516,6 @@ void RenderTableSection::layoutRows()
 
     ASSERT(!needsLayout());
 
-    int rHeight;
-    unsigned rindx;
     unsigned totalRows = m_grid.size();
 
     // Set the width of our section now.  The rows will also be this width.
@@ -549,9 +547,9 @@ void RenderTableSection::layoutRows()
             if (!cell || cs.inColSpan)
                 continue;
 
-            rindx = cell->rowIndex();
-            rHeight = m_rowPos[rindx + cell->rowSpan()] - m_rowPos[rindx] - vspacing;
-            
+            int rowIndex = cell->rowIndex();
+            int rHeight = m_rowPos[rowIndex + cell->rowSpan()] - m_rowPos[rowIndex] - vspacing;
+
             // Force percent height children to lay themselves out again.
             // This will cause these children to grow to fill the cell.
             // FIXME: There is still more work to do here to fully match WinIE (should
@@ -616,46 +614,12 @@ void RenderTableSection::layoutRows()
                 }
             }
 
-            int oldIntrinsicPaddingBefore = cell->intrinsicPaddingBefore();
-            int oldIntrinsicPaddingAfter = cell->intrinsicPaddingAfter();
-            int logicalHeightWithoutIntrinsicPadding = cell->pixelSnappedLogicalHeight() - oldIntrinsicPaddingBefore - oldIntrinsicPaddingAfter;
-
-            int intrinsicPaddingBefore = 0;
-            switch (cell->style()->verticalAlign()) {
-                case SUB:
-                case SUPER:
-                case TEXT_TOP:
-                case TEXT_BOTTOM:
-                case LENGTH:
-                case BASELINE: {
-                    LayoutUnit baseline = cell->cellBaselinePosition();
-                    if (baseline > cell->borderBefore() + cell->paddingBefore())
-                        intrinsicPaddingBefore = getBaseline(cell->rowIndex()) - (baseline - oldIntrinsicPaddingBefore);
-                    break;
-                }
-                case TOP:
-                    break;
-                case MIDDLE:
-                    intrinsicPaddingBefore = (rHeight - logicalHeightWithoutIntrinsicPadding) / 2;
-                    break;
-                case BOTTOM:
-                    intrinsicPaddingBefore = rHeight - logicalHeightWithoutIntrinsicPadding;
-                    break;
-                default:
-                    break;
-            }
-            
-            int intrinsicPaddingAfter = rHeight - logicalHeightWithoutIntrinsicPadding - intrinsicPaddingBefore;
-            cell->setIntrinsicPaddingBefore(intrinsicPaddingBefore);
-            cell->setIntrinsicPaddingAfter(intrinsicPaddingAfter);
+            cell->computeIntrinsicPadding(rHeight);
 
             LayoutRect oldCellRect = cell->frameRect();
 
             setLogicalPositionForCell(cell, c);
 
-            if (intrinsicPaddingBefore != oldIntrinsicPaddingBefore || intrinsicPaddingAfter != oldIntrinsicPaddingAfter)
-                cell->setNeedsLayout(true, MarkOnlyThis);
-
             if (!cell->needsLayout() && view()->layoutState()->pageLogicalHeight() && view()->layoutState()->pageLogicalOffset(cell, cell->logicalTop()) != cell->pageLogicalOffset())
                 cell->setChildNeedsLayout(true, MarkOnlyThis);
 
index ce21a9e..02a5299 100644 (file)
@@ -173,7 +173,7 @@ public:
     bool needsCellRecalc() const { return m_needsCellRecalc; }
     void setNeedsCellRecalc();
 
-    LayoutUnit getBaseline(unsigned row) { return m_grid[row].baseline; }
+    LayoutUnit rowBaseline(unsigned row) { return m_grid[row].baseline; }
 
     void rowLogicalHeightChanged(unsigned rowIndex);