Bug 191042 – CSS Custom Properties API Should Support syntax="*" and "<length>", and handle cycles properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Justin Michaud]

<https://bugs.webkit.org/show_bug.cgi?id=190039> was rolled out due to a performance regression in speedometer. Let's try this again.

[Justin Michaud]

<rdar://45477680>

[Justin Michaud]

Roll out: <rdar://problem/45546003>

[Justin Michaud]

Created attachment 353344 [details]
Patch

[Justin Michaud]

<rdar://45477680>

[Justin Michaud]

Created attachment 353382 [details]
Changes from last patch

[Antti Koivisto]

Justin said he is trying some template specialization for no-custom-properties case to eliminate the remaining regression.

[Justin Michaud]

Created attachment 353394 [details]
Patch

[Justin Michaud]

Created attachment 353395 [details]
Changes from unrolled patch

[Dean Jackson]

Comment on attachment 353395 [details]
Changes from unrolled patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353395&action=review

> Source/WebCore/css/StyleResolver.cpp:2356
> +                // We are in a cycle (eg. setting font size using registered custom property value containing em)
> +                // So this value should be unset

Nit: .

[Antti Koivisto]

Comment on attachment 353394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353394&action=review

> Source/WebCore/css/StyleResolver.cpp:2336
> +        return applyCascadedPropertiesImpl<false>(firstProperty, lastProperty, state);
> +    return applyCascadedPropertiesImpl<true>(firstProperty, lastProperty, state);

It would be nicer to use enum instead of bool:

applyCascadedPropertiesImpl<CustomPropertyCycleTracking::Enabled>(..)

or similar.

[Justin Michaud]

Created attachment 353513 [details]
Patch

[Justin Michaud]

Created attachment 353514 [details]
Changes from last patch

[Antti Koivisto]

Comment on attachment 353513 [details]
Patch

R=me

[Justin Michaud]

Created attachment 353574 [details]
Patch

[Justin Michaud]

Created attachment 353575 [details]
Changes from rolled out patch

[Justin Michaud]

Created attachment 353576 [details]
Patch

[Justin Michaud]

Created attachment 353577 [details]
Changes from rolled-out patch

[Justin Michaud]

This patch seems to have fixed all of the performance regressions. Once the perf trial finishes, it looks like it should be good to land.

[WebKit Commit Bot]

Comment on attachment 353576 [details]
Patch

Clearing flags on attachment: 353576

Committed r237697: <https://trac.webkit.org/changeset/237697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 353576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353576&action=review

A few minor additional comments.

> Source/WebCore/css/StyleResolver.cpp:2354
> +        if (TrackCycles == CustomPropertyCycleTracking::Disabled) {
> +            // If we don't have any custom properties, then there can't be any cycles.
> +            property.apply(*this, state);
> +        } else {
> +            if (UNLIKELY(state.inProgressProperties.get(propertyID))) {

We usually try to main flow of the function the common case and use early return/continue for uncommon cases. That would suggest structure like

if (TrackCycles == CustomPropertyCycleTracking::Enabled) {
    ...
    continue;
}
property.apply(*this, state);

> Source/WebCore/css/StyleResolver.h:328
> +    enum CustomPropertyCycleTracking { Enabled = 0, Disabled };

This should be a an enum class so we don't put Enabled and Disabled to StyleResolver namespace.

No need for = 0

[Antti Koivisto]

*try to make