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+

Sergio Villar Senin
Reported 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
Attachments
Patch (26.65 KB, patch)
2013-07-01 10:27 PDT, Sergio Villar Senin 		kling: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2013-07-01 10:27:51 PDT
Created attachment 205819 [details] Patch
Andreas Kling
Comment 2 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.
Sergio Villar Senin
Comment 3 2013-08-06 08:07:29 PDT
Committed r153752: <http://trac.webkit.org/changeset/153752>
Note You need to log in before you can comment on or make changes to this bug.