Implementing CounterStyle representation from StyleRuleCounterStyle #7114

vitorroriz · 2022-12-03T16:08:48Z

`ac2f8a4`

Implementing CounterStyle representation from StyleRuleCounterStyle
https://bugs.webkit.org/show_bug.cgi?id=248666
rdar://102910060

Reviewed by Antti Koivisto.
Reviewed by Antti Koivisto.

* Source/WebCore/Headers.cmake:
* Source/WebCore/Sources.txt:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
Adding CSSCounterStyle and CSSCounterStyleDescriptors .h and .cpp files.

* Source/WebCore/css/CSSCounterStyle.cpp: Added.
(WebCore::CounterStyle::isInRange const):
Returns true if counter is in range, as defined by
https://www.w3.org/TR/css-counter-styles-3/#descdef-counter-style-range.

(WebCore::CounterStyle::CounterStyle):
(WebCore::CounterStyle::create):

(WebCore::CounterStyle::createCounterStyleDecimal):
Create a CounterStyle object representing a Decimal counter.

(WebCore::CounterStyle::setFallbackReference):

(WebCore::CounterStyle::extendAndResolve):
Extend another CounterStyle as defined by
https://www.w3.org/TR/css-counter-styles-3/#extends-system and resolve
its Extend reference. The counter's system value is then promoted
from 'extend' to the valye of the CounterStyle we are extending.

(WebCore::CounterStyle::text):
Dummy text representation for the counter. The real representation wil
be implemented by rdar://103648354.

* Source/WebCore/css/CSSCounterStyle.h: Added.
(WebCore::CounterStyle::operator== const):
(WebCore::CounterStyle::fallbackName const):
(WebCore::CounterStyle::extendsName const):
(WebCore::CounterStyle::isFallbackUnresolved):
(WebCore::CounterStyle::isExtendsUnresolved):
(WebCore::CounterStyle::isExtendsSystem const):
(WebCore::CounterStyle::system const):
(WebCore::CounterStyle::isPredefinedCounterStyle const):
Returns true if CounterStyle is predefined by the user agent stylesheet.
A counter style parsed from author-defined rule will return false.

(WebCore::CounterStyle::isAutoRange const):
Returns true if the range descriptor is set to 'auto'. A 'auto' value
is represented by a empty vector of ranges.

* Source/WebCore/css/CSSCounterStyleRule.cpp:
(WebCore::toCounterStyleSystemEnum):
* Source/WebCore/css/CSSCounterStyleRule.h:
Moving enum to CSSCounterStyle.h.
Adding getters for the properties's CSS values.

* Source/WebCore/css/CSSStyleSheet.cpp:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeCounterStyleSystem):
* Source/WebCore/dom/Element.h:
* Source/WebCore/style/MatchedDeclarationsCache.cpp:
(WebCore::Style::MatchedDeclarationsCache::isCacheable):
Adding FIXME with matching radars.

CSSCounterStyleDescriptors defines the descriptors for the counter-style
rule in a specialized manner. It serves as a payload for CSSCounterStyle.

* Source/WebCore/css/CSSCounterStyleDescriptors.cpp: Added.
(WebCore::translateRangeFromStyleProperties):
(WebCore::symbolToString):
(WebCore::translatePadFromStyleProperties):
(WebCore::translateNegativeSymbolsFromStyleProperties):
(WebCore::translateSymbolsFromStyleProperties):
(WebCore::translateFallbackNameFromStyleProperties):
(WebCore::translatePrefixFromStyleProperties):
(WebCore::translateSuffixFromStyleProperties):
(WebCore::extractDataFromSystemDescriptor):
(WebCore::CSSCounterStyleDescriptors::setExplicitlySetDescriptors):
Translation from CSSValues of rule's properties to adequated
specialized types.
(WebCore::CSSCounterStyleDescriptors::create):
Creates an instance of CSSCounterStyleDescriptor based on
StyleProperties. A create function was preferred instead of a
constructor in order to maintain the class as a aggregate, enabling
desginated initializers.
* Source/WebCore/css/CSSCounterStyleDescriptors.h: Added.

Canonical link: https://commits.webkit.org/258434@main

3006368

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-03T16:09:12Z

