[webkit-reviews] review granted: [Bug 129713] [CSS Grid Layout] <string> not allowed in grid-{area | row | column} syntax : [Attachment 227939] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 129713: [CSS Grid Layout] <string> not allowed in grid-{area | row |
column} syntax
https://bugs.webkit.org/show_bug.cgi?id=129713

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=227939&action=review


> Source/WebCore/css/CSSParser.cpp:4698
> +static inline bool isValidCustomIdent(CSSParserValue& value)

Seems like the argument should be const& rather than just &.

> Source/WebCore/css/CSSParser.cpp:4749
> +	   if (m_valueList->next()) {
> +	       if (m_valueList->current() &&
!isForwardSlashOperator(m_valueList->current())) {
> +		   if (!parseIntegerOrCustomIdentFromGridPosition(numericValue,
gridLineName))
> +		       return nullptr;
> +	       }
> +	   }

There is no reason to check current() for null just after calling next(), since
next() returns the same thing that current() will return. This should be
written more like this:

    if (auto* nextValue = m_valueList->next()) {
	if (!isForwardSlashOperator(nextValue) &&
!parseIntegerOrCustomIdentFromGridPosition(numericValue, gridLineName))
	    return nullptr;
    }

But also, I think the logic here is getting really twisted. Repeating the
!isForwardSlashOperator code is not good. I’d like to look at rearranging the
function so this doesn’t have to be repeated.

> Source/WebCore/css/StyleResolver.cpp:1987
>      if (isSpanPosition)
> -	   position.setSpanPosition(gridLineNumber, gridLineName);
> +	   position.setSpanPosition(gridLineNumber ? gridLineNumber : 1,
gridLineName);
>      else
>	   position.setExplicitPosition(gridLineNumber, gridLineName);

The difference between the two sides of the if here seem really strange. Do we
have sufficient test coverage? Is there a value in adding a comment explaining
why we have a minimum of 1 in one case, but not the other?

Might also consider writing it as std::max(gridLineNumber, 1) instead of as a ?
: expression.