Let RenderTable reach table cells needing overflow recalc

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, 2013 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 925 matching lines @@
 #endif
 
     ASSERT(!needsLayout());
 
     // FIXME: Changing the height without a layout can change the overflow so it seems wrong.
 
     unsigned totalRows = m_grid.size();
 
     // Set the width of our section now.  The rows will also be this width.
     setLogicalWidth(table()->contentLogicalWidth());
-    m_overflow.clear();
+
+    // We will clear and recompute overflow in computeOverflowFromCells().
+    // Clear m_overflowCells early to avoid stale pointers from it.
     m_overflowingCells.clear();

[Julien - ping for review, 2014/11/26 23:21:25]

I missed that layoutRows calls computeOverflowFromCells so it makes sense just
to move all the overflow-related logic there.

[Xianzhu, 2014/11/27 00:28:48]

Acknowledged.

-    m_forceSlowPaintPathWithOverflowingCell = false;
 
     int vspacing = table()->vBorderSpacing();
     unsigned nEffCols = table()->numEffCols();
 
     LayoutState state(*this, locationOffset());
 
     for (unsigned r = 0; r < totalRows; r++) {
         // Set the row's x/y position and width/height.
         RenderTableRow* rowRenderer = m_grid[r].rowRenderer;
         if (rowRenderer) {
             rowRenderer->setLocation(LayoutPoint(0, m_rowPos[r]));
             rowRenderer->setLogicalWidth(logicalWidth());
             rowRenderer->setLogicalHeight(m_rowPos[r + 1] - m_rowPos[r] - vspacing);
             rowRenderer->updateLayerTransformAfterLayout();
-            rowRenderer->clearAllOverflows();
-            rowRenderer->addVisualEffectOverflow();
         }
 
         int rowHeightIncreaseForPagination = 0;
 
         for (unsigned c = 0; c < nEffCols; c++) {
             CellStruct& cs = cellAt(r, c);
             RenderTableCell* cell = cs.primaryCell();
 
             if (!cell || cs.inColSpan)
                 continue;
@@ skipping 71 matching lines @@
                 // FIXME: Pagination might have made us change size. For now just shrink or grow the cell to fit without doing a relayout.
                 // We'll also do a basic increase of the row height to accommodate the cell if it's bigger, but this isn't quite right
                 // either. It's at least stable though and won't result in an infinite # of relayouts that may never stabilize.
                 LayoutUnit oldLogicalHeight = cell->logicalHeight();
                 if (oldLogicalHeight > rHeight)
                     rowHeightIncreaseForPagination = std::max<int>(rowHeightIncreaseForPagination, oldLogicalHeight - rHeight);
                 cell->setLogicalHeight(rHeight);
                 cell->computeOverflow(oldLogicalHeight, false);
             }
 
-            if (rowRenderer)
-                rowRenderer->addOverflowFromCell(cell);
-
             LayoutSize childOffset(cell->location() - oldCellRect.location());
             if (childOffset.width() || childOffset.height()) {
                 // If the child moved, we have to issue paint invalidations to it as well as any floating/positioned
                 // descendants. An exception is if we need a layout. In this case, we know we're going to
                 // issue paint invalidations ourselves (and the child) anyway.
                 if (!table()->selfNeedsLayout())
                     cell->setMayNeedPaintInvalidation(true);
             }
         }
         if (rowHeightIncreaseForPagination) {
@@ skipping 22 matching lines @@
     unsigned totalRows = m_grid.size();
     unsigned nEffCols = table()->numEffCols();
     computeOverflowFromCells(totalRows, nEffCols);
 }
 
 void RenderTableSection::computeOverflowFromCells(unsigned totalRows, unsigned nEffCols)
 {
     unsigned totalCellsCount = nEffCols * totalRows;
     unsigned maxAllowedOverflowingCellsCount = totalCellsCount < gMinTableSizeToUseFastPaintPathWithOverflowingCell ? 0 : gMaxAllowedOverflowingCellRatioForFastPaintPath * totalCellsCount;
 
+    m_overflow.clear();
+    m_overflowingCells.clear();
+    m_forceSlowPaintPathWithOverflowingCell = false;
+
 #if ENABLE(ASSERT)
     bool hasOverflowingCell = false;
 #endif
     // Now that our height has been determined, add in overflow from cells.
     for (unsigned r = 0; r < totalRows; r++) {
+        RenderTableRow* rowRenderer = rowRendererAt(r);
+        if (rowRenderer) {
+            rowRenderer->clearAllOverflows();
+            rowRenderer->addVisualEffectOverflow();
+        }
+
         for (unsigned c = 0; c < nEffCols; c++) {
             CellStruct& cs = cellAt(r, c);
             RenderTableCell* cell = cs.primaryCell();
             if (!cell || cs.inColSpan)
                 continue;
             if (r < totalRows - 1 && cell == primaryCellAt(r + 1, c))
                 continue;
             addOverflowFromChild(cell);
 #if ENABLE(ASSERT)
             hasOverflowingCell |= cell->hasVisualOverflow();
 #endif
+
+            // FIXME: this misses visual effect overflow of of the rowRenderer. crbug.com/437012.

[Julien - ping for review, 2014/11/26 23:21:25]

Note that RenderTableRow::layout() actually calls addVisualEffectOverflow() so
we should centralize the re-computation of overflow as part of crbug.com/437012.

[Xianzhu, 2014/11/27 00:28:48]

Acknowledged.

[Xianzhu, 2014/11/27 00:28:48]

Acknowledged.

+            if (rowRenderer)
+                rowRenderer->addOverflowFromCell(cell);
+
             if (cell->hasVisualOverflow() && !m_forceSlowPaintPathWithOverflowingCell) {
                 m_overflowingCells.add(cell);
                 if (m_overflowingCells.size() > maxAllowedOverflowingCellsCount) {
                     // We need to set m_forcesSlowPaintPath only if there is a least one overflowing cells as the hit testing code rely on this information.
                     m_forceSlowPaintPathWithOverflowingCell = true;
                     // The slow path does not make any use of the overflowing cells info, don't hold on to the memory.
                     m_overflowingCells.clear();
                 }
             }
         }
     }
 
     ASSERT(hasOverflowingCell == this->hasOverflowingCell());
 }
 
