Bug 223150 – [css-counter-styles] Parse and add feature flag for @counter-style

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

[Tyler Wilcock]

https://drafts.csswg.org/css-counter-styles-3

[Tyler Wilcock]

Created attachment 423104 [details]
Patch

[Tyler Wilcock]

Created attachment 423130 [details]
Patch

[Tyler Wilcock]

Created attachment 423135 [details]
Patch

[Tyler Wilcock]

Created attachment 423139 [details]
Patch

[Tyler Wilcock]

Created attachment 423497 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75685352>

[Tyler Wilcock]

Created attachment 424377 [details]
Patch

[Tyler Wilcock]

Created attachment 424461 [details]
Patch

[Darin Adler]

Comment on attachment 424461 [details]
Patch

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

I’m not a huge fan of the abbreviation "ident". Generally in WebKit code we try to use words rather than abbreviations. I see that "ident" is becoming more common, though.

> Source/WebCore/css/CSSCounterStyleRule.cpp:25
> + */
> +#include "config.h"

Normally we’d leave a blank line here.

> Source/WebCore/css/CSSCounterStyleRule.cpp:36
> +#include "CSSDeferredParser.h"
> +#include "CSSParser.h"
> +#include "CSSPropertyParser.h"
> +#include "CSSRuleList.h"
> +#include "CSSStyleSheet.h"
> +#include "CSSTokenizer.h"
> +#include "Document.h"
> +#include "PropertySetCSSStyleDeclaration.h"
> +#include <wtf/text/StringBuilder.h>

These seem like too many includes. For example, I see nothing here using StringBuilder.

> Source/WebCore/css/CSSCounterStyleRule.h:65
> +    /** FIXME: Implement after we parse @counter-style descriptors. */

This format, C-style comments with two stars, is not used in WebKit code. Just use // for these comments as we do elsewhere.

> Source/WebCore/css/CSSCounterStyleRule.h:77
> +    /** FIXME: Implement after we parse @counter-style descriptors. */

Ditto.

> Source/WebCore/css/CSSRule.h:52
>          SUPPORTS_RULE = 12,

Can take out the "= 12" now.

> Source/WebCore/css/StyleSheetContents.cpp:39
> +#include "RuntimeEnabledFeatures.h"

Why does the patch add this include? I don’t see any use of it in this file.

> Source/WebCore/css/parser/CSSAtRuleID.cpp:32
> +#include "RuntimeEnabledFeatures.h"

Why does the patch add this include? I don’t see any use of it in this file.

> Source/WebCore/css/parser/CSSParserImpl.cpp:660
> +    CSSParserTokenRange rangeCopy = prelude; // For inspector callbacks

I suggest using auto here.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:654
> +    auto stringView = range.consumeIncludingWhitespace().value();

I think that stringView is not a great name for this local. I suggest identifier or ident.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2872
> +    return valueID >= CSSValueDisc && valueID <= CSSValueKatakanaIroha;

It’s not a good design to keep sprinkling separate checks for the "first and last" of CSS_PROP_LIST_STYLE_TYPE around the code. Please find a way to share this check with consumeCounterContent in CSSPropertyParser.cpp and CSSParserFastPaths::isValidKeywordPropertyAndValue.

> Tools/ChangeLog:16
> +        * Scripts/webkitpy/style/checker.py:
> +        Exclude Source/WebCore/css/CSSRule.h from the enum_casing lint.
> +        These enum variants are named to match IDL attributes, and thus should
> +        not be InterCaps cased as the lint suggests.

This doesn’t seem like a great pattern. Will exempting a whole source file be sufficient? If we ever have to use the constants anywhere, won’t we still get a warning.

Not thrilled with more special cases for certain file names.

[Tyler Wilcock]

Thanks for the review!

> > Tools/ChangeLog:16
> > +        * Scripts/webkitpy/style/checker.py:
> > +        Exclude Source/WebCore/css/CSSRule.h from the enum_casing lint.
> > +        These enum variants are named to match IDL attributes, and thus should
> > +        not be InterCaps cased as the lint suggests.
> 
> This doesn’t seem like a great pattern. Will exempting a whole source file
> be sufficient? If we ever have to use the constants anywhere, won’t we still
> get a warning.
> 
> Not thrilled with more special cases for certain file names.
The lint only checks the definition of the enum, so we wouldn't have to worry about usage also triggering the lint.  See relevant code, _EnumState#process_clean_line from cpp.py[1].

