Bug 79580 – Extract the logic for computing the dirty rows / columns out of RenderTableSection::paintObject

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

This function has been growing steadily as we fixed bugs and improve performance. It is time to split it to underline what's going on.

The function starts by determining which cells are included in the repaint rectangle. This can easily moved to some new functions.

[Julien Chaffraix]

Created attachment 128885 [details]
Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.

[Robert Hogan]

Comment on attachment 128885 [details]
Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.

View in context: https://bugs.webkit.org/attachment.cgi?id=128885&action=review

> Source/WebCore/rendering/RenderTableSection.cpp:1014
> +    LayoutUnit before = (style()->isHorizontalWritingMode() ? damageRect.y() : damageRect.x());

The enclosing parentheses are unnecessary here I think.

> Source/WebCore/rendering/RenderTableSection.cpp:1025
> +    LayoutUnit after = (style()->isHorizontalWritingMode() ? damageRect.maxY() : damageRect.maxX());

Here too.

[Robert Hogan]

Comment on attachment 128885 [details]
Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.

View in context: https://bugs.webkit.org/attachment.cgi?id=128885&action=review

> Source/WebCore/rendering/RenderTableSection.cpp:1074
> +    // FIXME: Why do we double the outline size?
> +    LayoutUnit outlineSize = 2 * maximalOutlineSize(paintPhase);

I guess the intention must have been to inflate each dimension of the rect by the size of the outline on each side of the rect? This must have worked as a fudge-factor because presumably inflation should happen outwards rather than downwards/rightwards - if you know what I mean. I can't see a reason for keeping this '2 *' now that you use inflate.

[Julien Chaffraix]

Comment on attachment 128885 [details]
Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.

View in context: https://bugs.webkit.org/attachment.cgi?id=128885&action=review

>> Source/WebCore/rendering/RenderTableSection.cpp:1014
>> +    LayoutUnit before = (style()->isHorizontalWritingMode() ? damageRect.y() : damageRect.x());
> 
> The enclosing parentheses are unnecessary here I think.

Good catch!

>> Source/WebCore/rendering/RenderTableSection.cpp:1025
>> +    LayoutUnit after = (style()->isHorizontalWritingMode() ? damageRect.maxY() : damageRect.maxX());
> 
> Here too.

Ditto!

>> Source/WebCore/rendering/RenderTableSection.cpp:1074
>> +    LayoutUnit outlineSize = 2 * maximalOutlineSize(paintPhase);
> 
> I guess the intention must have been to inflate each dimension of the rect by the size of the outline on each side of the rect? This must have worked as a fudge-factor because presumably inflation should happen outwards rather than downwards/rightwards - if you know what I mean. I can't see a reason for keeping this '2 *' now that you use inflate.

I don't think this reasoning works. We properly account for the outline in the existing code by removing |outlineSize| from the before / start edge and adding |outlineSize| to the after / end edge. This matches what inflate() does and I did not want any change of behavior (or it wouldn't be a refactoring).

It's a common anti-pattern in the rendering code to multiply the maximalOutlineSize by 2. I think this should be pushed to another bug to investigate more globally why this came to be (this line is here since the KHTML era so I don't have a good answer for this occurrence).

[Julien Chaffraix]

Created attachment 129093 [details]
Corrected wrong parentheses pointed out by Robert.

[Eric Seidel (no email)]

Comment on attachment 129093 [details]
Corrected wrong parentheses pointed out by Robert.

View in context: https://bugs.webkit.org/attachment.cgi?id=129093&action=review

LGTM.

> Source/WebCore/rendering/RenderTableSection.cpp:1064
> +    // FIXME: Why do we double the outline size?
> +    LayoutUnit outlineSize = 2 * maximalOutlineSize(paintPhase);

I suspect it was an attempt to deal with the fact that outline would appear equally on both sides of the rect.  Now using inflate it's obvious that 2x is wrong.  But it's unclear from the original code if 2x was wrong.

> Source/WebCore/rendering/RenderTableSection.h:42
>  
> +// Helper class for paintObject.
> +class CellSpan {
> +public:

It's possible that the whole paintObject function should move onto a helper object which existed just to contain table-paint-related logic.

[Julien Chaffraix]

Comment on attachment 129093 [details]
Corrected wrong parentheses pointed out by Robert.

View in context: https://bugs.webkit.org/attachment.cgi?id=129093&action=review

>> Source/WebCore/rendering/RenderTableSection.h:42
>> +public:
> 
> It's possible that the whole paintObject function should move onto a helper object which existed just to contain table-paint-related logic.

It is definitely possible. I would like to investigate adding a custom iterator to handle the current complexity (has multiple cell levels / overflowing cells / collapsing border phase) and remove some of the copy 'n' paste between the branches.

[WebKit Review Bot]

Comment on attachment 129093 [details]
Corrected wrong parentheses pointed out by Robert.

Clearing flags on attachment: 129093

Committed r109042: <http://trac.webkit.org/changeset/109042>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.