Stop abusing RenderTableSection::needsRecalcCells logic

Julien Chaffraix · Julien Chaffraix · commit 4355d203cc4f · 2011-11-03T17:20:01.000Z
https://bugs.webkit.org/show_bug.cgi?id=71420 Reviewed by Darin Adler. Source/WebCore: Change covered by existing tests like fast/repaint/table-extra-bottom-grow.html and fast/table/row-height-recalc* (among others). Cell recalculation is very expensive and should only be called when the section's structure changed in a way that requires a safe update to its structure (like removing a row as our column split may not be appropriate anymore). The current code would abuse cell recalculation to actually reset the logical height on the RowStruct. This change makes it do the right thing. * rendering/RenderTableCell.h: * rendering/RenderTableRow.h: Removed styleWillChange override as it was unneeded. * rendering/RenderTableCell.cpp: (WebCore::RenderTableCell::styleDidChange): * rendering/RenderTableRow.cpp: (WebCore::RenderTableRow::styleDidChange): Move the code from styleWillChange to styleDidChange. * rendering/RenderTableSection.cpp: (WebCore::RenderTableSection::rowLogicalHeightChanged): This function just reset the height on the |RowStruct| which is the only part of recalcCells that we would need. (WebCore::RenderTableSection::rowIndexForRenderer): Added this function to find out which index a column has (strangely RenderTableRow does not have this information). * rendering/RenderTableSection.h: Added the 2 previous functions. LayoutTests: * platform/chromium-linux/fast/repaint/table-extra-bottom-grow-expected.png: Update this test as this is a progression: we are not over-repainting the table anymore. * platform/chromium/test_expectations.txt: * platform/efl/Skipped: * platform/mac/Skipped: * platform/qt/test_expectations.txt: Skipped the test here as it needs a rebaseline. Canonical link: https://commits.webkit.org/87802@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@99212 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,20 @@
+2011-11-03  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Stop abusing RenderTableSection::needsRecalcCells logic
+        https://bugs.webkit.org/show_bug.cgi?id=71420
+
+        Reviewed by Darin Adler.
+
+        * platform/chromium-linux/fast/repaint/table-extra-bottom-grow-expected.png:
+        Update this test as this is a progression: we are not over-repainting the table
+        anymore.
+
+        * platform/chromium/test_expectations.txt:
+        * platform/efl/Skipped:
+        * platform/mac/Skipped:
+        * platform/qt/test_expectations.txt:
+        Skipped the test here as it needs a rebaseline.
+
 2011-11-03  Kentaro Hara  <haraken@chromium.org>
 
         Unreviewed rebaselining. Unskipped progress-event-constructor.html in Qt-Arm
diff --git a/LayoutTests/platform/chromium-linux/fast/repaint/table-extra-bottom-grow-expected.png b/LayoutTests/platform/chromium-linux/fast/repaint/table-extra-bottom-grow-expected.png
diff --git a/LayoutTests/platform/chromium/test_expectations.txt b/LayoutTests/platform/chromium/test_expectations.txt
@@ -3886,4 +3886,6 @@ BUGWK71450 MAC DEBUG SLOW : fast/frames/valid.html = PASS
 BUGWK71451 SLOW : fast/frames/sandboxed-iframe-navigation-windowopen.html = PASS
 BUGWK71451 LINUX : http/tests/security/contentSecurityPolicy/policy-does-not-affect-child.html = TEXT PASS
 BUGWK71451 LINUX : http/tests/security/contentSecurityPolicy/object-src-none-allowed.html = TEXT PASS
- 
+
+// This just needs a rebaseline.
+BUGWK71420 WIN : fast/repaint/table-extra-bottom-grow.html = IMAGE
diff --git a/LayoutTests/platform/efl/Skipped b/LayoutTests/platform/efl/Skipped
@@ -1923,3 +1923,6 @@ fast/dom/MicroData
 # Tests for MediaSource API. Feature is not yet functional.
 # https://bugs.webkit.org/show_bug.cgi?id=64731
 http/tests/media/media-source/
