Refactor StyleRuleCounterStyle for eliminating StyleProperties #12168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 18963fe)
|
vitorroriz
force-pushed
the
refactor-StyleRuleCounterStyle
branch
from
18963fe
to
d52f845
Compare
|
EWS run on previous version of this PR (hash d52f845)
|
anttijk
requested changes
Mar 30, 2023
| @@ -220,104 +160,125 @@ void CSSCounterStyleRule::reattach(StyleRuleBase& rule) | |||
| m_counterStyleRule = downcast<StyleRuleCounterStyle>(rule); | |||
| } | |||
|
|
|||
| RefPtr<CSSValue> CSSCounterStyleRule::CSSValueFromText(CSSPropertyID propertyID, const String& valueText) | |||
There was a problem hiding this comment.
In WebKit coding style function names start with lowercase letter: CSSCounterStyleRule::cssValueFromText
Comment on lines
87
to
99
| static CSSCounterStyleDescriptors::Name nameFromCSSValue(const CSSValue* value) | ||
| { | ||
| if (!value || !value->isPrimitiveValue()) | ||
| return { }; | ||
|
|
||
| auto& primitiveValue = downcast<CSSPrimitiveValue>(*value); | ||
| return makeAtomString(primitiveValue.stringValue()); | ||
| } | ||
|
|
||
| CSSCounterStyleDescriptors::Name nameFromCSSValue(RefPtr<CSSValue> value) | ||
| { | ||
| return nameFromCSSValue(value.get()); | ||
| } |
There was a problem hiding this comment.
We probably don't need two versions of this function, just the RefPtr one.
Comment on lines
104
to
112
| if (!value) | ||
| return { }; | ||
| return additiveSymbolsFromCSSValue(WTFMove(value)); | ||
| } | ||
|
|
||
| CSSCounterStyleDescriptors::AdditiveSymbols additiveSymbolsFromCSSValue(RefPtr<CSSValue> value) | ||
| { | ||
| if (!value) | ||
| return { }; |
There was a problem hiding this comment.
It should be sufficient to null test in one place only. If the caller is responsible then the additiveSymbolsFromCSSValue could take Ref<CSSValue>
This comment applies to other similar functions in this patch.
| auto systemData = extractSystemDataFromCSSValue(WTFMove(systemValue), system); | ||
| if (!mutableDescriptors().setSystemData(WTFMove(systemData))) | ||
| return; | ||
| CSSStyleSheet::RuleMutationScope mutationScope(this); |
There was a problem hiding this comment.
RuleMutationScope should be scoped over the mutation. That is the constructor should run before the mutation and destructor after (here and elsewhere).
anttijk
reviewed
Mar 30, 2023
Comment on lines
-67
to
-68
| // FIXME: we can get rid of m_properties and use only m_descriptors for storing the descriptors data (rdar://107267784). | ||
| Ref<StyleProperties> m_properties; |
vitorroriz
force-pushed
the
refactor-StyleRuleCounterStyle
branch
from
d52f845
to
6f76ee3
Compare
|
EWS run on previous version of this PR (hash 6f76ee3)
|
vitorroriz
force-pushed
the
refactor-StyleRuleCounterStyle
branch
from
6f76ee3
to
7e90aef
Compare
|
EWS run on previous version of this PR (hash 7e90aef)
|
anttijk
approved these changes
Mar 31, 2023
Comment on lines
317
to
322
| bool CSSCounterStyleDescriptors::setSystemData(CSSCounterStyleDescriptors::SystemData systemData) | ||
| { | ||
| if (m_extendsName == systemData.first && m_fixedSystemFirstSymbolValue == systemData.second) | ||
| return false; | ||
| m_extendsName = systemData.first; | ||
| m_fixedSystemFirstSymbolValue = systemData.second; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don't think the return bools in these functions are used. The code could be simplified so these are just simple setters.
vitorroriz
force-pushed
the
refactor-StyleRuleCounterStyle
branch
from
7e90aef
to
563b33c
Compare
|
EWS run on current version of this PR (hash 563b33c)
|
🧪 mac-AS-debug-wk2
https://bugs.webkit.org/show_bug.cgi?id=254524 rdar://107267784 Reviewed by Antti Koivisto. StyleRuleCounterStyle currently stores `StyleProperties` besides `CSSCounterStyleDescriptors`. This is used for facilitating serialization and read/write for the CSSOM api. We can get rid of the `StyleProperties` member and use only `CSSCounterStyleDescriptors` after we write a serializer for each descriptor and adapt the setter function for setting only the `CSSCounterStyleDescriptors` without havint to set the property that originated the descriptor. * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-at-rule/prefix-suffix-syntax-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-at-rule/speak-as-syntax-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-at-rule/symbols-syntax-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-prefix-suffix-syntax-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-speak-as-syntax-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-symbols-syntax-expected.txt: - We and other browsers don't support url() as symbol for the counter-style at-rule, so these should be failing. They were suceeding, because we parsed it correctly, but since we don't support it, we shouldn't be able to use this value for setting the descriptor. - We don't support the speak-as descriptor, it should be failing. - Note that these tests are duplicated. We should re-import WPT and get rid of the dupes (rdar://107410996) * Source/WebCore/css/CSSCounterStyle.cpp: (WebCore::CSSCounterStyle::counterForSystemCyclic const): (WebCore::CSSCounterStyle::counterForSystemFixed const): (WebCore::CSSCounterStyle::counterForSystemSymbolic const): (WebCore::CSSCounterStyle::counterForSystemAlphabetic const): (WebCore::CSSCounterStyle::counterForSystemNumeric const): (WebCore::CSSCounterStyle::counterForSystemAdditive const): (WebCore::CSSCounterStyle::applyNegativeSymbols const): (WebCore::CSSCounterStyle::applyPadSymbols const): (WebCore::CSSCounterStyle::extendAndResolve): - Since now Symbols is a struct, to support correct serialization of custom-ident and string values, we should access the text value with the .text member. * Source/WebCore/css/CSSCounterStyle.h: (WebCore::CSSCounterStyle::isExtendsUnresolved): - Moving this flag to the descriptor so we can use it for serialization of the system. * Source/WebCore/css/CSSCounterStyleDescriptors.cpp: - We are renaming the translating functions, removing the 'translate' prefix to become in better compliance with WebKit style. - The translation functions are now exposed via the header because when a descriptor is set via CSSOM, we need to translate its CSSValue to the descriptor value. - Each descriptor has now a setter method, which will also take care of its validation. Before, the validation would happen through setterInternal() and newValueInvalidOrEqual() methods: (WebCore::rangeFromStyleProperties): (WebCore::rangeFromCSSValue): (WebCore::symbolFromCSSValue): (WebCore::nameFromCSSValue): (WebCore::additiveSymbolsFromStyleProperties): (WebCore::additiveSymbolsFromCSSValue): (WebCore::padFromStyleProperties): (WebCore::padFromCSSValue): (WebCore::negativeSymbolsFromStyleProperties): (WebCore::negativeSymbolsFromCSSValue): (WebCore::symbolsFromStyleProperties): (WebCore::symbolsFromCSSValue): (WebCore::fallbackNameFromStyleProperties): (WebCore::fallbackNameFromCSSValue): (WebCore::prefixFromStyleProperties): (WebCore::suffixFromStyleProperties): (WebCore::extractSystemDataFromStyleProperties): (WebCore::extractSystemDataFromCSSValue): (WebCore::CSSCounterStyleDescriptors::setExplicitlySetDescriptors): (WebCore::CSSCounterStyleDescriptors::create): (WebCore::CSSCounterStyleDescriptors::areSymbolsValidForSystem): (WebCore::CSSCounterStyleDescriptors::isValid const): (WebCore::CSSCounterStyleDescriptors::setName): (WebCore::CSSCounterStyleDescriptors::setSystemData): (WebCore::CSSCounterStyleDescriptors::setNegative): (WebCore::CSSCounterStyleDescriptors::setPrefix): (WebCore::CSSCounterStyleDescriptors::setSuffix): (WebCore::CSSCounterStyleDescriptors::setRanges): (WebCore::CSSCounterStyleDescriptors::setPad): (WebCore::CSSCounterStyleDescriptors::setFallbackName): (WebCore::CSSCounterStyleDescriptors::setSymbols): (WebCore::CSSCounterStyleDescriptors::setAdditiveSymbols): (WebCore::CSSCounterStyleDescriptors::Symbol::cssText const): (WebCore::CSSCounterStyleDescriptors::nameCSSText const): (WebCore::CSSCounterStyleDescriptors::systemCSSText const): (WebCore::CSSCounterStyleDescriptors::negativeCSSText const): (WebCore::CSSCounterStyleDescriptors::prefixCSSText const): (WebCore::CSSCounterStyleDescriptors::suffixCSSText const): (WebCore::CSSCounterStyleDescriptors::rangesCSSText const): (WebCore::CSSCounterStyleDescriptors::Pad::cssText const): (WebCore::CSSCounterStyleDescriptors::padCSSText const): (WebCore::CSSCounterStyleDescriptors::fallbackCSSText const): (WebCore::CSSCounterStyleDescriptors::symbolsCSSText const): (WebCore::CSSCounterStyleDescriptors::additiveSymbolsCSSText const): (WebCore::translateRangeFromStyleProperties): Deleted. (WebCore::symbolToString): Deleted. (WebCore::translateAdditiveSymbolsFromStyleProperties): Deleted. (WebCore::translatePadFromStyleProperties): Deleted. (WebCore::translateNegativeSymbolsFromStyleProperties): Deleted. (WebCore::translateSymbolsFromStyleProperties): Deleted. (WebCore::translateFallbackNameFromStyleProperties): Deleted. (WebCore::translatePrefixFromStyleProperties): Deleted. (WebCore::translateSuffixFromStyleProperties): Deleted. (WebCore::extractDataFromSystemDescriptor): Deleted. (WebCore::CSSCounterStyleDescriptors::areSymbolsValidForSystem const): Deleted. * Source/WebCore/css/CSSCounterStyleDescriptors.h: (WebCore::CSSCounterStyleDescriptors::Symbol::operator== const): (WebCore::CSSCounterStyleDescriptors::Symbol::operator!= const): (WebCore::CSSCounterStyleDescriptors::setName): Deleted. * Source/WebCore/css/CSSCounterStyleRule.cpp: (WebCore::StyleRuleCounterStyle::StyleRuleCounterStyle): (WebCore::StyleRuleCounterStyle::create): - We no longer need to store the StyleProperties used for creating the CSSCounterStyleDescriptors inside StyleRuleCounterStyle. Now we store only CSSCounterStyleDescriptors. (WebCore::CSSCounterStyleRule::CSSValueFromText): (WebCore::CSSCounterStyleRule::setName): (WebCore::CSSCounterStyleRule::setSystem): (WebCore::CSSCounterStyleRule::setNegative): (WebCore::CSSCounterStyleRule::setPrefix): (WebCore::CSSCounterStyleRule::setSuffix): (WebCore::CSSCounterStyleRule::setRange): (WebCore::CSSCounterStyleRule::setPad): (WebCore::CSSCounterStyleRule::setFallback): (WebCore::CSSCounterStyleRule::setSymbols): (WebCore::CSSCounterStyleRule::setAdditiveSymbols): (WebCore::CSSCounterStyleRule::setSpeakAs): (WebCore::symbolsValidForSystem): Deleted. (WebCore::StyleRuleCounterStyle::newValueInvalidOrEqual const): Deleted. (WebCore::StyleRuleCounterStyle::mutableProperties): Deleted. (WebCore::CSSCounterStyleRule::setterInternal): Deleted. - The CSSCounterStyleRule setters now use the setters of CSSCounterStyleDescriptors to set each descriptor directly, without having validating it with setterInternal() and newValueInvalidOrEqual(), since each setter validates itself. * Source/WebCore/css/CSSCounterStyleRule.h: * Source/WebCore/css/parser/CSSParserImpl.cpp: (WebCore::CSSParserImpl::consumeCounterStyleRule): * Source/WebCore/rendering/RenderListMarker.cpp: (WebCore::RenderListMarker::updateContent): - Updating the access of symbol's via .text member, since it is now a struct for supporting serialization (see first comments). Canonical link: https://commits.webkit.org/262396@main
webkit-commit-queue
force-pushed
the
refactor-StyleRuleCounterStyle
branch
from
563b33c
to
b96c4a0
Compare
|
Committed 262396@main (b96c4a0): https://commits.webkit.org/262396@main Reviewed commits have been landed. Closing PR #12168 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
b96c4a0
563b33c
🛠 wincairo🧪 wpe-wk2🧪 ios-wk2🧪 mac-wk1🧪 gtk-wk2🧪 mac-wk2