Bug 238350

Summary: [css-cascade] Merge getRelatedPropertyId() and shouldApplyPropertyInParseOrder()
Product: WebKit Reporter: Oriol Brufau <obrufau>
Component: CSSAssignee: Oriol Brufau <obrufau>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, joepeck, koivisto, macpherson, menard, ntim, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 238356    
Bug Blocks: 238874, 238125, 238345    
Attachments:
Description Flags
testcase
none
Patch
none
Patch for EWS
none
Patch
none
Patch none

Oriol Brufau
Reported 2022-03-24 14:57:07 PDT
Created attachment 455692 [details] testcase getRelatedPropertyId() is automatically generated from the "related-property" flag in CSSProperties.json shouldApplyPropertyInParseOrder() is hardcoded in PropertyCascade.cpp Both pursue the same thing: if there are two properties (typically a standard one and a -webkit- prefixed one) which share the same field in RenderStyle, then we should take order into account when deciding which one to apply. For example, this works due to getRelatedPropertyId(): <div style="-webkit-text-orientation: mixed; text-orientation: upright">I'm upright</div> <div style="text-orientation: mixed; -webkit-text-orientation: upright">I'm upright</div> And this works due to shouldApplyPropertyInParseOrder(): <div style="-webkit-box-shadow: 0 0 10px red; box-shadow: 0 0 10px green">I have green shadow</div> <div style="box-shadow: 0 0 10px red; -webkit-box-shadow: 0 0 10px green">I have green shadow</div> They are implemented in different ways, though. And getRelatedPropertyId() fails here: <style> div { -webkit-text-orientation: mixed } div { text-orientation: upright } </style> <div>upright</div> (see attachments for the full testcase) shouldApplyPropertyInParseOrder() doesn't relate pairs of properties, but this will be needed for bug 238125. Also, shouldApplyPropertyInParseOrder() needs to be automatically generated for bug 238345. So I say: - Get rid of all consumers of getRelatedPropertyId(). - Mark shouldApplyPropertyInParseOrder() properties with the "related-property" flag. - Change shouldApplyPropertyInParseOrder() to just return getRelatedPropertyId(id) != CSSPropertyID::CSSPropertyInvalid
Attachments
testcase (889 bytes, text/html)
2022-03-24 14:57 PDT, Oriol Brufau 		no flags
Details
Patch (11.86 KB, patch)
2022-03-25 07:50 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch for EWS (34.50 KB, patch)
2022-03-25 07:52 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (10.10 KB, patch)
2022-04-06 09:28 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (10.22 KB, patch)
2022-04-08 11:15 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-03-24 16:09:56 PDT
<rdar://problem/90799930>
Oriol Brufau
Comment 2 2022-03-24 16:30:25 PDT
I realized that this won't work because text-orientation needs to be high priority, but deferred properties are applied at the end after low priority properties. So it's a pity but I guess I will keep text-orientation as-is and add a similar flag for deferred properties in bug 238345.
Oriol Brufau
Comment 3 2022-03-24 17:56:38 PDT
Actually, I can do this if in bug 238356 I turn -webkit-text-orientation into a shorthand.
Oriol Brufau
Comment 4 2022-03-25 07:50:28 PDT
Created attachment 455761 [details] Patch
Oriol Brufau
Comment 5 2022-03-25 07:52:50 PDT
Created attachment 455762 [details] Patch for EWS
Oriol Brufau
Comment 6 2022-03-25 07:58:20 PDT
PTAL
Tim Nguyen (:ntim)
Comment 7 2022-04-06 09:06:10 PDT
Seems like this patch needs rebasing, even with the dependent patch landed.
Oriol Brufau
Comment 8 2022-04-06 09:07:55 PDT
Yes, it needs to be rebased because I also landed bug 237487
Oriol Brufau
Comment 9 2022-04-06 09:28:19 PDT
Created attachment 456826 [details] Patch
Darin Adler
Comment 10 2022-04-08 10:05:18 PDT
Comment on attachment 456826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456826&action=review > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:305 > if (!value.isEmpty()) > return value; How important is this transformation of an empty string into a null string? I’d like to look at call sites of this function to see how they rely on that. > Source/WebCore/css/makeprop.pl:608 > + die "Property $name has an unknown related property: $related" if !exists($nameToId{$related}); > + die "High priority property $name can't have a related property: $related" if $nameIsHighPriority{$name}; > + die "Shorthand property $name can't have a related property: $related" if exists $propertiesWithStyleBuilderOptions{$name}{"longhands"}; > + die "Property $name can't have itself as a related property" if $related eq $name; > + die "Property $name has $related as a related property, but it's not reciprocal" if $relatedProperty{$related} ne $name; Do we want "\n" in these so we don’t get a perl source file and line number on these error messages? > Source/WebCore/css/parser/CSSParserImpl.cpp:130 > + seenProperties.set(propertyIDIndex); Now that we always do the test-and-set as a single operation, is there an optimization opportunity here? Typically it can be more efficient to test and set in a single atomic operation. > Source/WebCore/style/PropertyCascade.cpp:43 > + return getRelatedPropertyId(propertyID) != CSSPropertyInvalid; This looks like more work. Does this have a measurable performance effect?
Oriol Brufau
Comment 11 2022-04-08 10:38:48 PDT
Comment on attachment 456826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456826&action=review >> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:305 >> return value; > > How important is this transformation of an empty string into a null string? I’d like to look at call sites of this function to see how they rely on that. The returned values are used for IDL getters, i.e. they are webexposed. I don't know if it's important, but this condition was already there before related values were added in https://trac.webkit.org/changeset/259006/webkit#file28 So I would keep it just in case. >> Source/WebCore/css/parser/CSSParserImpl.cpp:130 >> + seenProperties.set(propertyIDIndex); > > Now that we always do the test-and-set as a single operation, is there an optimization opportunity here? Typically it can be more efficient to test and set in a single atomic operation. Again, I'm just reverting this to before https://trac.webkit.org/changeset/259006/webkit#file31 seenProperties is a std::bitset, I don't see a way to simmultaneously test and set. >> Source/WebCore/style/PropertyCascade.cpp:43 >> + return getRelatedPropertyId(propertyID) != CSSPropertyInvalid; > > This looks like more work. Does this have a measurable performance effect? In bug 238345 I will remove shouldApplyPropertyInParseOrder() and just use `id >= firstDeferredProperty`. That should be fast.
Darin Adler
Comment 12 2022-04-08 10:46:45 PDT
Comment on attachment 456826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456826&action=review >>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:305 >>> return value; >> >> How important is this transformation of an empty string into a null string? I’d like to look at call sites of this function to see how they rely on that. > > The returned values are used for IDL getters, i.e. they are webexposed. > I don't know if it's important, but this condition was already there before related values were added in https://trac.webkit.org/changeset/259006/webkit#file28 > So I would keep it just in case. If it’s web-exposed, then it’s very important: JavaScript null. But I wonder in what cases getPropertyValue returns an empty string. Given the name "internal" I wasn’t so clear this was web-exposed. >>> Source/WebCore/css/parser/CSSParserImpl.cpp:130 >>> + seenProperties.set(propertyIDIndex); >> >> Now that we always do the test-and-set as a single operation, is there an optimization opportunity here? Typically it can be more efficient to test and set in a single atomic operation. > > Again, I'm just reverting this to before https://trac.webkit.org/changeset/259006/webkit#file31 > seenProperties is a std::bitset, I don't see a way to simmultaneously test and set. Makes sense. I think we might get a tiny performance boost from writing this: auto seenPropertyBit = seenProperties[propertyIDIndex]; if (seenPropertyBit) continue; seenPropertyBit = true; The result is a std::bitset::reference. But maybe not worth trying it.
Oriol Brufau
Comment 13 2022-04-08 11:15:02 PDT
Created attachment 457099 [details] Patch
Oriol Brufau
Comment 14 2022-04-08 11:18:41 PDT
Comment on attachment 456826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456826&action=review >>>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:305 >>>> return value; >>> >>> How important is this transformation of an empty string into a null string? I’d like to look at call sites of this function to see how they rely on that. >> >> The returned values are used for IDL getters, i.e. they are webexposed. >> I don't know if it's important, but this condition was already there before related values were added in https://trac.webkit.org/changeset/259006/webkit#file28 >> So I would keep it just in case. > > If it’s web-exposed, then it’s very important: JavaScript null. But I wonder in what cases getPropertyValue returns an empty string. > > Given the name "internal" I wasn’t so clear this was web-exposed. The webexposed methods are actually CSSStyleDeclaration::propertyValueForCamelCasedIDLAttribute CSSStyleDeclaration::propertyValueForWebKitCasedIDLAttribute CSSStyleDeclaration::propertyValueForDashedIDLAttribute CSSStyleDeclaration::propertyValueForEpubCasedIDLAttribute CSSStyleDeclaration::cssFloat But they use `return getPropertyValueInternal(propertyID)` or `getPropertyValueInternal(CSSPropertyFloat)` Here is the spec: https://andreubotella.com/csswg-auto-build/cssom-1/#dom-cssstyledeclaration-cssfloat >> Source/WebCore/css/makeprop.pl:608 >> + die "Property $name has $related as a related property, but it's not reciprocal" if $relatedProperty{$related} ne $name; > > Do we want "\n" in these so we don’t get a perl source file and line number on these error messages? Done. >>>> Source/WebCore/css/parser/CSSParserImpl.cpp:130 >>>> + seenProperties.set(propertyIDIndex); >>> >>> Now that we always do the test-and-set as a single operation, is there an optimization opportunity here? Typically it can be more efficient to test and set in a single atomic operation. >> >> Again, I'm just reverting this to before https://trac.webkit.org/changeset/259006/webkit#file31 >> seenProperties is a std::bitset, I don't see a way to simmultaneously test and set. > > Makes sense. I think we might get a tiny performance boost from writing this: > > auto seenPropertyBit = seenProperties[propertyIDIndex]; > if (seenPropertyBit) > continue; > seenPropertyBit = true; > > The result is a std::bitset::reference. > > But maybe not worth trying it. Done.
EWS
Comment 15 2022-04-08 14:41:10 PDT
Committed r292636 (249456@main): <https://commits.webkit.org/249456@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457099 [details].
Note You need to log in before you can comment on or make changes to this bug.