Bug 129713 – [CSS Grid Layout] <string> not allowed in grid-{area | row | column} syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Latest specification defines the syntax for grid-{area | row | column} properties as <grid-line>, so no <string> clauses allowed,
  
  - http://www.w3.org/TR/css3-grid-layout/#typedef-grid-line

[Sergio Villar Senin]

Created attachment 227939 [details]
Patch

[Sergio Villar Senin]

ping reviewers :)

[Darin Adler]

Comment on attachment 227939 [details]
Patch

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.

[Sergio Villar Senin]

Comment on attachment 227939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227939&action=review

Thanks for the usual great review!

>> Source/WebCore/css/CSSParser.cpp:4749
>> +        }
> 
> 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.

Good point w.r.t. the next() thing.

Also I'll consider the rearrangement you mentioned. Just wanted to keep the change minimal, but it's true that the resulting logic might be a bit twisted.

>> Source/WebCore/css/StyleResolver.cpp:1987
>>          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.

In the if part we set a SpanPosition while in the else we set an ExplicitPosition. It's true that the arguments are the same but the type of position, and the resolution, is totally different.

Regarding the 1 as minimum, I agree it deserves a comment. For span positions the specs mandates that it should take an integer > 0, but that restriction does not apply for explicit positions. I'll consider switching to a std::max() expression.

Finally, we indeed have test coverage for all the potential permutations allowed by the specs.

[Sergio Villar Senin]

Committed r166712: <http://trac.webkit.org/changeset/166712>