Bug 224718 – [css-counter-styles] Parse @counter-style descriptors

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tyler Wilcock]

https://drafts.csswg.org/css-counter-styles-3/#the-counter-style-rule

[Tyler Wilcock]

Created attachment 426339 [details]
Patch

[Tyler Wilcock]

Created attachment 426370 [details]
Patch

[Tyler Wilcock]

Created attachment 426371 [details]
Patch

[Tyler Wilcock]

Created attachment 426507 [details]
Patch

[Tyler Wilcock]

Created attachment 426509 [details]
Patch

[Tyler Wilcock]

Created attachment 426620 [details]
Patch

[Tyler Wilcock]

> This test is skipped on Windows because I haven't been able to get the required feature flags (CSSCounterStyleAtRulesEnabled and CSSCounterStyleAtRuleImageSymbolsEnabled) to work properly for that port. 

The changelog goes into a lot more detail, so I won't quote it all -- just wanted to add that if skipping this test on Windows is unacceptable I can keep looking into it.

[Darin Adler]

Comment on attachment 426620 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4006
> +        /* These are intentionally unimplemented because they are actually descriptors for @counter-style. */

What’s going on with the C-style comments in this file? We use the C++ style "//" almost everywhere else.

> Source/WebCore/css/CSSCounterStyleRule.cpp:161
> +    StringBuilder result;

