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

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied 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 213699: Patch
https://bugs.webkit.org/attachment.cgi?id=213699&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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?