Bug 216947 – CSS serialization expects comments between certain tokens

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

CSS serialization expects comments between certain tokens

[Keith Miller]

Created attachment 409632 [details]
Patch

[Keith Miller]

Created attachment 409634 [details]
Patch

[Keith Miller]

Created attachment 409899 [details]
Patch

[Keith Miller]

Created attachment 409903 [details]
Patch

[Keith Miller]

Comment on attachment 409903 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserToken.cpp:489
> +            // Weirdly Clang errors if you try to use the fold expression in buildNextTokenNeedsCommentTable() because the true value is unused.
> +            // So we just build the table by hand here instead.

Before landing I'll amend this with a link to the radar against Clang for the warning: rdar://69710661

[Darin Adler]

Comment on attachment 409903 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-syntax/serialize-consecutive-tokens-expected.txt:2
> +PASS Serialization of consecutive foo and bar tokens. 

Looks like my "strip trailing spaces" change came after you generated this file. Need to strip the trailing spaces.

[Keith Miller]

Created attachment 409909 [details]
Patch

[Keith Miller]

Comment on attachment 409903 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/css/css-syntax/serialize-consecutive-tokens-expected.txt:2
>> +PASS Serialization of consecutive foo and bar tokens. 
> 
> Looks like my "strip trailing spaces" change came after you generated this file. Need to strip the trailing spaces.

Yeah, I just uploaded a new patch now. I'm double checking locally that I have the right number of new lines at the end of the file though.

[Simon Fraser (smfr)]

Comment on attachment 409909 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        don't understand, the spec requires a comment rather than
> +        whitespace so that's what this patch does. Since there are 32

Please provide spec links.

> LayoutTests/imported/w3c/web-platform-tests/css/selectors/anplusb-selector-parsing-expected.txt:102
> +FAIL :nth-last-of-type(1n+0) should be parsed and serialized correctly assert_equals: expected ":nth-last-of-type(n)" but got ":nth-last-of-type(1n/**/+0)"
> +FAIL :nth-last-of-type(n+0) should be parsed and serialized correctly assert_equals: expected ":nth-last-of-type(n)" but got ":nth-last-of-type(n/**/+0)"

This is very surprising.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        whitespace so that's what this patch does. Since there are 32
> 
> Please provide spec links.

Done.

>> LayoutTests/imported/w3c/web-platform-tests/css/selectors/anplusb-selector-parsing-expected.txt:102
>> +FAIL :nth-last-of-type(n+0) should be parsed and serialized correctly assert_equals: expected ":nth-last-of-type(n)" but got ":nth-last-of-type(n/**/+0)"
> 
> This is very surprising.

This happens because we incorrectly parse the an-plus-b syntax as a Dimension token followed by a number token. According to the table https://drafts.csswg.org/css-syntax/#serialization here, this should be serialized as the test is outputting. I plan on fixing our parsing of this syntax to the correct dimension token followed by delimiter then a number, which should fix this issue, in my next patch.

[Darin Adler]

Comment on attachment 409909 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserToken.cpp:429
> +        constexpr auto nextTokenNeedsCommentTable = buildNextTokenNeedsCommentTable(IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken);
> +        appendCommentIfNeeded(nextTokenNeedsCommentTable, '-');

Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this:

    appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken });

We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParserToken.cpp:429
>> +        appendCommentIfNeeded(nextTokenNeedsCommentTable, '-');
> 
> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this:
> 
>     appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken });
> 
> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.

Sure, that makes sense.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>>> Source/WebCore/css/parser/CSSParserToken.cpp:429
>>> +        appendCommentIfNeeded(nextTokenNeedsCommentTable, '-');
>> 
>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this:
>> 
>>     appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken });
>> 
>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.
> 
> Sure, that makes sense.

Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time.

[Darin Adler]

Comment on attachment 409909 [details]
Patch

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

>>>> Source/WebCore/css/parser/CSSParserToken.cpp:429
>>>> +        appendCommentIfNeeded(nextTokenNeedsCommentTable, '-');
>>> 
>>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this:
>>> 
>>>     appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken });
>>> 
>>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.
>> 
>> Sure, that makes sense.
> 
> Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time.

I’m not sure why you’re saying this. We already do this for OptionSet; it works.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>>>>> Source/WebCore/css/parser/CSSParserToken.cpp:429
>>>>> +        appendCommentIfNeeded(nextTokenNeedsCommentTable, '-');
>>>> 
>>>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this:
>>>> 
>>>>     appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken });
>>>> 
>>>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.
>>> 
>>> Sure, that makes sense.
>> 
>> Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time.
> 
> I’m not sure why you’re saying this. We already do this for OptionSet; it works.

