Bug 144198 – Eliminate styleDidChange with StyleDifferenceEqual when updates are actually necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Eliminate styleDidChange with StyleDifferenceEqual when updates are actually necessary

[Simon Fraser (smfr)]

Created attachment 251654 [details]
Patch

[Darin Adler]

Comment on attachment 251654 [details]
Patch

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

> Source/WebCore/page/animation/CompositeAnimation.cpp:332
>              CSSPropertyTransitionsMap::const_iterator end = m_transitions.end();
>              for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) {
> -                if (ImplicitAnimation* anim = it->value.get())
> -                    anim->animate(this, &renderer, currentStyle, &targetStyle, resultStyle);
> +                ImplicitAnimation* anim = it->value.get();
> +                if (anim && anim->animate(this, &renderer, currentStyle, &targetStyle, resultStyle))
> +                    animationStateChanged = true;
>              }

Would look really great with a modern for loop:

    for (auto* animation : m_transitions.values()) {
        if (animation && animation->animate(this, &renderer, currentStyle, &targetStyle, resultStyle))
            animationStateChanged = true;
    }

> Source/WebCore/page/animation/ImplicitAnimation.cpp:91
> +        // FIXME: still need this hack?

Could you write a slightly longer version of this FIXME that is sentence capitalized and also explains why you think we might not need it?

> Source/WebCore/rendering/RenderElement.h:48
> +    // hasSideEffects is true when out-of-band state (e.g. animations) requires that styleDidChange processing
> +    // continues even if the style didn't change.

The grammar here is a little strange. You say that hasSideEffects “is true”, but the comment is about this function, not about the callers to this function. Could you word it better? The comment should be information about what th function does or what the caller should do, I think. Or it could be about the caller if you say “Called with”, I guess. I probably wouldn’t have complained if it was worded that way.

> Source/WebCore/rendering/RenderElement.h:330
> +    bool animationStateChanged;
> +    setStyle(animation().updateAnimations(*this, WTF::move(style), animationStateChanged), hasSideEffects || animationStateChanged);

This is not right. There is no guarantee about ordering of the evaluation of arguments, so it might compute "hasSideEffects || animationStateChanged" before updateAnimations returns. You need to put that in a separate statement to makes sure the ordering is right.

> Source/WebCore/style/StyleResolveTree.cpp:205
> +    bool animationsChanged;
> +    newRenderer->setStyleInternal(newRenderer->animation().updateAnimations(*newRenderer, newRenderer->style(), animationsChanged));

Same problem with arguments here.

[Antti Koivisto]

Comment on attachment 251654 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        compositing or other work has to happen from styleStyle(). This is achieved by

styleStyle?

> Source/WebCore/page/animation/CompositeAnimation.h:58
> +    Ref<RenderStyle> animate(RenderElement&, RenderStyle* currentStyle, RenderStyle& targetStyle, bool& animationStateChanged);

Why does this return the style as return value and the state change boolean as argument...

> Source/WebCore/page/animation/ImplicitAnimation.cpp:65
> +bool ImplicitAnimation::animate(CompositeAnimation*, RenderElement*, const RenderStyle*, RenderStyle* targetStyle, RefPtr<RenderStyle>& animatedStyle)

...while this does the opposite?

> Source/WebCore/rendering/RenderElement.cpp:397
> +void RenderElement::setStyle(Ref<RenderStyle>&& style, bool hasSideEffects)

At minimum this should use an enum.

The name is bit vague. What do "side effects" mean here and why do they map to StyleDifferenceRecompositeLayer?

How about passing the base StyleDifference as parameter instead?

> Source/WebCore/style/StyleResolveTree.cpp:634
> +            renderer->setAnimatableStyle(*newStyle, current.styleChangeType() == SyntheticStyleChange);

Then this code would read something like

auto minimumStyleDifference = current.styleChangeType() == SyntheticStyleChange ? StyleDifferenceRecompositeLayer : StyleDifferenceEqual;
renderer->setAnimatableStyle(*newStyle, minimumStyleDifference);

I think something would be clearer and more flexible than going via another bool/enum.

[Simon Fraser (smfr)]

Created attachment 251746 [details]
For EWS

[Simon Fraser (smfr)]

Comment on attachment 251746 [details]
For EWS

Carrying review forward

[Simon Fraser (smfr)]

https://trac.webkit.org/r183454

[Simon Fraser (smfr)]

Followup fix in https://trac.webkit.org/r183461