<rdar://problem/58961750>

Created attachment 389037 [details]
Patch

Comment on attachment 389037 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:600
> +            if (keyframeEffect->isCurrentlyAffectingProperty(property, true))

That 'true' argument is a boolean trap. I've no idea what it means here.

> Source/WebCore/animation/KeyframeEffect.cpp:804
> +    computeAcceleratedPropertiesMembership();

"membership" is weird.

> Source/WebCore/animation/KeyframeEffect.cpp:1132
> +    if (!hasSomeAcceleratedProperties && !hasSomeUnacceleratedProperties)
> +        m_acceleratedPropertiesMembership = AcceleratedProperties::None;
> +    m_acceleratedPropertiesMembership = hasSomeUnacceleratedProperties ? AcceleratedProperties::Some : AcceleratedProperties::All;

Awkward. Better written with an else. Or just a small 2x2 lookup table.

> Source/WebCore/animation/KeyframeEffect.h:188
> +    AcceleratedProperties m_acceleratedPropertiesMembership { AcceleratedProperties::None };

"Membership" is weird. Maybe m_acceleratedPropertiesState.

> Source/WebCore/animation/KeyframeEffect.h:190
> +    bool m_isRunningAccelerated { false };

What does this mean? Does it mean some or all properties are being accelerated? Maybe rename for clarity.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2788
> +        if (auto* effectsStack = element->keyframeEffectStack()) {
> +            return (effectsStack->isCurrentlyAffectingProperty(CSSPropertyOpacity)
> +                && (usesCompositing() || (m_compositingTriggers & ChromeClient::AnimatedOpacityTrigger)))
> +                || effectsStack->isCurrentlyAffectingProperty(CSSPropertyFilter)
> +#if ENABLE(FILTERS_LEVEL_2)
> +                || effectsStack->isCurrentlyAffectingProperty(CSSPropertyWebkitBackdropFilter)
> +#endif
> +                || effectsStack->isCurrentlyAffectingProperty(CSSPropertyTransform);

Can this be hidden behind a function on the Timeline? Also, can you avoid iterating all the effects for each property?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3349
>      return renderer.animation().isRunningAnimationOnRenderer(renderer, CSSPropertyTransform);

Does renderer.animation() do anything for Web Animations? Should it be removed, or made to work with Web Animations?

> LayoutTests/webanimations/partly-accelerated-transition-by-removing-property-expected.txt:10
> +          (position 108.00 13.00)

It's better to put the target before other elements, or give it position:absolute so that these numbers are not sensitive to font size, which can differ between platforms.

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 389037 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389037&action=review
> 
> > Source/WebCore/animation/DocumentTimeline.cpp:600
> > +            if (keyframeEffect->isCurrentlyAffectingProperty(property, true))
> 
> That 'true' argument is a boolean trap. I've no idea what it means here.

Replacing it with an `accelerated` argument and `enum class Accelerated : uint8_t { Yes, No }`.

> > Source/WebCore/animation/KeyframeEffect.cpp:804
> > +    computeAcceleratedPropertiesMembership();
> 
> "membership" is weird.

I'm replacing it with "state" as suggested below.

> > Source/WebCore/animation/KeyframeEffect.cpp:1132
> > +    if (!hasSomeAcceleratedProperties && !hasSomeUnacceleratedProperties)
> > +        m_acceleratedPropertiesMembership = AcceleratedProperties::None;
> > +    m_acceleratedPropertiesMembership = hasSomeUnacceleratedProperties ? AcceleratedProperties::Some : AcceleratedProperties::All;
> 
> Awkward. Better written with an else. Or just a small 2x2 lookup table.

Rewriting as if / else if / else to cover the three possible values, it is clearer.

> > Source/WebCore/animation/KeyframeEffect.h:188
> > +    AcceleratedProperties m_acceleratedPropertiesMembership { AcceleratedProperties::None };
> 
> "Membership" is weird. Maybe m_acceleratedPropertiesState.

Yes, I'm making that change.

> > Source/WebCore/animation/KeyframeEffect.h:190
> > +    bool m_isRunningAccelerated { false };
> 
> What does this mean? Does it mean some or all properties are being
> accelerated? Maybe rename for clarity.

It actually doesn't matter in the code. I'm not sure how best to rename it that would anything clearer.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2788
> > +        if (auto* effectsStack = element->keyframeEffectStack()) {
> > +            return (effectsStack->isCurrentlyAffectingProperty(CSSPropertyOpacity)
> > +                && (usesCompositing() || (m_compositingTriggers & ChromeClient::AnimatedOpacityTrigger)))
> > +                || effectsStack->isCurrentlyAffectingProperty(CSSPropertyFilter)
> > +#if ENABLE(FILTERS_LEVEL_2)
> > +                || effectsStack->isCurrentlyAffectingProperty(CSSPropertyWebkitBackdropFilter)
> > +#endif
> > +                || effectsStack->isCurrentlyAffectingProperty(CSSPropertyTransform);
> 
> Can this be hidden behind a function on the Timeline? Also, can you avoid
> iterating all the effects for each property?

So, the timeline isn't actually the right place to host this, and timelines encapsulate more information than it should and I've been progressively moving things onto KeyframeEffectStack. Indeed, an effect stack could combine several effects belonging to animations registered with different timelines.

I think this kind of logic belongs in the rendering code more than in the animation code. I'd rather keep it here.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:3349
> >      return renderer.animation().isRunningAnimationOnRenderer(renderer, CSSPropertyTransform);
> 
> Does renderer.animation() do anything for Web Animations? Should it be
> removed, or made to work with Web Animations?

There is a renderer.documentTimeline(). In this case though, we should return `false` if the runtime flag to have CSS Animations as Web Animations is false before running this code. Making that change.

> > LayoutTests/webanimations/partly-accelerated-transition-by-removing-property-expected.txt:10
> > +          (position 108.00 13.00)
> 
> It's better to put the target before other elements, or give it
> position:absolute so that these numbers are not sensitive to font size,
> which can differ between platforms.

Good point, will do.

So a few tests are failing in WK1 alone, for instance LayoutTests/webanimations/opacity-animation-yields-compositing.html. They are failing because in RenderLayerCompositor::requiresCompositingForAnimation(), usesCompositing() returns false while in WK2 it returns true. Trying to understand why.

Hmm, so in RenderLayerCompositor::cacheAcceleratedCompositingFlags(), m_forceCompositingMode is set to false because m_renderView.settings().forceCompositingMode() is false, but it's true in WK2, or at least in WKTR vs. DRT. Simon, do you know what's happening?

OK, as it turns out, in WK1 we don't use compositing when opacity is the only potentially accelerated property to be animated. That explains all the WK1 failures but one, for which I have a fix.

Created attachment 389159 [details]
Patch

Committed r255383: <https://trac.webkit.org/changeset/255383>