[webkit-reviews] review granted: [Bug 73349] compareBorders() is called too often during painting : [Attachment 122053] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Robert Hogan
<robert at webkit.org>'s request for review:
Bug 73349: compareBorders() is called too often during painting
https://bugs.webkit.org/show_bug.cgi?id=73349

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

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


> Source/WebCore/ChangeLog:26
> +	    Compute, and also cache if the full border including color was
computed. 

Compute, and cache _ if ...

(looks like a noun is missing and should be put instead of the placeholder
(disclaimer, I am a non-native English speaker))

> Source/WebCore/rendering/RenderTableCell.cpp:63
> +	   recalcSection->removeCellCollapsedBorders(this);

You should check that the table has collapsing borders before calling
removeCellCollapsedBorders (adding 4 table lookups for nothing).

> Source/WebCore/rendering/RenderTableCell.h:176
> +    CollapsedBorderValue collapsedLeftBorder(RenderStyle*) const;
> +    CollapsedBorderValue collapsedRightBorder(RenderStyle*) const;
> +    CollapsedBorderValue collapsedTopBorder(RenderStyle*) const;
> +    CollapsedBorderValue collapsedBottomBorder(RenderStyle*) const;

Those should be renamed cachedCollapsed*Border as they use in the cache now.

> Source/WebCore/rendering/RenderTableSection.cpp:1346
> +	   m_cellsCollapsedBorders.take(make_pair(cell, side));

This should be m_cellsCollapsedBorders.remove as we don't need the return value
from take.

> Source/WebCore/rendering/RenderTableSection.cpp:1356
> +    HashMap<pair<const RenderTableCell*, int>,
CollapsedBorderValue>::iterator it =
m_cellsCollapsedBorders.find(make_pair(cell, side));

All the 3 previous functions should ASSERT that our table has collapsing
borders.

> Source/WebCore/rendering/RenderTableSection.h:141
> +    void removeCellCollapsedBorders(const RenderTableCell*);
> +    void setCollapsedBorder(const RenderTableCell*, CollapsedBorderSide,
CollapsedBorderValue);

For consistency, those 2 functions should be named:
removeCachedCellCollapsedBorders and setCachedCollapsedBorder.

If you have more consistent and better naming, feel free to use it (I am not
entirely satisfied by the 2 previous names).