Here's the lint message I was trying to avoid:

ERROR: Source/WebCore/css/CSSRule.h:50:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]

The `Type` enum in CSSRule.h [2] breaks this rule today without this new patch, and only "passes" because the "= 12," tricks the regex used in _EnumState#process_clean_line.

    enum Type {
        UNKNOWN_RULE,
        STYLE_RULE,
        CHARSET_RULE,
        IMPORT_RULE,
        MEDIA_RULE,
        FONT_FACE_RULE,
        PAGE_RULE,
        KEYFRAMES_RULE,
        KEYFRAME_RULE,
        MARGIN_RULE,
        NAMESPACE_RULE,
        SUPPORTS_RULE = 12,
    };

That being said, I just noticed here[3] that adding a comment of // WebIDL enum above an enum will cause this lint to be skipped.  This seems appropriate here since these variants are cased to match IDL values, and allows us to not turn off the lint for the entire file.

[1]: https://github.com/WebKit/WebKit/blob/176b84eddd8d2e22b9bb94ba0365bd3ae648ec43/Tools/Scripts/webkitpy/style/checkers/cpp.py#L1345

[2]: https://github.com/WebKit/WebKit/blob/176b84eddd8d2e22b9bb94ba0365bd3ae648ec43/Source/WebCore/css/CSSRule.h#L39

[3]: https://github.com/WebKit/WebKit/blob/176b84eddd8d2e22b9bb94ba0365bd3ae648ec43/Tools/Scripts/webkitpy/style/checkers/cpp.py#L2503

[Tyler Wilcock]

Created attachment 424833 [details]
Patch

[Darin Adler]

Comment on attachment 424833 [details]
Patch

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

> Source/WebCore/css/CSSCounterStyleRule.h:37
> +    static Ref<StyleRuleCounterStyle> create(const AtomString &name, Ref<StyleProperties>&&);

Formatting mistake here. Should be const AtomString& with no space before the "&".

> Source/WebCore/css/CSSCounterStyleRule.h:54
> +    static Ref<CSSCounterStyleRule> create(StyleRuleCounterStyle& rule, CSSStyleSheet* sheet) { return adoptRef(*new CSSCounterStyleRule(rule, sheet)); }

This function body should be in the .cpp file. There’s no value to inlining it. The constructor should be inlined in the create function, no need to have the create function inlined in the code that calls that.

[Tyler Wilcock]

Created attachment 424858 [details]
Patch

[Tyler Wilcock]

Hmm, looks like after this patch[1] landed last night, my patch fails release unified GTK builds. I can also reproduce this exact error consistently on `master` (i.e. without my patch) with `--gtk --release --no-unified-builds`.

If anyone has any ideas on how to fix this I would really appreciate the advice.  AFAICT, `IDLUndefined` is indeed a primary expression[2], and JSWebGLLoseContext.cpp includes IDLTypes.h which is where `IDLUndefined` is defined.

Because this is platform-specific with different behavior between unified and non-unified builds, I'm guessing an include is missing somewhere...but I'm not sure how to find out what or where.

Error:

_/WebCore/DerivedSources/JSWebGLLoseContext.cpp.o -c WebCore/DerivedSources/JSWebGLLoseContext.cpp
In file included from JavaScriptCore/PrivateHeaders/JavaScriptCore/ThrowScope.h:28,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/JSString.h:33,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/JSObject.h:47,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/JSArray.h:27,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayAllocationProfile.h:29,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/JSGlobalObject.h:24,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/InternalFunctionAllocationProfile.h:28,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/FunctionRareData.h:28,
                 from JavaScriptCore/PrivateHeaders/JavaScriptCore/JSFunction.h:26,
                 from WebCore/DerivedSources/JSDOMBindingInternalsBuiltins.h:35,
                 from WebCore/DerivedSources/WebCoreJSBuiltinInternals.h:38,
                 from ../../Source/WebCore/bindings/js/JSDOMGlobalObject.h:29,
                 from ../../Source/WebCore/bindings/js/JSDOMWrapper.h:24,
                 from WebCore/DerivedSources/JSWebGLLoseContext.h:25,
                 from WebCore/DerivedSources/JSWebGLLoseContext.cpp:25:
