[webkit-reviews] review granted: [Bug 103313] [CSS Grid Layout] Implement support for grid-template : [Attachment 213758] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 103313: [CSS Grid Layout] Implement support for grid-template
https://bugs.webkit.org/show_bug.cgi?id=103313

Attachment 213758: Patch
https://bugs.webkit.org/attachment.cgi?id=213758&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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('-web
kit-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('-webk
it-grid-template')", '"foobar"')

Hmm... maybe not the getElementById part then, but at least it could take an
element as a parameter.