Bug 118255 – [CSS Grid Layout] Allow defining named grid lines on the grid element

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

We should consider merging:

    Allow defining named grid lines on the grid element
    
    This change adds parsing, style resolution and getComputedStyle
    support for named grid lines at the grid element level (ie extends
    our <track-list> support). Per the specification, we allow multiple
    grid lines with the same name.
    
    To fully support resolving the grid lines to a position on our
    grid, we need to add the parsing at the grid item's level (which
    means extending our <grid-line> support). This will be done in a
    follow-up change.
    
    BUG=234192
    TEST=fast/css-grid-layout/named-grid-line-get-set.html
    
    Review URL: https://chromiumcodereview.appspot.com/14786002

[Sergio Villar Senin]

Created attachment 205819 [details]
Patch

[Andreas Kling]

Comment on attachment 205819 [details]
Patch

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.

[Sergio Villar Senin]

Committed r153752: <http://trac.webkit.org/changeset/153752>