[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]

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.