Bug 238350 – [css-cascade] Merge getRelatedPropertyId() and shouldApplyPropertyInParseOrder()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

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

[Radar WebKit Bug Importer]

<rdar://problem/90799930>

[Oriol Brufau]

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]

Actually, I can do this if in bug 238356 I turn -webkit-text-orientation into a shorthand.

[Oriol Brufau]

Created attachment 455761 [details]
Patch

[Oriol Brufau]

Created attachment 455762 [details]
Patch for EWS

[Oriol Brufau]

PTAL

[Tim Nguyen (:ntim)]

Seems like this patch needs rebasing, even with the dependent patch landed.

[Oriol Brufau]

Yes, it needs to be rebased because I also landed bug 237487

[Oriol Brufau]

Created attachment 456826 [details]
Patch

[Darin Adler]

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 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 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]

Created attachment 457099 [details]
Patch

[Oriol Brufau]

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]

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].