+
+# Needs a rebaseline
+fast/repaint/table-extra-bottom-grow.html
diff --git a/LayoutTests/platform/mac/Skipped b/LayoutTests/platform/mac/Skipped
@@ -467,3 +467,7 @@ fast/css/webkit-mask-crash-table.html
 # https://bugs.webkit.org/show_bug.cgi?id=70989
 fast/canvas/webgl/attrib-location-length-limits.html
 fast/canvas/webgl/uniform-location-length-limits.html
+
+# https://bugs.webkit.org/show_bug.cgi?id=71420
+# It needs a rebaseline
+fast/repaint/table-extra-bottom-grow.html
diff --git a/LayoutTests/platform/qt/test_expectations.txt b/LayoutTests/platform/qt/test_expectations.txt
@@ -25,3 +25,6 @@ BUGWK67007 DEBUG : fast/ruby/generated-after-counter-doesnt-crash.html = CRASH
 BUGWK67007 DEBUG : fast/ruby/generated-before-and-after-counter-doesnt-crash.html = CRASH
 
 BUGWK62662 DEBUG : inspector/cookie-parser.html = CRASH PASS
+
+// Needs a rebaseline
+BUGWK71420 : fast/repaint/table-extra-bottom-grow.html = IMAGE
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,41 @@
+2011-11-03  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Stop abusing RenderTableSection::needsRecalcCells logic
+        https://bugs.webkit.org/show_bug.cgi?id=71420
+
+        Reviewed by Darin Adler.
+
+        Change covered by existing tests like fast/repaint/table-extra-bottom-grow.html
+        and fast/table/row-height-recalc* (among others).
+
+        Cell recalculation is very expensive and should only be called when the section's structure
+        changed in a way that requires a safe update to its structure (like removing a row as our
+        column split may not be appropriate anymore).
+
+        The current code would abuse cell recalculation to actually reset the logical height on the
+        RowStruct. This change makes it do the right thing.
+
+        * rendering/RenderTableCell.h:
+        * rendering/RenderTableRow.h:
+        Removed styleWillChange override as it was unneeded.
+
+        * rendering/RenderTableCell.cpp:
+        (WebCore::RenderTableCell::styleDidChange):
+        * rendering/RenderTableRow.cpp:
+        (WebCore::RenderTableRow::styleDidChange):
+        Move the code from styleWillChange to styleDidChange.
+
+        * rendering/RenderTableSection.cpp:
+        (WebCore::RenderTableSection::rowLogicalHeightChanged):
+        This function just reset the height on the |RowStruct| which is the
+        only part of recalcCells that we would need.
+
+        (WebCore::RenderTableSection::rowIndexForRenderer):
+        Added this function to find out which index a column has (strangely
+        RenderTableRow does not have this information).
+
+        * rendering/RenderTableSection.h: Added the 2 previous functions.
+
 2011-11-03  Andreas Kling  <kling@webkit.org>
 
         CSSRuleList: Move rule orphaning from deleteRule() out to callers.
diff --git a/Source/WebCore/rendering/RenderTableCell.cpp b/Source/WebCore/rendering/RenderTableCell.cpp
@@ -310,21 +310,16 @@ int RenderTableCell::cellBaselinePosition() const
     return paddingBefore() + borderBefore() + contentLogicalHeight();
 }
 
-void RenderTableCell::styleWillChange(StyleDifference diff, const RenderStyle* newStyle)
-{
-    if (parent() && section() && style() && style()->height() != newStyle->height())
-        section()->setNeedsCellRecalc();
-
-    ASSERT(newStyle->display() == TABLE_CELL);
-
-    RenderBlock::styleWillChange(diff, newStyle);
-}
-
 void RenderTableCell::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
+    ASSERT(style()->display() == TABLE_CELL);
+
     RenderBlock::styleDidChange(diff, oldStyle);
     setHasBoxDecorations(true);
 
