Style and performance tweaks to CSS property name and keyword value tables#5986
Merged
webkit-early-warning-system merged 1 commit intoWebKit:mainfrom Nov 2, 2022
Conversation
darinadler
requested review from
cdumez,
dcrousso,
graouts,
patrickangle and
rniwa
as code owners
Collaborator
|
EWS run on previous version of this PR (hash ea13951) Details
|
webkit-ews-buildbot
added
the
merging-blocked
darinadler
removed
the
merging-blocked
darinadler
force-pushed
the
eng/Style-and-performance-tweaks-to-CSS-property-name-and-keyword-value-tables
branch
from
ea13951 to
c56e455
Compare
Collaborator
|
EWS run on previous version of this PR (hash c56e455) Details
|
webkit-ews-buildbot
added
the
merging-blocked
darinadler
removed
the
merging-blocked
darinadler
force-pushed
the
eng/Style-and-performance-tweaks-to-CSS-property-name-and-keyword-value-tables
branch
from
c56e455 to
7ea5910
Compare
Collaborator
|
EWS run on previous version of this PR (hash 7ea5910) Details
|
darinadler
force-pushed
the
eng/Style-and-performance-tweaks-to-CSS-property-name-and-keyword-value-tables
branch
2 times, most recently
from
86b824c to
07b0a87
Compare
Collaborator
|
EWS run on previous version of this PR (hash 86b824c) Details
|
Collaborator
|
EWS run on current version of this PR (hash 07b0a87) Details |
weinig
approved these changes
Nov 1, 2022
Contributor
There was a problem hiding this comment.
Not necessarily something to actually change, but what is your feeling on this:
auto list = allLonghandCSSProperties().compactMap([](auto propertyID) -> std::optional<CSSProperty> {
if (auto value = propertyValue(propertyID))
return CSSProperty(propertyID, WTFMove(value)));
return std::nullopt;
});(though, I think it would probably be auto list = WTF::compactMap(allLonghandCSSProperties(), ... since Vector doesn't have a compactMap of its own).
Member
Author
There was a problem hiding this comment.
I love that; I am tempted to follow up with it.
Contributor
There was a problem hiding this comment.
StyleProperties would be another good one to add an iterator for at some point.
Source/WebCore/css/makeprop.pl
Outdated
Contributor
There was a problem hiding this comment.
Any idea why only some of these quote #includes are escaped?
Member
Author
There was a problem hiding this comment.
None whatsoever. I didn’t even notice!
Source/WebCore/css/makeprop.pl
Outdated
Contributor
There was a problem hiding this comment.
I bet you can remove this now that we don't have $runtimeFlags.
Source/WebCore/css/makeprop.pl
Outdated
Contributor
There was a problem hiding this comment.
Might be a good idea to add a size() function that returns last - first so that the sized container algorithms can be a bit more efficient.
Member
Author
There was a problem hiding this comment.
I like that idea. I just learned about the std::ranges concept sized_range.
But is there somewhere I would use that in this patch?
Member
Author
There was a problem hiding this comment.
It’s last - first + 1, because last + 1 == end.
Contributor
There was a problem hiding this comment.
I had a sinking suspicion I would have an off by one there :).
Source/WebCore/css/makevalues.pl
Outdated
Contributor
There was a problem hiding this comment.
Same thought about a size() function, but this one could just return numCSSValueKeywords
…ables https://bugs.webkit.org/show_bug.cgi?id=247244 rdar://101730654 Reviewed by Sam Weinig. * Source/WebCore/animation/CSSPropertyAnimation.cpp: (WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap): Use nameLiteral, numCSSPRoperties instead of lastCSSProperty, and allCSSProperties. * Source/WebCore/animation/CSSTransition.cpp: (WebCore::CSSTransition::createEvent): Use nameString. * Source/WebCore/animation/CSSTransition.h: Use const AtomString& for the return value of transitionProperty to cut down on reference count churn. Use nameString, moved isCSSTransition to private. * Source/WebCore/animation/KeyframeEffect.cpp: (WebCore::KeyframeEffect::CSSPropertyIDToIDLAttributeName): Use nameForIDL. (WebCore::processKeyframeLikeObject): Use isExposed. * Source/WebCore/css/CSSBackgroundRepeatValue.cpp: (WebCore::CSSBackgroundRepeatValue::customCSSText const): Use nameString. * Source/WebCore/css/CSSComputedStyleDeclaration.cpp: (WebCore::CSSComputedStyleDeclaration::item const): Ditto. * Source/WebCore/css/CSSCustomPropertyValue.cpp: (WebCore::CSSCustomPropertyValue::customCSSText const): Ditto. * Source/WebCore/css/CSSFunctionValue.cpp: (WebCore::CSSFunctionValue::customCSSText const): Use nameLiteral. * Source/WebCore/css/CSSGridAutoRepeatValue.cpp: (WebCore::CSSGridAutoRepeatValue::customCSSText const): Ditto. * Source/WebCore/css/CSSPrimitiveValue.cpp: (WebCore::propertyName): Deleted. (WebCore::valueName): Deleted. (WebCore::CSSPrimitiveValue::stringValue const): Use nameString. (WebCore::CSSPrimitiveValue::formatNumberForCustomCSSText const): Ditto. (WebCore::CSSPrimitiveValue::equals const): Compare property and value IDs rather than converting to strings and comparing the strings. * Source/WebCore/css/CSSStyleDeclaration.cpp: (WebCore::parseJavaScriptCSSPropertyName): Use AtomString for the map keys for better performance. Don't add a trailing NUL character, since it's not required any more. Use findCSSProperty. (WebCore::lookupCSSPropertyFromIDLAttribute): Don't add a trailing NUL character since it's not required any more. Use findCSSProperty. (WebCore::CSSStyleDeclaration::getCSSPropertyIDFromJavaScriptPropertyName): Merged propertyIDFromJavaScriptCSSPropertyName in here, we don't need two separate functions. Use isExposed. * Source/WebCore/css/CSSValuePool.cpp: (WebCore::StaticCSSValuePool::StaticCSSValuePool): Use allCSSValueKeywords. (WebCore::CSSValuePool::createIdentifierValue): Do the assertion without using firstCSSValueKeyword and lastCSSValueKeyword. * Source/WebCore/css/ComputedStyleExtractor.cpp: (WebCore::createTransitionPropertyValue): Use nameString. (WebCore::ComputedStyleExtractor::propertyValue): Use isExposed. (WebCore::ComputedStyleExtractor::valueForPropertyInStyle): Ditto. (WebCore::ComputedStyleExtractor::copyProperties): Use allLonghandCSSProperties. * Source/WebCore/css/DOMCSSNamespace.cpp: (WebCore::DOMCSSNamespace::supports): Use isExposed. * Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp: (WebCore::PropertySetCSSStyleDeclaration::length const): Use isExposed. (WebCore::PropertySetCSSStyleDeclaration::item const): Ditto. (WebCore::PropertySetCSSStyleDeclaration::getPropertyCSSValue): Ditto. (WebCore::PropertySetCSSStyleDeclaration::getPropertyValue): Ditto. (WebCore::PropertySetCSSStyleDeclaration::getPropertyPriority): Ditto. (WebCore::PropertySetCSSStyleDeclaration::getPropertyShorthand): Ditto. (WebCore::PropertySetCSSStyleDeclaration::isPropertyImplicit): Ditto. (WebCore::PropertySetCSSStyleDeclaration::setProperty): Ditto. (WebCore::PropertySetCSSStyleDeclaration::removeProperty): Ditto. (WebCore::PropertySetCSSStyleDeclaration::getPropertyValueInternal): Ditto. (WebCore::PropertySetCSSStyleDeclaration::setPropertyInternal): Ditto. (WebCore::PropertySetCSSStyleDeclaration::isExposed const): Renamed from isCSSPropertyExposed and added the check for CSSPropertyInvalid here so we don't need to repeat it at all callers. * Source/WebCore/css/PropertySetCSSStyleDeclaration.h: Updated for above. * Source/WebCore/css/StyleColor.cpp: (WebCore::StyleColor::colorFromAbsoluteKeyword): Use nameLiteral. * Source/WebCore/css/StyleProperties.cpp: (WebCore::StyleProperties::fontValue const): Use nameString and nameLiteral. (WebCore::StyleProperties::get2Values const): Use nameString. (WebCore::StyleProperties::get4Values const): Use nameString. (WebCore::StyleProperties::getLayeredShorthandValue const): Use nameLiteral. (WebCore::StyleProperties::getPropertyShorthand const): Use nameString. (WebCore::StyleProperties::asTextInternal const): Use isLonghand and nameLiteral. (WebCore::MutableStyleProperties::setProperty): Use isExposed and isInternal. (WebCore::StyleProperties::PropertyReference::cssName const): Use nameString. * Source/WebCore/css/makeprop.pl: Removed support for runtime-flag. We don't need it any more. Moved the computedPropertyIDs array out of the header. Moved the hash table structure in here from HashTools.h. Set the %7bit flag since all the clients already forbid non-ASCII characters. Removed unneeded custom lookup-function-name, hash-function-name, and word-array-name since class-name already prevents collisions. Added isLonghand. Renamed isInternalCSSProperty to just isInternal since the argument is a strongly typed CSSPRopertyID. Similarly renamed isCSSPropertyExposed to isExposed. Replaced the findProperty function with one named findCSSProperty that returns a CSSPropertyID insted of a pointer into the hash table. Renamed getPropertyName to nameLiteral. Renamed getPropertyNameAtomString to nameString. Use a NeverDestroyed instead of allocating the array of AtomString on the heap. Got rid of getPropertyNameString. Renamed getJSPropertyName to nameForIDL. Made some tables constexpr. Renamed getRelatedPropertyID to relatedProperty. Made the property index constants use uint16_t and the string length constant use unsigned instead of size_t. Use constexpr for them. Changed computedPropertyIDs to be a std::array. Removed convertToCSSPropertyID. Added CSSPropertiesRange class template to help us implement range-based for statements. Added allCSSProperties and allLonghandCSSProperties. * Source/WebCore/css/makevalues.pl: Moved the hash table structure in here from HashTools.h. Set the %7bit flag since all the clients already forbid non-ASCII characters. Removed unneeded custom lookup-function-name, hash-function-name, and word-array-name since class-name already prevents collisions. Replaced the findValue function with one named findCSSValueKeyword that returns a CSSValueID instead of a pointer into the hash table. Renamed getValueName to nameLiteral. Renamed getValueNameAtomString to nameString. Got rid of getValueNameString. Made the index constants use uint16_t and the string length constant use unsigned instead of size_t. Added AllCSSValueKeywordsRange class template to help us implement range-based for statements. Added allCSSValueKeywords. * Source/WebCore/css/parser/CSSParserFastPaths.cpp: (WebCore::CSSParserFastPaths::maybeParseValue): Use isExposed. * Source/WebCore/css/parser/CSSParserImpl.cpp: (WebCore::CSSParserImpl::consumeDeclaration): Use isExposed. * Source/WebCore/css/parser/CSSPropertyParser.cpp: Removed include of HashTools.h. (WebCore::cssPropertyID): Removed unneeded extra characters in buffer. Removed code that adds a trailing NUL character. Use findCSSProperty. (WebCore::isAppleLegacyCSSValueKeyword): Removed unnecessarily complex handling of "-apple-wireless" prefix, simplifying making it match the other cases. Also tweaked the logic to do less excess checking. (WebCore::cssValueKeywordID): Removed code that adds a trailing NUL character. SImplified the isAppleLegacyCSSValueKeyword section a little and removed a misplaced comment. Use findCSSValueKeyword. (WebCore::CSSPropertyParser::addProperty): Use isExposed. (WebCore::consumeWillChange): Use isExposed. (WebCore::CSSPropertyParser::parseSingleValue): Use isExposed. (WebCore::CSSPropertyParser::parseCounterStyleDescriptor): Use isExposed. (WebCore::CSSPropertyParser::parseFontFaceDescriptor): Use isExposed. (WebCore::CSSPropertyParser::parseFontPaletteValuesDescriptor): Use isExposed. (WebCore::CSSPropertyParser::consumeContainIntrinsicSizeShorthand): Use isExposed. * Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp: (WebCore::ComputedStylePropertyMapReadOnly::get const): Use isExposed. (WebCore::ComputedStylePropertyMapReadOnly::getAll const): Use isExposed. (WebCore::ComputedStylePropertyMapReadOnly::entries const): Use nameString. * Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp: (WebCore::MainThreadStylePropertyMapReadOnly::get const): Use isExposed. (WebCore::MainThreadStylePropertyMapReadOnly::getAll const): Use isExposed. * Source/WebCore/css/typedom/StylePropertyMap.cpp: (WebCore::StylePropertyMap::remove): Use isExposed. * Source/WebCore/dom/Document.cpp: (WebCore::Document::exposedComputedCSSPropertyIDs): Tweaked code since computedPropertyIDs is now a std::array. Also decreased copying a little using std::optional::emplace. We could do even better if we made a FixedVector constructor that took a pair of iterators or a Span. Use isExposed. (WebCore::Document::detachFromFrame): Fixed comment typo. * Source/WebCore/html/CustomPaintImage.cpp: Use nameString. * Source/WebCore/inspector/InspectorStyleSheet.cpp: (WebCore::InspectorStyle::collectProperties const): Use allCSSProperties and isExposed. Fixes a bug where we would not iterate over the last CSS property. Use nameString. (WebCore::InspectorStyle::styleWithProperties const): Use isExposed and nameString. * Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp: (WebCore::buildObjectForKeyframes): Use nameString. * Source/WebCore/inspector/agents/InspectorCSSAgent.cpp: (WebCore::InspectorCSSAgent::getSupportedCSSProperties): Use allCSSProperties, isExposed, nameString, and allCSSValueKeywords. * Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp: (WebCore::CaptionUserPreferencesMediaAF::captionsWindowCSS const): Use nameLiteral. (WebCore::appendCSS): Turned this into a template to cut down on temporary strings. (WebCore::CaptionUserPreferencesMediaAF::windowRoundedCornerRadiusCSS const): Reorder arguments and stop using nameString when calling appendCSS. (WebCore::CaptionUserPreferencesMediaAF::colorPropertyCSS const): Ditto. (WebCore::CaptionUserPreferencesMediaAF::captionsTextEdgeCSS const): Ditto. Also got rid of NeverDestroyed<const String> because we are appending these strings, so we can just use literals. * Source/WebCore/platform/HashTools.h: Remove Property, Value, findProperty, and findValue. * Source/WebCore/platform/animation/Animation.cpp: (WebCore::operator<<): Use nameLiteral. * Source/WebCore/rendering/style/RenderStyleConstants.cpp: (WebCore::operator<<): Ditto. * Source/WebCore/style/PropertyCascade.cpp: (WebCore::Style::PropertyCascade::lastDeferredPropertyResolvingRelated const): Use relatedProperty. * Source/WebCore/style/StyleBuilderCustom.h: (WebCore::Style::BuilderCustom::applyValueWillChange): Use isExposed. Canonical link: https://commits.webkit.org/256223@main
webkit-early-warning-system
force-pushed
the
eng/Style-and-performance-tweaks-to-CSS-property-name-and-keyword-value-tables
branch
from
07b0a87 to
a10c81d
Compare
Collaborator
|
Committed 256223@main (a10c81d): https://commits.webkit.org/256223@main Reviewed commits have been landed. Closing PR #5986 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
darinadler
deleted the
eng/Style-and-performance-tweaks-to-CSS-property-name-and-keyword-value-tables
branch
webkit-early-warning-system
pushed a commit
to rcaliman-apple/WebKit
that referenced
this pull request
Dec 8, 2022
…es marked as unsupported https://bugs.webkit.org/show_bug.cgi?id=248314 Reviewed by Darin Adler. Querying `WebCore::cssPropertyID()` defined in `CSSPropertyParser.cpp` with a CSS custom property name (aka CSS variable) mistakenly returns `CSSPropertyID::CSSPropertyInvalid` instead of `CSSPropertyID::CSSPropertyCustom`. This was also the case before WebKit#5986 landed, so this wasn't regressed by it. What the refactoring in WebKit#5986 did was to introduce stricter checks in `PropertySetCSSStyleDeclaration::isExposed()` and in `WebCore::isExposed()` declared in the generated file `CSSPropertyNames.cpp` to return `false` when encountering `CSSPropertyID::CSSPropertyInvalid`. The specific `CSSPropertyID` wasn't of particular importance for Web Inspector so the wrong id didn't cause issues previously. But `InspectorStyle::styleWithProperties()` makes use of the `isExposed()` check to mark a CSS custom property as `property->setParsedOk(false)` which ultimately marks it as unsupported in the Web Inspector frontend. Now that the check is more strict, compounded with the effect of the mistaken `CSSPropertyID::CSSPropertyInvalid`, causes CSS custom properties to be incorrectly marked as unsupported. To mitigate this: - explicitly check if the property is a custom property and assign it the correct id of `CSSPropertyID::CSSPropertyCustom`. - since `WebCore::nameString()` can't return an arbitrary CSS custom property name even if it were provided the correct `CSSPropertyID`, we guard for this before calling the method. There are many callers of `WebCore::cssPropertyID()` and only some of them manually check for `WebCore::isCustomPropertyName()` to take special action. Fixing the root issue so that it correctly returns `CSSPropertyID::CSSPropertyCustom` will be done in another patch to account for any regressions it may introduce. * LayoutTests/inspector/css/css-property-expected.txt: * LayoutTests/inspector/css/css-property.html: Added a check that CSS custom properties are valid. `Protocol::CSS::CSSProperty::parsedOk` maps to `WI.CSSProperty.valid`. * Source/WebCore/inspector/InspectorStyleSheet.cpp: (WebCore::InspectorStyle::styleWithProperties const): Canonical link: https://commits.webkit.org/257564@main
rjepstein
pushed a commit
that referenced
this pull request
Dec 8, 2022
Web Inspector: REGRESSION(r256223@main) Styles Panel: All CSS variables marked as unsupported
https://bugs.webkit.org/show_bug.cgi?id=248314
Reviewed by Darin Adler.
Querying `WebCore::cssPropertyID()` defined in `CSSPropertyParser.cpp`
with a CSS custom property name (aka CSS variable) mistakenly returns
`CSSPropertyID::CSSPropertyInvalid` instead of `CSSPropertyID::CSSPropertyCustom`.
This was also the case before #5986 landed,
so this wasn't regressed by it.
What the refactoring in #5986 did was to
introduce stricter checks in `PropertySetCSSStyleDeclaration::isExposed()`
and in `WebCore::isExposed()` declared in the generated file `CSSPropertyNames.cpp`
to return `false` when encountering `CSSPropertyID::CSSPropertyInvalid`.
The specific `CSSPropertyID` wasn't of particular importance for Web Inspector so the
wrong id didn't cause issues previously.
But `InspectorStyle::styleWithProperties()` makes use of the `isExposed()` check to
mark a CSS custom property as `property->setParsedOk(false)` which ultimately marks it
as unsupported in the Web Inspector frontend.
Now that the check is more strict, compounded with the effect of the mistaken
`CSSPropertyID::CSSPropertyInvalid`, causes CSS custom properties to be incorrectly
marked as unsupported.
To mitigate this:
- explicitly check if the property is a custom property and assign it the correct id of
`CSSPropertyID::CSSPropertyCustom`.
- since `WebCore::nameString()` can't return an arbitrary CSS custom property name
even if it were provided the correct `CSSPropertyID`, we guard for this before calling the method.
There are many callers of `WebCore::cssPropertyID()` and only some of them
manually check for `WebCore::isCustomPropertyName()` to take special action.
Fixing the root issue so that it correctly returns `CSSPropertyID::CSSPropertyCustom`
will be done in another patch to account for any regressions it may introduce.
* LayoutTests/inspector/css/css-property-expected.txt:
* LayoutTests/inspector/css/css-property.html:
Added a check that CSS custom properties are valid.
`Protocol::CSS::CSSProperty::parsedOk` maps to `WI.CSSProperty.valid`.
* Source/WebCore/inspector/InspectorStyleSheet.cpp:
(WebCore::InspectorStyle::styleWithProperties const):
Canonical link: https://commits.webkit.org/257564@main
Canonical link: https://commits.webkit.org/257350.6@safari-7615.1.14-branch
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
a10c81d
07b0a87
🧪 api-mac🧪 mac-AS-debug-wk2