[webkit-reviews] review granted: [Bug 118255] [CSS Grid Layout] Allow defining named grid lines on the grid element : [Attachment 205819] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 6 00:47:34 PDT 2013
Andreas Kling <akling at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 118255: [CSS Grid Layout] Allow defining named grid lines on the grid
element
https://bugs.webkit.org/show_bug.cgi?id=118255
Attachment 205819: Patch
https://bugs.webkit.org/attachment.cgi?id=205819&action=review
------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205819&action=review
> Source/WebCore/css/CSSParser.cpp:4788
> + RefPtr<CSSPrimitiveValue> name =
createPrimitiveStringValue(m_valueList->current());
> + values->append(name);
I wouldn't use a temporary here, it adds nothing but ref count churn.
> Source/WebCore/rendering/style/RenderStyle.h:1301
> + void setNamedGridColumnLines(const NamedGridLinesMap&
namedGridColumnLines) { SET_VAR(rareNonInheritedData.access()->m_grid,
m_namedGridColumnLines, namedGridColumnLines); }
> + void setNamedGridRowLines(const NamedGridLinesMap& namedGridRowLines) {
SET_VAR(rareNonInheritedData.access()->m_grid, m_namedGridRowLines,
namedGridRowLines); }
This highlights a flaw with the SET_VAR macro; even if the new value is equal
to the new one, we'll still always detach from the shared rareNonInheritedData
(due to there being two levels of indirection, and SET_VAR handles only one.)
Not specific to this patch, we already have other instances of
SET_VAR(foo.access(), ...) that we should do something about to avoid
unnecessary cloning.
> Source/WebCore/rendering/style/StyleGridData.h:47
> - return m_gridColumns == o.m_gridColumns && m_gridRows ==
o.m_gridRows && m_gridAutoFlow == o.m_gridAutoFlow && m_gridAutoRows ==
o.m_gridAutoRows && m_gridAutoColumns == o.m_gridAutoColumns;
> + return m_gridColumns == o.m_gridColumns && m_gridRows ==
o.m_gridRows && m_gridAutoFlow == o.m_gridAutoFlow && m_gridAutoRows ==
o.m_gridAutoRows && m_gridAutoColumns == o.m_gridAutoColumns &&
m_namedGridColumnLines == o.m_namedGridColumnLines && m_namedGridRowLines ==
o.m_namedGridRowLines;
Comparing two hash maps here doesn't look great for performance. Probably not a
problem initially, but something we should keep in mind going forward.
More information about the webkit-reviews
mailing list