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.
Created attachment 128885 [details] Proposed refactoring: introduce CellSpan and 2 functions, cleaning up as we go.
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 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 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).
Created attachment 129093 [details] Corrected wrong parentheses pointed out by Robert.
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 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 on attachment 129093 [details] Corrected wrong parentheses pointed out by Robert. Clearing flags on attachment: 129093 Committed r109042: <http://trac.webkit.org/changeset/109042>
All reviewed patches have been landed. Closing bug.