+    if (parent() && section() && oldStyle && style()->height() != oldStyle->height())
+        section()->rowLogicalHeightChanged(row());
+
     // If border was changed, notify table.
     if (parent()) {
         RenderTable* table = this->table();
diff --git a/Source/WebCore/rendering/RenderTableCell.h b/Source/WebCore/rendering/RenderTableCell.h
@@ -153,7 +153,6 @@ class RenderTableCell : public RenderBlock {
     void setCellWidthChanged(bool b = true) { m_cellWidthChanged = b; }
 
 protected:
-    virtual void styleWillChange(StyleDifference, const RenderStyle* newStyle);
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
 
 private:
diff --git a/Source/WebCore/rendering/RenderTableRow.cpp b/Source/WebCore/rendering/RenderTableRow.cpp
@@ -53,16 +53,6 @@ void RenderTableRow::willBeDestroyed()
         recalcSection->setNeedsCellRecalc();
 }
 
-void RenderTableRow::styleWillChange(StyleDifference diff, const RenderStyle* newStyle)
-{
-    if (section() && style() && style()->logicalHeight() != newStyle->logicalHeight())
-        section()->setNeedsCellRecalc();
-
-    ASSERT(newStyle->display() == TABLE_ROW);
-
-    RenderBox::styleWillChange(diff, newStyle);
-}
-
 void RenderTableRow::updateBeforeAndAfterContent()
 {
     if (!isAnonymous() && document()->usesBeforeAfterRules()) {
@@ -73,12 +63,17 @@ void RenderTableRow::updateBeforeAndAfterContent()
 
 void RenderTableRow::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
+    ASSERT(style()->display() == TABLE_ROW);
+
     RenderBox::styleDidChange(diff, oldStyle);
     propagateStyleToAnonymousChildren();
 
     if (parent())
         updateBeforeAndAfterContent();
 
+    if (section() && oldStyle && style()->logicalHeight() != oldStyle->logicalHeight())
+        section()->rowLogicalHeightChanged(section()->rowIndexForRenderer(this));
+
     // If border was changed, notify table.
     if (parent()) {
         RenderTable* table = this->table();
diff --git a/Source/WebCore/rendering/RenderTableRow.h b/Source/WebCore/rendering/RenderTableRow.h
@@ -63,7 +63,6 @@ class RenderTableRow : public RenderBox {
 
     virtual void imageChanged(WrappedImagePtr, const IntRect* = 0);
 
-    virtual void styleWillChange(StyleDifference, const RenderStyle* newStyle);
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
 
     RenderObjectChildList m_children;
diff --git a/Source/WebCore/rendering/RenderTableSection.cpp b/Source/WebCore/rendering/RenderTableSection.cpp
@@ -1158,6 +1158,11 @@ void RenderTableSection::recalcCells()
     setNeedsLayout(true);
 }
 
+void RenderTableSection::rowLogicalHeightChanged(unsigned rowIndex)
+{
+    setRowLogicalHeightToRowStyleLogicalHeightIfNotRelative(m_grid[rowIndex]);
+}
+
 void RenderTableSection::setNeedsCellRecalc()
 {
     m_needsCellRecalc = true;
@@ -1300,4 +1305,14 @@ bool RenderTableSection::nodeAtPoint(const HitTestRequest& request, HitTestResul
 
 }
 
+unsigned RenderTableSection::rowIndexForRenderer(const RenderTableRow* row) const
+{
+    for (size_t i = 0; i < m_grid.size(); ++i) {
+        if (m_grid[i].rowRenderer == row)
+            return i;
+    }
+    ASSERT_NOT_REACHED();
+    return 0;
+}
+
 } // namespace WebCore
diff --git a/Source/WebCore/rendering/RenderTableSection.h b/Source/WebCore/rendering/RenderTableSection.h
@@ -118,6 +118,10 @@ class RenderTableSection : public RenderBox {
 
     LayoutUnit getBaseline(int row) { return m_grid[row].baseline; }
 
+    void rowLogicalHeightChanged(unsigned rowIndex);
+
+    unsigned rowIndexForRenderer(const RenderTableRow*) const;
+
 protected:
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
 
