Table rows are not totally invalidated after r170349

Source/core/rendering/RenderTableRow.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, 2007, 2008, 2009, 2010, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 163 matching lines @@
         if (!cell->needsLayout())
             cell->markForPaginationRelayoutIfNeeded(layouter);
         if (cell->needsLayout()) {
             cell->computeAndSetBlockDirectionMargins(table());
             cell->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
     // the fact that we did not invalidate paints in setStyle() because we had a layout hint.
     if (selfNeedsLayout()) {
         for (RenderTableCell* cell = firstCell(); cell; cell = cell->nextCell()) {
             // FIXME: Is this needed when issuing paint invalidations after layout?
             cell->setShouldDoFullPaintInvalidation();
         }
     }
@@ skipping 43 matching lines @@
 }
 
 RenderTableRow* RenderTableRow::createAnonymousWithParentRenderer(const RenderObject* parent)
 {
     RenderTableRow* newRow = RenderTableRow::createAnonymous(&parent->document());
     RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyleWithDisplay(parent->style(), TABLE_ROW);
     newRow->setStyle(newStyle.release());
     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