Bug 129713

Summary: [CSS Grid Layout] <string> not allowed in grid-{area | row | column} syntax
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: Layout and RenderingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, andersca, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, hyatt, kling, macpherson, menard, rego, svillar, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 129041    
Bug Blocks:    
Attachments:
Description Flags
Patch darin: review+

Description Javier Fernandez 2014-03-04 16:25:09 PST
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
Comment 1 Sergio Villar Senin 2014-03-27 04:23:43 PDT
Created attachment 227939 [details]
Patch
Comment 2 Sergio Villar Senin 2014-04-02 04:41:48 PDT
ping reviewers :)
Comment 3 Darin Adler 2014-04-02 09:29:13 PDT
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.
Comment 4 Sergio Villar Senin 2014-04-02 10:23:55 PDT
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.
Comment 5 Sergio Villar Senin 2014-04-03 03:16:31 PDT
Committed r166712: <http://trac.webkit.org/changeset/166712>