Bug 223150

Summary: [css-counter-styles] Parse and add feature flag for @counter-style
Product: WebKit Reporter: Tyler Wilcock <twilco.o>
Component: CSSAssignee: Tyler Wilcock <twilco.o>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, benjamin, calvaris, cdumez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jbedard, joepeck, kai.hollberg, kondapallykalyan, macpherson, menard, ryuan.choi, sam, sergio, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 167645    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tyler Wilcock 2021-03-13 09:19:19 PST
https://drafts.csswg.org/css-counter-styles-3
Comment 1 Tyler Wilcock 2021-03-13 11:05:40 PST
Created attachment 423104 [details]
Patch
Comment 2 Tyler Wilcock 2021-03-14 10:02:18 PDT
Created attachment 423130 [details]
Patch
Comment 3 Tyler Wilcock 2021-03-14 15:32:57 PDT
Created attachment 423135 [details]
Patch
Comment 4 Tyler Wilcock 2021-03-14 19:37:45 PDT
Created attachment 423139 [details]
Patch
Comment 5 Tyler Wilcock 2021-03-17 09:40:55 PDT
Created attachment 423497 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2021-03-22 04:37:53 PDT
<rdar://problem/75685352>
Comment 7 Tyler Wilcock 2021-03-26 11:22:21 PDT
Created attachment 424377 [details]
Patch
Comment 8 Tyler Wilcock 2021-03-27 12:32:30 PDT
Created attachment 424461 [details]
Patch
Comment 9 Darin Adler 2021-03-30 08:39:35 PDT
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.
Comment 10 Tyler Wilcock 2021-03-30 17:49:05 PDT
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
Comment 11 Tyler Wilcock 2021-03-31 15:33:44 PDT
Created attachment 424833 [details]
Patch
Comment 12 Darin Adler 2021-03-31 18:22:32 PDT
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.
Comment 13 Tyler Wilcock 2021-03-31 18:42:07 PDT
Created attachment 424858 [details]
Patch
Comment 14 Tyler Wilcock 2021-04-01 10:08:05 PDT
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
Comment 15 Darin Adler 2021-04-01 10:22:24 PDT
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;
    }
Comment 16 Tyler Wilcock 2021-04-02 07:17:10 PDT
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
Comment 17 Tyler Wilcock 2021-04-16 08:51:57 PDT
Created attachment 426231 [details]
Patch
Comment 18 Tyler Wilcock 2021-04-16 11:45:35 PDT
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?
Comment 19 EWS 2021-04-16 12:19:14 PDT
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].