Table rows are not totally invalidated after r170349

Source/core/rendering/RenderTableRow.cpp

Index: Source/core/rendering/RenderTableRow.cpp
diff --git a/Source/core/rendering/RenderTableRow.cpp b/Source/core/rendering/RenderTableRow.cpp
index 4639668a44cd4d423c367e3d208a6f67b18865d1..9979310e78d8660f2d6fdce6888f07a054824b40 100644
--- a/Source/core/rendering/RenderTableRow.cpp
+++ b/Source/core/rendering/RenderTableRow.cpp
@@ -181,6 +181,7 @@ void RenderTableRow::layout()
 
     m_overflow.clear();
     addVisualEffectOverflow();
+    // We do not call addOverflowFromCell as part of layout as the cells are not fully laid out.

[dsinclair, 2014/10/23 14:22:01]

Can we reword this since it's not that we don't call it as part of layout, it's
that we don't call it as part of the table row layout.

[Julien - ping for review, 2014/10/23 18:40:08]

Sure!

 
     // We only ever need to issue paint invalidations if our cells didn't, which means that they didn't need
     // layout, so we know that our bounds didn't change. This code is just making up for
@@ -244,4 +245,23 @@ RenderTableRow* RenderTableRow::createAnonymousWithParentRenderer(const RenderOb
     return newRow;
 }
 
+void RenderTableRow::addOverflowFromCell(const RenderTableCell* cell)
+{
+    // Non-row-spanning-cells don't create overflow (they are fully contained within this row).
+    if (cell->rowSpan() == 1)
+        return;
+
+    // Cells only generates visual overflow.
+    LayoutRect cellVisualOverflowRect = cell->visualOverflowRectForPropagation(style());
+    // The cell and the row share the section's coordinate system so we
+    // need to shift the cell to be in the row's coordinate system.

[dsinclair, 2014/10/23 14:22:01]

I don't understand this comment. If cell and row share a coordinate system, why
isn't cell already in the right coordinate system?

[Julien - ping for review, 2014/10/23 18:40:08]

They are both in the section's coordinate system, even if they are parent /
child, which is uncommon. In order to compute the row's visual overflow, we need
the cell's coordinates in the row's coordinate system though. Let me update this
comment to be less confusing.

+    // RenderTableCell::location() seems bogus for that, so using the different rows.

[dsinclair, 2014/10/23 14:22:01]

Any reason why this is bogus? Should we have a fixme or, is it different for a
reason?

[Julien - ping for review, 2014/10/23 18:40:08]

I was going to say that I am pretty sure we don't use it for painting but it
looks like we do. After digging up, that was a bad comment. We should use
cell->location() as it results in less invalidation in the new test case.

+    unsigned row = cell->rowIndex();
+    RenderTableRow* cellStartingRow = section()->rowRendererAt(row);
+    LayoutUnit cellOffsetLogicalTopDifference = cellStartingRow->location().y() + cell->intrinsicPaddingBefore() - location().y();
+    cellVisualOverflowRect.move(0, cellOffsetLogicalTopDifference);
+
+    addVisualOverflow(cellVisualOverflowRect);
+}
+
 } // namespace blink