Table rows are not totally invalidated after r170349

Table rows are not totally invalidated after r170349

The change removed
RenderTableRow::clippedOverflowRectForPaintInvalidation
but the row's visual overflow doesn't include any cells
spanning several rows, which is required to be able to
invalidate rows correctly.

The fix is to include the spanning cells into the
row's visual overflow. Because they both share the
section's coordinate system, we need to translate the
cell's coordinate into the row's to avoid some wrong
visual overflow (aka over-invalidations).

BUG=417060
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184296

[Julien - ping for review, 2014-10-23 01:02:17]

jchaffraix@chromium.org changed reviewers:
+ dsinclair@chromium.org, xianxianzhu@chromium.org

[Julien - ping for review, 2014-10-23 01:02:18]

Took me 3 tries but here is a good fix for the regression.

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

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableRow.cpp (right):

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRow.cpp:184: // We do not call
addOverflowFromCell as part of layout as the cells are not fully laid out.
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.

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRow.cpp:257: // need to shift the cell to be in
the row's coordinate system.
I don't understand this comment. If cell and row share a coordinate system, why
isn't cell already in the right coordinate system?

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRow.cpp:258: // RenderTableCell::location()
seems bogus for that, so using the different rows.
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:09]

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableRow.cpp (right):

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRow.cpp:184: // We do not call
addOverflowFromCell as part of layout as the cells are not fully laid out.
On 2014/10/23 14:22:01, dsinclair wrote:
> 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.

Sure!

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRow.cpp:257: // need to shift the cell to be in
the row's coordinate system.
On 2014/10/23 14:22:01, dsinclair wrote:
> I don't understand this comment. If cell and row share a coordinate system,
why
> isn't cell already in the right coordinate system?

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.

https://codereview.chromium.org/673533002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderTableRow.cpp:258: // RenderTableCell::location()
seems bogus for that, so using the different rows.
On 2014/10/23 14:22:01, dsinclair wrote:
> Any reason why this is bogus? Should we have a fixme or, is it different for a
> reason?

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.

[dsinclair, 2014-10-23 18:50:59]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2014-10-23 18:51:00]

lgtm

[commit-bot: I haz the power, 2014-10-23 18:52:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/673533002/80001

[commit-bot: I haz the power, 2014-10-23 19:51:29]

Committed patchset #5 (id:80001) as 184296