Bug 108397

Created attachment 185610 [details] Proposed change I.

Comment on attachment 185610 [details] Proposed change I. View in context: https://bugs.webkit.org/attachment.cgi?id=185610&action=review > Source/WebCore/css/CSSParser.cpp:2520 > + case CSSPropertyWebkitGridAutoFlow: > + if (id == CSSValueNone || id == CSSValueRows || id == CSSValueColumns) > + validPrimitive = true; > + break; I think this should go in isValidKeywordPropertyAndValue. > Source/WebCore/css/CSSPrimitiveValueMappings.h:4117 > + Remove this extra blank line. > Source/WebCore/css/CSSValueKeywords.in:1005 > +// -webkit-grid-auto-flow > +// none > +columns > +rows Can you ask on www-style if we can make these column/row to match flex-flow? It's OK to check this in, but we shouldn't make web devs have to think whether the values need to be plural or not. > Source/WebCore/rendering/style/RenderStyleConstants.h:494 > +enum GridAutoFlow { > + AutoFlowNone, > + AutoFlowColumns, > + AutoFlowRows > +}; Nit: Should this be on one line like many of the other enums? > LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:19 > +.gridAutoFlowNone { > + -webkit-grid-auto-flow: none; > +} > +.gridAutoFlowColumns { > + -webkit-grid-auto-flow: columns; > +} > +.gridAutoFlowRows { > + -webkit-grid-auto-flow: rows; > +} > +.gridAutoFlowInherit { Can we move these into resources/grid.css?

Comment on attachment 185610 [details] Proposed change I. View in context: https://bugs.webkit.org/attachment.cgi?id=185610&action=review >> Source/WebCore/css/CSSValueKeywords.in:1005 >> +rows > > Can you ask on www-style if we can make these column/row to match flex-flow? It's OK to check this in, but we shouldn't make web devs have to think whether the values need to be plural or not. Asked on www-style: http://lists.w3.org/Archives/Public/www-style/2013Jan/0634.html >> Source/WebCore/rendering/style/RenderStyleConstants.h:494 >> +}; > > Nit: Should this be on one line like many of the other enums? This is not our usual style to put everything on one line, but nothing matches our usual style in RenderStyleConstants :( >> LayoutTests/fast/css-grid-layout/grid-auto-flow-get-set.html:19 >> +.gridAutoFlowInherit { > > Can we move these into resources/grid.css? SGTM, only the valid values above though, I don't see much use in sharing the wrong values.

Created attachment 186071 [details] Proposed change II: won't commit it until next week to wait for some spec update.

Comment on attachment 186071 [details] Proposed change II: won't commit it until next week to wait for some spec update. Seems fine. Don't think we need to block on the spec discussion. We can always change the names of the values later if the spec changes differently than how we expect.

Comment on attachment 186071 [details] Proposed change II: won't commit it until next week to wait for some spec update. View in context: https://bugs.webkit.org/attachment.cgi?id=186071&action=review > LayoutTests/fast/css-grid-layout/resources/grid-auto-flow-get-set.js:4 > +description('Test that setting and getting grid-auto-flow works as expected'); > + > +debug("Test getting -webkit-auto-flow set through CSS"); > +var gridAutoFlowNone = document.getElementById("gridAutoFlowNone"); Nit: I would inline this in the test file. It seems unlikely that we're going to reuse this .js file and it's not a pure js test.

Comment on attachment 186071 [details] Proposed change II: won't commit it until next week to wait for some spec update. Attachment 186071 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16336087

Comment on attachment 186071 [details] Proposed change II: won't commit it until next week to wait for some spec update. View in context: https://bugs.webkit.org/attachment.cgi?id=186071&action=review >> LayoutTests/fast/css-grid-layout/resources/grid-auto-flow-get-set.js:4 >> +var gridAutoFlowNone = document.getElementById("gridAutoFlowNone"); > > Nit: I would inline this in the test file. It seems unlikely that we're going to reuse this .js file and it's not a pure js test. +1 external files in tests are always a pain to maintain.

Comment on attachment 186071 [details] Proposed change II: won't commit it until next week to wait for some spec update. View in context: https://bugs.webkit.org/attachment.cgi?id=186071&action=review > Don't think we need to block on the spec discussion. We can always change the names of the values later if the spec changes differently than how we expect. That was because the W3C F2F is going on right now and grid layout will be discussed (also Tab / fantasai took over the editorship) so this week will be rocky. >>> LayoutTests/fast/css-grid-layout/resources/grid-auto-flow-get-set.js:4 >>> +var gridAutoFlowNone = document.getElementById("gridAutoFlowNone"); >> >> Nit: I would inline this in the test file. It seems unlikely that we're going to reuse this .js file and it's not a pure js test. > > +1 external files in tests are always a pain to maintain. Fair enough.

Created attachment 186413 [details] Patch for landing

Comment on attachment 186413 [details] Patch for landing Clearing flags on attachment: 186413 Committed r141787: <http://trac.webkit.org/changeset/141787>

All reviewed patches have been landed. Closing bug.