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
<rdar://problem/90799930>
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.
Actually, I can do this if in bug 238356 I turn -webkit-text-orientation into a shorthand.
Created attachment 455761 [details] Patch
Created attachment 455762 [details] Patch for EWS
PTAL
Seems like this patch needs rebasing, even with the dependent patch landed.
Yes, it needs to be rebased because I also landed bug 237487
Created attachment 456826 [details] Patch
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?
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.
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.
Created attachment 457099 [details] Patch
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.
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].