[webkit-reviews] review granted: [Bug 223150] [css-counter-styles] Parse and add feature flag for @counter-style : [Attachment 424461] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tyler Wilcock
<twilco.o at protonmail.com>'s request for review:
Bug 223150: [css-counter-styles] Parse and add feature flag for @counter-style
https://bugs.webkit.org/show_bug.cgi?id=223150

Attachment 424461: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 424461
  --> https://bugs.webkit.org/attachment.cgi?id=424461
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.