Bug 103313

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. ***