Bug 118255 - [CSS Grid Layout] Allow defining named grid lines on the grid element
Summary: [CSS Grid Layout] Allow defining named grid lines on the grid element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks: 103310 118025
  Show dependency treegraph
 
Reported: 2013-07-01 10:21 PDT by Sergio Villar Senin
Modified: 2013-08-07 01:56 PDT (History)
14 users (show)

See Also:


Attachments
Patch (26.65 KB, patch)
2013-07-01 10:27 PDT, Sergio Villar Senin
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>