grid-template allows the grid to be defined graphically using identifiers to name grid areas: grid-template = [ " [ <identifier> | . ]+ " ]+ | none We also need to be careful with the interaction with grid-definition-{rows|columns}.
Fixing this involves merging 3 different CLs from Blink: * Implement 'grid-template' parsing This change makes us recognize grid-template, parse it, store it and return properly to JavaScript. The specification mandates to validate to check that the areas defined by 'grid-template' are rectangular which involves a lot of the CSSParser code. Review URL: https://chromiumcodereview.appspot.com/18532004 * [CSS Grid Layout] Fix grid-template serialization This is a follow-up of https://codereview.chromium.org/18532004/#msg6 We didn't know if serializing strings was specified and it turns out that it is: http://dev.w3.org/csswg/cssom/#serialize-a-string * Move GridCoordinate and GridSpan to a separate file In order to implement 'grid-template', we need a way to describe a grid area's coordinates. That's what GridCoordinate is about and instead of duplicating code, splitting the classes will enable code sharing. While moving the code, added some comments about the 2 classes. Review URL: https://chromiumcodereview.appspot.com/18345011
Created attachment 213696 [details] Patch
Created attachment 213699 [details] Patch
Comment on attachment 213699 [details] Patch Attachment 213699 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/3758003
Comment on attachment 213699 [details] Patch Attachment 213699 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3758006
Comment on attachment 213699 [details] Patch Attachment 213699 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/3714254
Comment on attachment 213699 [details] Patch Attachment 213699 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/3716246
Comment on attachment 213699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213699&action=review Nearly ready once build errors are fixed. > Source/WebCore/css/CSSGridTemplateValue.cpp:54 > + NamedGridAreaMap::const_iterator end = gridAreaMap.end(); > + for (NamedGridAreaMap::const_iterator it = gridAreaMap.begin(); it != end; ++it) { You can use "auto" here now. for (auto it = gridAreaMap.begin(), end = gridAreaMap.end(); it != end; ++it) > Source/WebCore/css/CSSGridTemplateValue.cpp:61 > + end = gridAreaMap.end(); > + for (NamedGridAreaMap::const_iterator it = gridAreaMap.begin(); it != end; ++it) { Ditto. > Source/WebCore/css/CSSParser.cpp:5173 > + if (!columnCount) { > + columnCount = columnNames.size(); > + ASSERT(columnCount); > + } else if (columnCount != columnNames.size()) { > + // The declaration is invalid is all the rows don't have the number of columns. > + return 0; > + } Please write this with the return first. if (columnCount != columnNames.size()) { // Comment, note typo is -> if return 0; } Although I guess that assumes that columnCount is non-zero too. > Source/WebCore/css/CSSParser.cpp:5189 > + NamedGridAreaMap::iterator gridAreaIt = gridAreaMap.find(gridAreaName); Maybe auto gridAreaIterator?
Comment on attachment 213699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213699&action=review Thx for the review dino, new patch upcoming > Source/WebCore/css/CSSGridTemplateValue.h:72 > + Actually I'm also removing these three and using CSS_VALUE_TYPE_CASTS that does the same (and more) and it's more WebKit-style
Created attachment 213758 [details] Patch
Comment on attachment 213758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213758&action=review > Source/WebCore/css/CSSGridTemplateValue.cpp:2 > + * Copyright (C) 2013 Google Inc. All rights reserved. Do you really mean to assign Google the copyright? Why not your own name? (do you work for them?) > Source/WebCore/css/CSSGridTemplateValue.cpp:14 > + * * Neither the name of Google Inc. nor the names of its DItto. > Source/WebCore/css/CSSGridTemplateValue.h:2 > + * Copyright (C) 2013 Google Inc. All rights reserved. Ditto. And other places too. > Source/WebCore/css/CSSParser.cpp:5177 > + for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) { Could you expand Col to Column? I don't see the need to shorten (there are a few instances of this) > LayoutTests/fast/css-grid-layout/grid-template-get-set.html:67 > + shouldBeEqualToString("window.getComputedStyle(gridWithSpanningRowsDotTemplate).getPropertyValue('-webkit-grid-template')", '"span" "."') Please extract window.getComputedStyle(gridWithSpanningRowsDotTemplate).getPropertyValue('-webkit-grid-template') into a helper function. In fact, the function could wrap the getElementById + shouldBeEqualToString part as well, so it is just something like testGridTemplateValue("gridWithDefaultTemplate", "none"); > LayoutTests/fast/css-grid-layout/grid-template-get-set.html:76 > + element.style.webkitGridTemplate = "'foobar'"; > + shouldBeEqualToString("window.getComputedStyle(element).getPropertyValue('-webkit-grid-template')", '"foobar"') Hmm... maybe not the getElementById part then, but at least it could take an element as a parameter.
We need to keep the Google copyright notice if the code was copied from Blink or if the author looked at Blink code to write this.
(In reply to comment #11) > (From update of attachment 213758 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213758&action=review > > > Source/WebCore/css/CSSGridTemplateValue.cpp:2 > > + * Copyright (C) 2013 Google Inc. All rights reserved. > > Do you really mean to assign Google the copyright? Why not your own name? (do you work for them?) As Ryosuke mentions this work is based on some patches from Blink so it's fair/correct/legal to keep the copyright notice as is. I might add the Igalia copyright too. > > Source/WebCore/css/CSSParser.cpp:5177 > > + for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) { > > Could you expand Col to Column? I don't see the need to shorten (there are a few instances of this) Also expanded lookAheadCol -> lookAheadColumn > > LayoutTests/fast/css-grid-layout/grid-template-get-set.html:76 > > + element.style.webkitGridTemplate = "'foobar'"; > > + shouldBeEqualToString("window.getComputedStyle(element).getPropertyValue('-webkit-grid-template')", '"foobar"') > > Hmm... maybe not the getElementById part then, but at least it could take an element as a parameter. Done
Committed r157211: <http://trac.webkit.org/changeset/157211>
*** Bug 119922 has been marked as a duplicate of this bug. ***