EWS run on previous version of this PR (hash 848a0ff)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk1	❌ 🧪 api-gtk
	❌ 🛠 tv	❌ 🧪 mac-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	❌ 🛠 watch	❌ 🧪 mac-wk2-stress
	❌ 🛠 watch-sim

webkit-early-warning-system · 2022-12-03T16:55:16Z

EWS run on previous version of this PR (hash 5792294)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	❌ 🛠 ios-sim	❌ 🛠 mac-AS-debug	❌ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🧪 gtk-wk2
	❌ 🧪 api-ios	❌ 🧪 mac-wk1	❌ 🧪 api-gtk
	❌ 🛠 tv	❌ 🧪 mac-wk2
	❌ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	❌ 🛠 watch	❌ 🧪 mac-wk2-stress
	❌ 🛠 watch-sim

webkit-early-warning-system · 2022-12-03T17:53:35Z

EWS run on previous version of this PR (hash a1e3c1e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	❌ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-06T13:51:47Z

EWS run on previous version of this PR (hash 83b454c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	❌ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
⏳ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-20T23:16:48Z

EWS run on previous version of this PR (hash 7361ba0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
~~🧪 bindings~~	~~🛠 ios-sim~~	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk2
	⏳ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-wk2-stress
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2022-12-20T23:20:06Z

EWS run on previous version of this PR (hash 0a79b29)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	❌ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	⏳ 🧪 api-mac	⏳ 🧪 gtk-wk2
	❌ 🧪 api-ios	⏳ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk2
	⏳ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-wk2-stress
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2022-12-21T00:22:08Z

EWS run on previous version of this PR (hash 2903417)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	❌ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-22T13:54:12Z

EWS run on previous version of this PR (hash 2d5d818)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

anttijk · 2022-12-22T19:34:07Z

Source/WebCore/css/CSSCounterStyle.cpp

+CounterStyle::CounterStyle(Name name, CounterStyleSystem system, NegativeSymbols negativeSymbols, Symbol prefix, Symbol suffix, Ranges&& ranges, Pad pad, Name fallback, Vector<Symbol>&& symbols, CounterStyleSpeakAs speakAs, Name speakAsReference)
+: m_predefinedCounterStyle { true }, m_name { name }, m_system { system }, m_negativeSymbols { negativeSymbols }, m_prefix { prefix }, m_suffix { suffix }, m_ranges { WTFMove(ranges) }, m_pad { pad }, m_fallbackName { fallback }, m_symbols { WTFMove(symbols) }, m_speakAs { speakAs }, m_speakAsNameReference { speakAsReference }
+{ }
+
+CounterStyle::CounterStyle(const StyleRuleCounterStyle& rule)
+: m_name { rule.name() }, m_system { toCounterStyleSystemEnum(rule.systemCSSValue()) }, m_negativeSymbols { translateNegativeSymbolsFromStyleRule(rule) }, m_prefix { translatePrefixFromStyleRule(rule) }, m_suffix { translateSuffixFromStyleRule(rule) }, m_ranges { translateRangeFromStyleRule(rule) }, m_pad { translatePadFromStyleRule(rule) }, m_fallbackName { translateFallbackNameFromStyleRule(rule) }, m_symbols { translateSymbolsFromStyleRule(rule) }, m_speakAs { CounterStyleSpeakAs::Auto }, m_speakAsNameReference { ""_s }


That's a lot of constructor parameters. Maybe this type could be a struct (with public fields) instead of a class? Then you just initialize it with { ... } without writing an explicit constructor.

CounterStyle is a derived class so it won't be classified as an aggregate, right?
https://en.cppreference.com/w/cpp/language/aggregate_initialization#Designated_initializers

Right but the question is if should/needs to be refcounted. Alternatively you could have a separate pure struct that just contains the fields. CounterStyleDescriptors or something.

The formatting does not follow WebKit style. Each field initialization should be in a separate line, indented.

anttijk · 2022-12-22T19:37:55Z

Source/WebCore/css/CSSCounterStyleRule.h

+    SpellOut,
+    CounterStyleNameReference,
+};
+
 class StyleRuleCounterStyle final : public StyleRuleBase {


This should really be in a file of its own.

anttijk · 2022-12-22T19:43:42Z

Source/WebCore/css/CSSCounterStyleRule.h

+    RefPtr<CSSValue> systemCSSValue() const { return getPropertyCSSValue(CSSPropertySystem); }
+    RefPtr<CSSValue> negativeCSSValue() const { return getPropertyCSSValue(CSSPropertyNegative); }
+    RefPtr<CSSValue> prefixCSSValue() const { return getPropertyCSSValue(CSSPropertyPrefix); }
+    RefPtr<CSSValue> suffixCSSValue() const { return getPropertyCSSValue(CSSPropertySuffix); }
+    RefPtr<CSSValue> rangeCSSValue() const { return getPropertyCSSValue(CSSPropertyRange); }
+    RefPtr<CSSValue> padCSSValue() const { return getPropertyCSSValue(CSSPropertyPad); }
+    RefPtr<CSSValue> fallbackCSSValue() const { return getPropertyCSSValue(CSSPropertyFallback); }
+    RefPtr<CSSValue> symbolsCSSValue() const { return getPropertyCSSValue(CSSPropertySymbols); }
+    RefPtr<CSSValue> additiveSymbolsCSSValue() const { return getPropertyCSSValue(CSSPropertyAdditiveSymbols); }
+    RefPtr<CSSValue> speakAsCSSValue() const { return getPropertyCSSValue(CSSPropertySpeakAs); }


I know you didn't implement this type but the factoring here is really not ideal. Instead of a StyleProperties member there should be a more processed type that represents the counter style as enums/OptionSets or whatever is appropriate of each type.

You have such a type. Maybe you can replace StyleProperties with your CounterStyle type here?

anttijk · 2022-12-22T19:49:48Z

Source/WebCore/css/CSSCounterStyle.h

+    CounterStyleMap m_authorCounterStyles;
+};
+
+class CounterStyle : public RefCounted<CounterStyle>, public CanMakeWeakPtr<CounterStyle> {


This should be in a file of its own.

Filename should match the class name (CSSCounterStyle vs CounterStyle)

anttijk · 2022-12-22T19:50:14Z

Source/WebCore/css/CSSCounterStyle.h

+class CounterStyle;
+using CounterStyleMap = HashMap<AtomString, RefPtr<CounterStyle>>;
+
+class CounterStyleManager : public RefCounted<CounterStyleManager> {


We don't like Managers. How about Registry?

This should be in a file of its own.

anttijk · 2022-12-22T19:56:15Z

Source/WebCore/css/CSSCounterStyle.h

+    WeakPtr<const CounterStyle> m_extendsReference { nullptr };
+    int m_fixedSystemFirstSymbolValue { 1 };
+
+    OptionSet<ExplicitlySetDescriptors> m_explicitlySetDescriptors;


Instead of maintaining this OptionSet maybe you could make the individual fields std::optional<>?

It doesn't achieve the same intent. All the descriptors values are defined, implicitly or explicitly. No explicit values should not be overridden when resolving Extends. I.E.: when CounterStyleA extends CounterStyleB, A will just inherit the values from the descriptors that were explicitly defined by the author, but all the descriptors have at least an implicit value.

webkit-early-warning-system · 2022-12-22T20:45:20Z

EWS run on previous version of this PR (hash 6b429b3)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2022-12-23T16:33:01Z

EWS run on previous version of this PR (hash 213f69c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-02T12:46:41Z

EWS run on previous version of this PR (hash d71b203)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	⏳ 🧪 gtk-wk2
	✅ 🧪 api-ios	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-02T13:46:22Z

EWS run on previous version of this PR (hash 166f529)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-02T16:31:19Z

EWS run on previous version of this PR (hash 832fb78)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

anttijk · 2023-01-03T11:57:08Z

Source/WebCore/css/CSSCounterStyle.h

+    CounterStyleMap m_authorCounterStyles;
+};
+
+class CounterStyle : public RefCounted<CounterStyle>, public CanMakeWeakPtr<CounterStyle> {


Filename should match the class name (CSSCounterStyle vs CounterStyle)

anttijk · 2023-01-03T11:59:38Z

Source/WebCore/css/CSSCounterStyle.cpp

+CounterStyle::CounterStyle(Name name, CounterStyleSystem system, NegativeSymbols negativeSymbols, Symbol prefix, Symbol suffix, Ranges&& ranges, Pad pad, Name fallback, Vector<Symbol>&& symbols, CounterStyleSpeakAs speakAs, Name speakAsReference)
+: m_predefinedCounterStyle { true }, m_name { name }, m_system { system }, m_negativeSymbols { negativeSymbols }, m_prefix { prefix }, m_suffix { suffix }, m_ranges { WTFMove(ranges) }, m_pad { pad }, m_fallbackName { fallback }, m_symbols { WTFMove(symbols) }, m_speakAs { speakAs }, m_speakAsNameReference { speakAsReference }
+{ }
+
+CounterStyle::CounterStyle(const StyleRuleCounterStyle& rule)
+: m_name { rule.name() }, m_system { toCounterStyleSystemEnum(rule.systemCSSValue()) }, m_negativeSymbols { translateNegativeSymbolsFromStyleRule(rule) }, m_prefix { translatePrefixFromStyleRule(rule) }, m_suffix { translateSuffixFromStyleRule(rule) }, m_ranges { translateRangeFromStyleRule(rule) }, m_pad { translatePadFromStyleRule(rule) }, m_fallbackName { translateFallbackNameFromStyleRule(rule) }, m_symbols { translateSymbolsFromStyleRule(rule) }, m_speakAs { CounterStyleSpeakAs::Auto }, m_speakAsNameReference { ""_s }


Right but the question is if should/needs to be refcounted. Alternatively you could have a separate pure struct that just contains the fields. CounterStyleDescriptors or something.

anttijk · 2023-01-03T12:01:34Z

Source/WebCore/css/CSSCounterStyle.cpp

+CounterStyle::CounterStyle(Name name, CounterStyleSystem system, NegativeSymbols negativeSymbols, Symbol prefix, Symbol suffix, Ranges&& ranges, Pad pad, Name fallback, Vector<Symbol>&& symbols, CounterStyleSpeakAs speakAs, Name speakAsReference)
+: m_predefinedCounterStyle { true }, m_name { name }, m_system { system }, m_negativeSymbols { negativeSymbols }, m_prefix { prefix }, m_suffix { suffix }, m_ranges { WTFMove(ranges) }, m_pad { pad }, m_fallbackName { fallback }, m_symbols { WTFMove(symbols) }, m_speakAs { speakAs }, m_speakAsNameReference { speakAsReference }
+{ }
+
+CounterStyle::CounterStyle(const StyleRuleCounterStyle& rule)
+: m_name { rule.name() }, m_system { toCounterStyleSystemEnum(rule.systemCSSValue()) }, m_negativeSymbols { translateNegativeSymbolsFromStyleRule(rule) }, m_prefix { translatePrefixFromStyleRule(rule) }, m_suffix { translateSuffixFromStyleRule(rule) }, m_ranges { translateRangeFromStyleRule(rule) }, m_pad { translatePadFromStyleRule(rule) }, m_fallbackName { translateFallbackNameFromStyleRule(rule) }, m_symbols { translateSymbolsFromStyleRule(rule) }, m_speakAs { CounterStyleSpeakAs::Auto }, m_speakAsNameReference { ""_s }


The formatting does not follow WebKit style. Each field initialization should be in a separate line, indented.

anttijk · 2023-01-03T12:05:13Z

Source/WebCore/css/CSSCounterStyle.cpp

+static CounterStyle::Ranges translateRangeFromStyleRule(const StyleRuleCounterStyle& rule)
+{
+    auto ranges = rule.rangeCSSValue();
+    // auto range will return an empty Ranges
+    if (!ranges || !is<CSSValueList>(ranges))
+        return { };
+    auto& list = downcast<CSSValueList>(*ranges);
+    CounterStyle::Ranges result;
+    for (auto& rangeValue : list) {
+        ASSERT(rangeValue->isPrimitiveValue());


Having to parse these from a style declaration in StyleRuleCounterStyle is awkward. I think we could have done this at parse time and saved a CounterStyle (or a type that just contains the parsed fields, CounterStyleDescriptor or something) in StyleRuleCounterStyle.

It is bit unclear to me what CounterStyle class represents. Is it final computed counter style that can be pointed to by RenderStyle and directly used by layout/rendering code? Is it an intermediate type that acts as input for style resolution? Or both?

anttijk · 2023-01-03T12:07:26Z

Source/WebCore/css/CSSCounterStyle.h

+        return m_name == other.m_name
+        && m_system == other.m_system
+        && m_negativeSymbols == other.m_negativeSymbols
+        && m_prefix == other.m_prefix
+        && m_suffix == other.m_suffix
+        && m_ranges == other.m_ranges
+        && m_pad == other.m_pad
+        && m_fallbackName == other.m_fallbackName
+        && m_symbols == other.m_symbols
+        && m_additiveSymbols == other.m_additiveSymbols
+        && m_speakAs == other.m_speakAs
+        && m_speakAsNameReference == other.m_speakAsNameReference
+        && m_predefinedCounterStyle == other.m_predefinedCounterStyle
+        && m_explicitlySetDescriptors == other.m_explicitlySetDescriptors;


&& lines should be indented.

anttijk · 2023-01-03T12:08:28Z

Source/WebCore/css/CSSCounterStyle.h

+    struct Pad {
+        unsigned m_padMinimumLength = 0;
+        Symbol m_padSymbol = "-"_s;
+        bool operator==(const Pad& other) const = default;


Unfortunately I think these still fail to compile in some supported linux configurations.

anttijk · 2023-01-03T12:09:26Z

Source/WebCore/css/CSSCounterStyle.h

+    String counterForSystemCyclic(int);
+    String counterForSystemFixed(int);
+    String counterForSystemSymbolic(int);
+    String counterForSystemAlphabetic(int);
+    String counterForSystemNumeric(int);
+    String counterForSystemAdditive(int);


These are declared but not defined. It would be nicer to at least add stubs.

anttijk · 2023-01-03T12:16:30Z

Source/WebCore/css/CSSCounterStyle.cpp

+    for (auto& symbolValue : list) {
+        auto symbolString = symbolToString(&symbolValue.get());
+        if (!symbolString.isNull())
+            result.constructAndAppend(WTFMove(symbolString));


Why constructAndAppend instead of just append?

anttijk · 2023-01-03T12:16:59Z

Source/WebCore/css/CSSCounterStyle.cpp

+        if (high->isInteger())
+            convertedHigh = high->intValue();
+        std::pair<int, int> newRange { convertedLow, convertedHigh };
+        result.constructAndAppend(WTFMove(newRange));


Why constructAndAppend instead of just append?

anttijk · 2023-01-03T12:37:29Z

Source/WebCore/css/CSSCounterStyle.cpp

+    if (rule.systemCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::System);
+    if (rule.negativeCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Negative);
+    if (rule.prefixCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Prefix);
+    if (rule.suffixCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Suffix);
+    if (rule.rangeCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Range);
+    if (rule.padCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Pad);
+    if (rule.fallbackCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Fallback);
+    if (rule.symbolsCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::Symbols);
+    if (rule.additiveSymbolsCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::AdditiveSymbols);
+    if (rule.speakAsCSSValue())
+        m_explicitlySetDescriptors.add(ExplicitlySetDescriptors::SpeakAs);
+}


It might be better to not try to implement all descriptors in one go. Maybe just start with a few of the most common ones and add the rest when the whole mechanism is in place? It would make these patches easier to follow and review.

webkit-early-warning-system · 2023-01-03T23:59:06Z

EWS run on previous version of this PR (hash a8cbff0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-04T13:09:21Z

EWS run on current version of this PR (hash 3006368)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=248666 rdar://102910060 Reviewed by Antti Koivisto. Reviewed by Antti Koivisto. * Source/WebCore/Headers.cmake: * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: Adding CSSCounterStyle and CSSCounterStyleDescriptors .h and .cpp files. * Source/WebCore/css/CSSCounterStyle.cpp: Added. (WebCore::CounterStyle::isInRange const): Returns true if counter is in range, as defined by https://www.w3.org/TR/css-counter-styles-3/#descdef-counter-style-range. (WebCore::CounterStyle::CounterStyle): (WebCore::CounterStyle::create): (WebCore::CounterStyle::createCounterStyleDecimal): Create a CounterStyle object representing a Decimal counter. (WebCore::CounterStyle::setFallbackReference): (WebCore::CounterStyle::extendAndResolve): Extend another CounterStyle as defined by https://www.w3.org/TR/css-counter-styles-3/#extends-system and resolve its Extend reference. The counter's system value is then promoted from 'extend' to the valye of the CounterStyle we are extending. (WebCore::CounterStyle::text): Dummy text representation for the counter. The real representation wil be implemented by rdar://103648354. * Source/WebCore/css/CSSCounterStyle.h: Added. (WebCore::CounterStyle::operator== const): (WebCore::CounterStyle::fallbackName const): (WebCore::CounterStyle::extendsName const): (WebCore::CounterStyle::isFallbackUnresolved): (WebCore::CounterStyle::isExtendsUnresolved): (WebCore::CounterStyle::isExtendsSystem const): (WebCore::CounterStyle::system const): (WebCore::CounterStyle::isPredefinedCounterStyle const): Returns true if CounterStyle is predefined by the user agent stylesheet. A counter style parsed from author-defined rule will return false. (WebCore::CounterStyle::isAutoRange const): Returns true if the range descriptor is set to 'auto'. A 'auto' value is represented by a empty vector of ranges. * Source/WebCore/css/CSSCounterStyleRule.cpp: (WebCore::toCounterStyleSystemEnum): * Source/WebCore/css/CSSCounterStyleRule.h: Moving enum to CSSCounterStyle.h. Adding getters for the properties's CSS values. * Source/WebCore/css/CSSStyleSheet.cpp: * Source/WebCore/css/CSSValueKeywords.in: * Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp: (WebCore::CSSPropertyParserHelpers::consumeCounterStyleSystem): * Source/WebCore/dom/Element.h: * Source/WebCore/style/MatchedDeclarationsCache.cpp: (WebCore::Style::MatchedDeclarationsCache::isCacheable): Adding FIXME with matching radars. CSSCounterStyleDescriptors defines the descriptors for the counter-style rule in a specialized manner. It serves as a payload for CSSCounterStyle. * Source/WebCore/css/CSSCounterStyleDescriptors.cpp: Added. (WebCore::translateRangeFromStyleProperties): (WebCore::symbolToString): (WebCore::translatePadFromStyleProperties): (WebCore::translateNegativeSymbolsFromStyleProperties): (WebCore::translateSymbolsFromStyleProperties): (WebCore::translateFallbackNameFromStyleProperties): (WebCore::translatePrefixFromStyleProperties): (WebCore::translateSuffixFromStyleProperties): (WebCore::extractDataFromSystemDescriptor): (WebCore::CSSCounterStyleDescriptors::setExplicitlySetDescriptors): Translation from CSSValues of rule's properties to adequated specialized types. (WebCore::CSSCounterStyleDescriptors::create): Creates an instance of CSSCounterStyleDescriptor based on StyleProperties. A create function was preferred instead of a constructor in order to maintain the class as a aggregate, enabling desginated initializers. * Source/WebCore/css/CSSCounterStyleDescriptors.h: Added. Canonical link: https://commits.webkit.org/258434@main

webkit-commit-queue · 2023-01-04T17:15:51Z

Committed 258434@main (ac2f8a4): https://commits.webkit.org/258434@main

Reviewed commits have been landed. Closing PR #7114 and removing active labels.

vitorroriz requested review from rniwa and cdumez as code owners December 3, 2022 16:08

vitorroriz self-assigned this Dec 3, 2022

vitorroriz added the CSS Cascading Style Sheets implementation label Dec 3, 2022

vitorroriz marked this pull request as draft December 3, 2022 16:09

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 3, 2022

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 3, 2022

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 848a0ff to 5792294 Compare December 3, 2022 16:54

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 3, 2022

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 3, 2022

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 5792294 to a1e3c1e Compare December 3, 2022 17:53

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 3, 2022

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 6, 2022

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from a1e3c1e to 83b454c Compare December 6, 2022 13:50

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 6, 2022

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 20, 2022

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 83b454c to 7361ba0 Compare December 20, 2022 23:16

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 7361ba0 to 0a79b29 Compare December 20, 2022 23:19

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 21, 2022

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 21, 2022

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 0a79b29 to 2903417 Compare December 21, 2022 00:15

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 21, 2022

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 21, 2022

vitorroriz marked this pull request as ready for review December 22, 2022 15:37

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 22, 2022

anttijk reviewed Dec 22, 2022

View reviewed changes

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Dec 22, 2022

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 2d5d818 to 6b429b3 Compare December 22, 2022 20:44

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 6b429b3 to 213f69c Compare December 23, 2022 16:32

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 213f69c to d71b203 Compare January 2, 2023 12:46

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from d71b203 to 166f529 Compare January 2, 2023 13:46

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 166f529 to 832fb78 Compare January 2, 2023 16:30

anttijk reviewed Jan 3, 2023

View reviewed changes

anttijk approved these changes Jan 3, 2023

View reviewed changes

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 832fb78 to a8cbff0 Compare January 3, 2023 23:58

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 4, 2023

vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Jan 4, 2023

vitorroriz force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from a8cbff0 to 3006368 Compare January 4, 2023 13:09

vitorroriz added the merge-queue Applied to send a pull request to merge-queue label Jan 4, 2023

webkit-early-warning-system force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from 3006368 to f18c3d3 Compare January 4, 2023 17:12

webkit-early-warning-system force-pushed the eng/Implement-representation-for-CounterStyle-translated-from-StyleRuleCounterStyle branch from f18c3d3 to ac2f8a4 Compare January 4, 2023 17:15

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 4, 2023

webkit-early-warning-system merged commit ac2f8a4 into WebKit:main Jan 4, 2023

Implementing CounterStyle representation from StyleRuleCounterStyle #7114

Implementing CounterStyle representation from StyleRuleCounterStyle #7114

Conversation

vitorroriz commented Dec 3, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Dec 3, 2022 • edited

webkit-early-warning-system commented Dec 3, 2022 • edited

webkit-early-warning-system commented Dec 3, 2022 • edited

webkit-early-warning-system commented Dec 6, 2022 • edited

webkit-early-warning-system commented Dec 20, 2022 • edited

webkit-early-warning-system commented Dec 20, 2022 • edited

webkit-early-warning-system commented Dec 21, 2022 • edited

webkit-early-warning-system commented Dec 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anttijk Dec 22, 2022 • edited by nt1m

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Dec 22, 2022 • edited

webkit-early-warning-system commented Dec 23, 2022 • edited

webkit-early-warning-system commented Jan 2, 2023 • edited

webkit-early-warning-system commented Jan 2, 2023 • edited

webkit-early-warning-system commented Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anttijk Jan 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anttijk Jan 3, 2023 • edited

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 3, 2023 • edited

webkit-early-warning-system commented Jan 4, 2023 • edited

webkit-commit-queue commented Jan 4, 2023

vitorroriz commented Dec 3, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Dec 3, 2022 •

edited

webkit-early-warning-system commented Dec 3, 2022 •

edited

webkit-early-warning-system commented Dec 3, 2022 •

edited

webkit-early-warning-system commented Dec 6, 2022 •

edited

webkit-early-warning-system commented Dec 20, 2022 •

edited

webkit-early-warning-system commented Dec 20, 2022 •

edited

webkit-early-warning-system commented Dec 21, 2022 •

edited

webkit-early-warning-system commented Dec 22, 2022 •

edited

anttijk Dec 22, 2022 •

edited by nt1m

webkit-early-warning-system commented Dec 22, 2022 •

edited

webkit-early-warning-system commented Dec 23, 2022 •

edited

webkit-early-warning-system commented Jan 2, 2023 •

edited

webkit-early-warning-system commented Jan 2, 2023 •

edited

webkit-early-warning-system commented Jan 2, 2023 •

edited

anttijk Jan 3, 2023 •

edited

anttijk Jan 3, 2023 •

edited

webkit-early-warning-system commented Jan 3, 2023 •

edited

webkit-early-warning-system commented Jan 4, 2023 •

edited