Ohh, you're saying buildNextTokenNeedsCommentTable should be converted into a class. Yeah, I think that would work.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>>>>>> Source/WebCore/css/parser/CSSParserToken.cpp:429
>>>>>> +        appendCommentIfNeeded(nextTokenNeedsCommentTable, '-');
>>>>> 
>>>>> Can we refactor so this syntax isn’t needed? Seems like we can make it so we can write this:
>>>>> 
>>>>>     appendCommentIfNeeded('-', { IdentToken, FunctionToken, UrlToken, BadUrlToken, NumberToken, PercentageToken, DimensionToken, CDCToken, LeftParenthesisToken });
>>>>> 
>>>>> We can still have it all be just as constexpr/folded, just written slightly differently using std::initializer_list.
>>>> 
>>>> Sure, that makes sense.
>>> 
>>> Actually, I'm not sure this will work because the initializer_list isn't known to be a compile time constant while you are in the lambda. I'm not sure there's another way to make it work, since you need to assign the table to a constexpr variable to force the compiler to build the table at compile time.
>> 
>> I’m not sure why you’re saying this. We already do this for OptionSet; it works.
> 
> Ohh, you're saying buildNextTokenNeedsCommentTable should be converted into a class. Yeah, I think that would work.

Although, I think the downside is that it isn't guaranteed to be a constexpr value if you do it that way, AFAIK. It's possible it get generated that way in practice though.

[Darin Adler]

Comment on attachment 409909 [details]
Patch

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

> Source/WebCore/css/parser/CSSParserToken.cpp:403
> +template<typename... CSSParserTokenTypes>
> +constexpr std::array<bool, numberOfCSSParserTokenTypes> buildNextTokenNeedsCommentTable(CSSParserTokenTypes... nextTokenNeedsCommentTable)
> +{
> +    std::array<bool, numberOfCSSParserTokenTypes> table { false };
> +    (table[nextTokenNeedsCommentTable] = ... = true);
> +    return table;
> +}

OK as is, but: Seems like this is *really* close to reinventing OptionSet. Seems a shame to build something so specific to this use when we would have something reusable. Also seems likely that using bits and shifting would be even more efficient than the arrays here, and the count here is comfortably under the 64 or 32 boundary.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParserToken.cpp:403
>> +}
> 
> OK as is, but: Seems like this is *really* close to reinventing OptionSet. Seems a shame to build something so specific to this use when we would have something reusable. Also seems likely that using bits and shifting would be even more efficient than the arrays here, and the count here is comfortably under the 64 or 32 boundary.

Per our offline discussion, it looks like converting this function into a struct with a constexpr constructor does happen to get optimized away (from reading the disassembly of this function). So I'm going to change this function into:

```
struct NextTokenNeedsCommentBuilder {
    constexpr NextTokenNeedsCommentBuilder(std::initializer_list<CSSParserTokenType> tokens)
    {
        for (auto token : tokens)
            buffer[token] = false;
    }

    std::array<bool, numberOfCSSParserTokenTypes> buffer { false };
};
```

and have the lambda take a `const NextTokenNeedsCommentBuilder&` passed via the initializer_list as you recommended below.

[Keith Miller]

Comment on attachment 409909 [details]
Patch

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

>>> Source/WebCore/css/parser/CSSParserToken.cpp:403
>>> +}
>> 
>> OK as is, but: Seems like this is *really* close to reinventing OptionSet. Seems a shame to build something so specific to this use when we would have something reusable. Also seems likely that using bits and shifting would be even more efficient than the arrays here, and the count here is comfortably under the 64 or 32 boundary.
> 
> Per our offline discussion, it looks like converting this function into a struct with a constexpr constructor does happen to get optimized away (from reading the disassembly of this function). So I'm going to change this function into:
> 
> ```
> struct NextTokenNeedsCommentBuilder {
>     constexpr NextTokenNeedsCommentBuilder(std::initializer_list<CSSParserTokenType> tokens)
>     {
>         for (auto token : tokens)
>             buffer[token] = false;
>     }
> 
>     std::array<bool, numberOfCSSParserTokenTypes> buffer { false };
> };
> ```
> 
> and have the lambda take a `const NextTokenNeedsCommentBuilder&` passed via the initializer_list as you recommended below.

Err, that loop should be `buffer[token] = true`. Turns out testing does help!

[Keith Miller]

Created attachment 410048 [details]
Patch for landing

[EWS]

Committed r267766: <https://trac.webkit.org/changeset/267766>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410048 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69764946>