Bug 118255

Summary: [CSS Grid Layout] Allow defining named grid lines on the grid element
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, commit-queue, ddkilzer, dino, esprehn+autocc, glenn, hyatt, jchaffraix, kling, koivisto, macpherson, menard, svillar, tony
Priority: P2 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 103310, 118025    
Attachments:
Description Flags
Patch kling: review+

Description Sergio Villar Senin 2013-07-01 10:21:42 PDT
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
Comment 1 Sergio Villar Senin 2013-07-01 10:27:51 PDT
Created attachment 205819 [details]
Patch
Comment 2 Andreas Kling 2013-08-06 00:47:16 PDT
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.
Comment 3 Sergio Villar Senin 2013-08-06 08:07:29 PDT
Committed r153752: <http://trac.webkit.org/changeset/153752>