WebCore/DerivedSources/JSWebGLLoseContext.cpp: In function ‘JSC::EncodedJSValue WebCore::jsWebGLLoseContextPrototypeFunction_loseContextBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::IDLOperation<WebCore::JSWebGLLoseContext>::ClassParameter)’:
WebCore/DerivedSources/JSWebGLLoseContext.cpp:147:78: error: expected primary-expression before ‘>’ token
  147 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
      |                                                                              ^
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:147:80: warning: value computed is not used [-Wunused-value]
  147 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<WebCore::IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
      |                                                                                ^~~~~~~~~~~~~~~~~~~~
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:147:80: warning: left operand of comma operator has no effect [-Wunused-value]
  147 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<WebCore::IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
      |                                                                                ^~~~~~~~~~~~~~~~~~~~
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:147:167: warning: right operand of comma operator has no effect [-Wunused-value]
  147 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<WebCore::IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.loseContext(); })));
      |                                                                                                                                                                       ^
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp: In function ‘JSC::EncodedJSValue WebCore::jsWebGLLoseContextPrototypeFunction_restoreContextBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::IDLOperation<WebCore::JSWebGLLoseContext>::ClassParameter)’:
WebCore/DerivedSources/JSWebGLLoseContext.cpp:162:69: error: expected primary-expression before ‘>’ token
  162 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
      |                                                                     ^
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:162:71: warning: value computed is not used [-Wunused-value]
  162 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
      |                                                                       ^~~~~~~~~~~~~~~~~~~~
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:162:71: warning: left operand of comma operator has no effect [-Wunused-value]
  162 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
      |                                                                       ^~~~~~~~~~~~~~~~~~~~
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~
WebCore/DerivedSources/JSWebGLLoseContext.cpp:162:161: warning: right operand of comma operator has no effect [-Wunused-value]
  162 |     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.restoreContext(); })));
      |                                                                                                                                                                 ^
JavaScriptCore/PrivateHeaders/JavaScriptCore/ExceptionScope.h:113:16: note: in definition of macro ‘RELEASE_AND_RETURN’
  113 |         return expression__; \
      |                ^~~~~~~~~~~~


[1]: https://github.com/WebKit/WebKit/compare/68ef98af0e41...38078b56fd06

[2]: https://docs.microsoft.com/en-us/cpp/cpp/primary-expressions

https://github.com/WebKit/WebKit/blob/main/Introduction.md#build-failures-with-unified-sources

[Darin Adler]

I looked at those errors above. I think the issue is this: For "undefined" the CodeGeneratorJS.pm script adds an include of IDLTypes.h:

    if ($type->name eq "undefined") {
        AddToIncludes("IDLTypes.h", $includesRef, $conditional);
        return;
    }

After studying the generated code a bit, it seems that it should also generate an include of the file JSDOMConvertBase.h. Sam might have some insight into whether that’s exactly right.

If I am correct, then we just need to add another line of code:

    if ($type->name eq "undefined") {
        AddToIncludes("IDLTypes.h", $includesRef, $conditional);
        AddToIncludes("JSDOMConvertBase.h", $includesRef, $conditional);
        return;
    }

[Tyler Wilcock]

Thanks, that fixed it.  I've uploaded a patch with that change and a few other non-unified build fixes: https://bugs.webkit.org/show_bug.cgi?id=224091

[Tyler Wilcock]

Created attachment 426231 [details]
Patch

[Tyler Wilcock]

The previous patch was stale, so I've rebased and pushed a new one.

I think all review comments have been addressed.  Darin, when you find some time, could you please take another look at this and cq+ if all looks good?

[EWS]

Committed r276152 (236643@main): <https://commits.webkit.org/236643@main>

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