This is an old fashioned and inefficient way to write this function. Here’s how we should write it instead:

    String systemText = system();
    const char* systemPrefix = systemText.isEmpty() ? "" : " system: ";
    const char* systemSuffix = systemText.isEmpty() ? "" : ";";

    ...

    return makeString("@counter-style ", name, "  {", systemPrefix, systemText, systemSuffix ...

Continue following that same pattern for all the strings. This is much more efficient than using StringBuilder, because it only allocates a single large string rather than many temporaries and much resizing.

[Tyler Wilcock]

Created attachment 426678 [details]
Patch

[Tyler Wilcock]

> What’s going on with the C-style comments in this file? We use the C++ style
> "//" almost everywhere else.
Not sure.  This latest patch uses the correct style for the newly added lines.  I uploaded another patch to fix the others in this file: https://bugs.webkit.org/show_bug.cgi?id=224875
 
> > Source/WebCore/css/CSSCounterStyleRule.cpp:161
> > +    StringBuilder result;
> 
> This is an old fashioned and inefficient way to write this function. Here’s
> how we should write it instead:
Fixed.

[Darin Adler]

Comment on attachment 426678 [details]
Patch

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

> Source/WebCore/css/CSSCounterStyleRule.cpp:201
> +    return makeString("@counter-style ", name(), " {", systemPrefix, systemText, systemSuffix, symbolsPrefix, symbolsText, symbolsSuffix, additiveSymbolsPrefix, additiveSymbolsText, additiveSymbolsSuffix, negativePrefix, negativeText, negativeSuffix, prefixTextPrefix, prefixText, prefixTextSuffix, suffixTextPrefix, suffixText, suffixTextSuffix, padPrefix, padText, padSuffix, rangePrefix, rangeText, rangeSuffix, fallbackPrefix, fallbackText, fallbackSuffix, speakAsPrefix, speakAsText, speakAsSuffix, " }");

It’s OK to have a single long line. Could also break this logically like this:

    return makeString("@counter-style ", name(), " {",
        systemPrefix, systemText, systemSuffix,
        symbolsPrefix, symbolsText, symbolsSuffix,
        ...
        "  }"
    )

Or some variation on that format.

> LayoutTests/imported/w3c/ChangeLog:22
> +        These <string> values serialize without quotes because WebKit's representation
> +        of custom identifiers is not a separate type, but instead overloaded onto the
> +        CSS_STRING type.  This means that during serialization time, WebKit must guess
> +        whether it is actually serializing a string (and include quotes if so), or if
> +        it's serializing a custom ident (leaving off quotes if so).

So we will fix this.

[Tyler Wilcock]

> It’s OK to have a single long line. Could also break this logically like
> this:
OK, good to know.  Yeah, I couldn't find anything in the style guide[1] about line-length limits, and saw some other really long makeString(...)s, so I went with a single line.

> So we will fix this.
Is fixing it blocking for this patch?  In case not, I've marked cq?.  I did find a related bug: https://bugs.webkit.org/show_bug.cgi?id=28869 

[1]: https://webkit.org/code-style-guidelines/

[Darin Adler]

(In reply to Tyler Wilcock from comment #12)
> Is fixing it blocking for this patch?

No.

[Darin Adler]

(In reply to Tyler Wilcock from comment #12)
> Yeah, I couldn't find anything in the style guide[1]
> about line-length limits

We don’t have any that we enforce.

> and saw some other really long makeString(...)s,
> so I went with a single line.

Sure. At a certain point it’s an aesthetic judgement, not something we have hard and fast rules for. My goal was to show you something that fits within our coding style that might be even easier to read than the single line.

I try to keep in mind that we always have the goal: make the code easy to read and understand.

[EWS]

Committed r276380 (236855@main): <https://commits.webkit.org/236855@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 224912

[Radar WebKit Bug Importer]

<rdar://problem/76996739>

[Tyler Wilcock]

The patch was reverted due to a stack-use-after-scope error caught by AddressSanitizer.  See here for the full ASAN report of one failure: https://bugs.webkit.org/show_bug.cgi?id=224912#c3

> void CSSCounterStyleRule::setterInternal(CSSPropertyID propertyID, const String& valueText)
> {
>     auto tokens = CSSTokenizer(valueText).tokenRange();
>     auto newValue = CSSPropertyParser::parseCounterStyleDescriptor(propertyID, tokens, parserContext());
The anonymous CSSTokenizer is created and destroyed in this line, which invalidates the token range.

I'll upload a new patch with this fixed, and with a non-single line makeString.

[Tyler Wilcock]

Created attachment 426858 [details]
Patch

[Tyler Wilcock]

https://bugs.webkit.org/attachment.cgi?oldid=426678&action=interdiff&newid=426858&headers=1

Here's the diff between the previous patch and the latest patch.

Notable changes:

* CSSParserContext::isPropertyRuntimeDisabled disables the newly added @counter-style descriptor properties based on the feature flag state in CSSParserContext.

* Use `consumeInteger(range, 0)` instead of `consumeInteger(range, ValueRangeNonNegative)`.  consumeInteger takes a minimum value, not a ValueRange.

* The "settings-flag" fields were in the wrong place for the newly added properties in CSSProperties.json.  They have been moved to the correct spot under the "codegen-properties" object.

* stack-use-after-scope fixed by storing the CSSTokenizer in a named local variable so that it lives long enough to be used by the parser.  I re-ran the tests with an ASan build and they now pass.

Darin, when you find some time, could you please review this new patch?  Thanks as always for your reviews.

[Darin Adler]

Comment on attachment 426858 [details]
Patch

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

> Source/WebCore/css/CSSCounterStyleRule.cpp:224
> +    auto tokenRange = tokenizer.tokenRange();

Would be better without a local variable for tokenRange?

> Source/WebCore/css/CSSCounterStyleRule.cpp:236
> +    auto tokenRange = tokenizer.tokenRange();

Would be better without a local variable for tokenRange?

[Darin Adler]

(In reply to Tyler Wilcock from comment #20)
> * Use `consumeInteger(range, 0)` instead of `consumeInteger(range,
> ValueRangeNonNegative)`.  consumeInteger takes a minimum value, not a
> ValueRange.

How is this tested? How did we miss it last time?

> * The "settings-flag" fields were in the wrong place for the newly added
> properties in CSSProperties.json.  They have been moved to the correct spot
> under the "codegen-properties" object.

Can we create something that tests this, so we catch it if we do it wrong in the future?

[EWS]

Committed r276488 (236947@main): <https://commits.webkit.org/236947@main>

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

[Tyler Wilcock]

> Would be better without a local variable for tokenRange?
We could do this if these functions took a const CSSParserTokenRange& rather than a CSSParserTokenRange&, but they can't be const because the parsing process consumes the range.  Another alternative would be to take the range by value, CSSParserTokenRange.

Here's what the error looks like without the tokenRange local:

../../../Source/WebCore/css/CSSCounterStyleRule.cpp:236:100: error: cannot bind non-const lvalue reference of type ‘WebCore::CSSParserTokenRange&’ to an rvalue of type ‘WebCore::CSSParserTokenRange’
  236 |     auto newValue = CSSPropertyParser::parseCounterStyleDescriptor(propertyID, tokenizer.tokenRange(), parserContext());

> * Use `consumeInteger(range, 0)` instead of `consumeInteger(range,
> ValueRangeNonNegative)`.  consumeInteger takes a minimum value, not a
> ValueRange.
> 
> How is this tested? How did we miss it last time?
This compiled because ValueRangeNonNegative is the second variant of `enum ValueRange` and was coerced to 1.0 (the minimum value parameter) in the call to consumeInteger.  This means we would've incorrectly disallowed 0 values for the `pad` and `additive-symbols` descriptors.  Neither of the WPTs for these descriptors test zero values, so I'll open a PR and get that added.

ValueRange being an `enum class` also would've prevented this.  I can look into doing that.

> * The "settings-flag" fields were in the wrong place for the newly added
> properties in CSSProperties.json.  They have been moved to the correct spot
> under the "codegen-properties" object.
> 
> Can we create something that tests this, so we catch it if we do it wrong in the future?
It seems like this should've been caught by jsonchecker.py, which ensures there are no unexpected keys:

https://github.com/WebKit/WebKit/blob/581b92165b792160d4b87f8551b1ea3a1eadb6a1/Tools/Scripts/webkitpy/style/checkers/jsonchecker.py#L263

I will figure out why this didn't trip the automated lint.

[Tyler Wilcock]

Here are the followups:

>  Neither of the WPTs for these descriptors test zero values, so I'll open a PR and get that added.
https://github.com/web-platform-tests/wpt/pull/28665

> ValueRange being an `enum class` also would've prevented this.  I can look into doing that.
https://bugs.webkit.org/show_bug.cgi?id=224981

> I will figure out why this didn't trip the automated lint.
https://bugs.webkit.org/show_bug.cgi?id=224978