+bool RenderTableSection::recalcChildOverflowAfterStyleChange()
+{
+    ASSERT(childNeedsOverflowRecalcAfterStyleChange());
+    clearChildNeedsOverflowRecalcAfterStyleChange();
+
+    unsigned totalRows = m_grid.size();
+    unsigned numEffCols = table()->numEffCols();
+    bool childrenOverflowChanged = false;
+
+    for (unsigned r = 0; r < totalRows; r++) {
+        RenderTableRow* rowRenderer = rowRendererAt(r);
+        if (!rowRenderer || !rowRenderer->childNeedsOverflowRecalcAfterStyleChange())
+            continue;
+
+        rowRenderer->clearChildNeedsOverflowRecalcAfterStyleChange();

[Julien - ping for review, 2014/11/26 23:21:25]

Do we check somewhere that this is cleared for every renderer in the tree?

[Xianzhu, 2014/11/27 00:28:48]

No for now. Will consider adding it.

+
+        for (unsigned c = 0; c < numEffCols; c++) {
+            CellStruct& cs = cellAt(r, c);
+            RenderTableCell* cell = cs.primaryCell();
+            if (!cell || cs.inColSpan || !cell->needsOverflowRecalcAfterStyleChange())
+                continue;
+            childrenOverflowChanged |= cell->recalcOverflowAfterStyleChange();
+        }
+    }
+
+    // FIXME: We should do a full recomputeOverflow which will be feasible after we make RenderBox recomputeOverflow-capable.
+    // crbug.com/437012.

[Julien - ping for review, 2014/11/26 23:21:25]

I don't think we should do it this way: we should fix the computeOverflow
situation and then this change would land without nefarious FIXMEs and
performance degradations.

[Xianzhu, 2014/11/27 00:28:48]

Removed FIXME. With extracted RenderTableRow::recomputeOverflow(), there is also
no performance degradations.

+    if (childrenOverflowChanged)
+        computeOverflowFromCells(totalRows, numEffCols);
+
+    return childrenOverflowChanged;
+}
+
 int RenderTableSection::calcBlockDirectionOuterBorder(BlockBorderSide side) const
 {
     unsigned totalCols = table()->numEffCols();
     if (!m_grid.size() || !totalCols)
         return 0;
 
     unsigned borderWidth = 0;
 
     const BorderValue& sb = side == BorderBefore ? style()->borderBefore() : style()->borderAfter();
     if (sb.style() == BHIDDEN)
@@ skipping 471 matching lines @@
     // FIXME: The table's direction should determine our row's direction, not the section's (see bug 96691).
     if (!style()->isLeftToRightDirection())
         cellLocation.setX(table()->columnPositions()[table()->numEffCols()] - table()->columnPositions()[table()->colToEffCol(cell->col() + cell->colSpan())] + horizontalBorderSpacing);
     else
         cellLocation.setX(table()->columnPositions()[effectiveColumn] + horizontalBorderSpacing);
 
     cell->setLogicalLocation(cellLocation);
 }
 
 } // namespace blink