[webkit-reviews] review granted: [Bug 73349] compareBorders() is called too often during painting : [Attachment 122053] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 12 08:38:15 PST 2012
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).
More information about the webkit-reviews
mailing list