Bug 79580

Summary: Extract the logic for computing the dirty rows / columns out of RenderTableSection::paintObject
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, eric, hyatt, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.
none
Corrected wrong parentheses pointed out by Robert. none

Description Julien Chaffraix 2012-02-25 14:56:01 PST
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.
Comment 1 Julien Chaffraix 2012-02-25 15:09:52 PST
Created attachment 128885 [details]
Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.
Comment 2 Robert Hogan 2012-02-25 23:59:37 PST
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.
Comment 3 Robert Hogan 2012-02-27 00:09:41 PST
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.
Comment 4 Julien Chaffraix 2012-02-27 11:07:33 PST
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).
Comment 5 Julien Chaffraix 2012-02-27 13:54:18 PST
Created attachment 129093 [details]
Corrected wrong parentheses pointed out by Robert.
Comment 6 Eric Seidel (no email) 2012-02-27 14:27:23 PST
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.
Comment 7 Julien Chaffraix 2012-02-27 16:14:06 PST
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.
Comment 8 WebKit Review Bot 2012-02-27 17:05:26 PST
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>
Comment 9 WebKit Review Bot 2012-02-27 17:05:31 PST
All reviewed patches have been landed.  Closing bug.