[webkit-reviews] review canceled: [Bug 88066] Make RenderTableSection - nodeAtPoint and paintObject reuse spanning logic : [Attachment 145969] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 88066: Make RenderTableSection -  nodeAtPoint and paintObject reuse
spanning logic
https://bugs.webkit.org/show_bug.cgi?id=88066

Attachment 145969: Patch
https://bugs.webkit.org/attachment.cgi?id=145969&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145969&action=review


> Source/WebCore/rendering/RenderTableSection.cpp:1053
>  CellSpan RenderTableSection::dirtiedRows(const LayoutRect& damageRect) const


I would really have loved for dirtiedRows / dirtiedColumns to just die but it
seems that we need to patch the CellSpan returned from spannedRows. Having
different behaviors for hit-testing and painting is unfortunate and should make
us wonder if we don't prefer a unified code path (less bug but also any
improvement would improve both code path).

> Source/WebCore/rendering/RenderTableSection.cpp:1058
> +    CellSpan coveredRows = spannedRows(damageRect);

I don't think this is right: spannedRows takes a rect that is flipped to
account for the section's direction. However the damage rect is in device
coordinates which doesn't account for direction.

> Source/WebCore/rendering/RenderTableSection.cpp:1062
> +    // To repaint the border we might need to repaint first or last row even
if they are not spanned themselves.
> +    if (coveredRows.start() >= m_rowPos.size() - 1 &&
m_rowPos[m_rowPos.size() - 1] + table()->outerBorderAfter() >= damageRect.y())
> +	   --coveredRows.start();

Is your change working with vertical writing modes as m_rowPos is actually in
the x direction? (which was why |before| needed to take the writing mode into
account)

> Source/WebCore/rendering/RenderTableSection.cpp:1064
> +    if (!coveredRows.end() && m_rowPos[0] - table()->outerBorderBefore() <=
damageRect.maxY())

This is not mixed direction aware AFAICT (e.g if table and table-section have
opposite directions - see bug 87900). Could you add a FIXME about that?