Bug 211695 – [Web Animations] Refactor animation comparison by composite order in a single utility function

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] Refactor animation comparison by composite order in a single utility function

[Antoine Quint]

Created attachment 398972 [details]
Patch

[Darin Adler]

Comment on attachment 398972 [details]
Patch

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

One problem here is that functions named "compare" functions aren’t obviously C++ standard library "operator<" functions. There’s another tradition of compare based on the C standard library that uses int and -1/0/1. I think the names probably need something to make it clear that it's the "<" style. I worry they will make it a bit inelegant.

I mention some style issues, but also some actual mistakes that I think you should consider fixing.

> Source/WebCore/animation/WebAnimationUtilities.cpp:41
> +inline bool compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder(Element* lhsOwningElement, Element* rhsOwningElement)

I think this code is easier to read with "a" and "b". Our rule against abbreviations doesn’t have an exception for "lhs" and "rhs" and so long as we are abbreviating, we should use something easy to see, rather than something with so many repeated identical letters. In fact, I don’t even know if I would use the words "owning element", just "a" and "b", given the context of the function.

There’s no need to mark this inline. The compiler will make a good decision about inlining either way. So "inline" basically means "hope it’s OK I put this in the header file". Here we should use just "static". In fact, even if we were using "inline" we’d still also want to use "static" to give these internal linkage.

> Source/WebCore/animation/WebAnimationUtilities.cpp:47
> +    // With regard to pseudo-elements, the sort order is as follows:
> +    //     - element
> +    //     - ::before
> +    //     - ::after
> +    //     - element children

Another way to implement this that could be a little easier to get right is to add an integer for sorting elements and pseudo elements, call it the typeIndex:

Set it to 0 for an element and set the referenceElement to the element.
Set it to 1 for the before element and set the referenceElement to the hostElement.
Set it to 2 for the before element and set the referenceElement to the hostElement.

Then, after computing that for both a and b, we just write:

    if (aReferenceElement == bReferenceElement)
        return aTypeIndex < bTypeIndex;
    return isBefore(aReferenceElement, bReferenceElement));

What’s nice about this is that we don’t have to write repeated, similar but slightly different, code for the before and after elements.

> Source/WebCore/animation/WebAnimationUtilities.cpp:54
> +        if (lhsPseudoElement->hostElement() == rhsPseudoElement->hostElement())
> +            return lhsPseudoElement->isBeforePseudoElement();

I think that this function should correctly return *false* when both elements are the same, even if the element is a before pseudo element, so I suggest adding that check somewhere.

> Source/WebCore/animation/WebAnimationUtilities.cpp:58
> +    // Or comparing a pseudo-element that is compared to another non-pseudo element, in which case
> +    // we want to see if it's hosted on that other element, and if not use its host element to compare.

This comment says we are comparing to a non-pseudo element, but that’s too specific. This is used any time the two elements are not comparing to the sam host.

> Source/WebCore/animation/WebAnimationUtilities.cpp:60
> +        auto* lhsHostElement = downcast<PseudoElement>(lhsOwningElement)->hostElement();

Do we have a strong guarantee that we will never be passed a pseudo element with a host element of null?

> Source/WebCore/animation/WebAnimationUtilities.cpp:76
> +inline bool compareCSSTransitions(const CSSTransition& lhsTransition, const CSSTransition& rhsTransition)

Same thoughts about "a" and "b" and inline.

> Source/WebCore/animation/WebAnimationUtilities.cpp:79
> +    auto* lhsOwningElement = lhsTransition.owningElement();
> +    auto* rhsOwningElement = rhsTransition.owningElement();

Are these possibly null or guaranteed non-null? If guaranteed non-null, please use references. If possibly null, then compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder has a null dereference error where it calls compareDocumentPosition without checking for null.

> Source/WebCore/animation/WebAnimationUtilities.cpp:94
> +inline bool compareCSSAnimations(const CSSAnimation& lhs, const CSSAnimation& rhs)

Same thoughts about "a" and "b" and inline.

> Source/WebCore/animation/WebAnimationUtilities.cpp:98
> +    auto* lhsOwningElement = lhs.owningElement();
> +    auto* rhsOwningElement = rhs.owningElement();

Same thoughts about null.

> Source/WebCore/animation/WebAnimationUtilities.cpp:105
> +    auto* cssAnimationList = lhsOwningElement->ensureKeyframeEffectStack().cssAnimationList();

If this can’t be null, then it should return a reference.

> Source/WebCore/animation/WebAnimationUtilities.cpp:114
> +        if (animation == lhsBackingAnimation)
> +            return true;

This does the wrong thing if both lhsBackingAnimation == rhsBackingAnimation. We need to return false in that case. One way to fix that is to do the check for rhsBackingAnimation first. Another is to check for == before this function. Of course if this can never happen then my comment is silly, but also we could assert they are not the same. If so, then guarantees we aren’t ever passed the same CSSAnimation for both a and b.

[Antoine Quint]

(In reply to Darin Adler from comment #2)
> Comment on attachment 398972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398972&action=review
> 
> One problem here is that functions named "compare" functions aren’t
> obviously C++ standard library "operator<" functions. There’s another
> tradition of compare based on the C standard library that uses int and
> -1/0/1. I think the names probably need something to make it clear that it's
> the "<" style. I worry they will make it a bit inelegant.
> 
> I mention some style issues, but also some actual mistakes that I think you
> should consider fixing.
> 
> > Source/WebCore/animation/WebAnimationUtilities.cpp:41
> > +inline bool compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder(Element* lhsOwningElement, Element* rhsOwningElement)
> 
> I think this code is easier to read with "a" and "b". Our rule against
> abbreviations doesn’t have an exception for "lhs" and "rhs" and so long as
> we are abbreviating, we should use something easy to see, rather than
> something with so many repeated identical letters. In fact, I don’t even
> know if I would use the words "owning element", just "a" and "b", given the
> context of the function.

I'll rename the parameters to all the functions to just `a` and `b`.

> There’s no need to mark this inline. The compiler will make a good decision
> about inlining either way. So "inline" basically means "hope it’s OK I put
> this in the header file". Here we should use just "static". In fact, even if
> we were using "inline" we’d still also want to use "static" to give these
> internal linkage.

Cool! I'll remove `inline`.

> > Source/WebCore/animation/WebAnimationUtilities.cpp:47
> > +    // With regard to pseudo-elements, the sort order is as follows:
> > +    //     - element
> > +    //     - ::before
> > +    //     - ::after
> > +    //     - element children
> 
> Another way to implement this that could be a little easier to get right is
> to add an integer for sorting elements and pseudo elements, call it the
> typeIndex:
> 
> Set it to 0 for an element and set the referenceElement to the element.
> Set it to 1 for the before element and set the referenceElement to the
> hostElement.
> Set it to 2 for the before element and set the referenceElement to the
> hostElement.
> 
> Then, after computing that for both a and b, we just write:
> 
>     if (aReferenceElement == bReferenceElement)
>         return aTypeIndex < bTypeIndex;
>     return isBefore(aReferenceElement, bReferenceElement));
> 
> What’s nice about this is that we don’t have to write repeated, similar but
> slightly different, code for the before and after elements.

Sounds good, will rewrite using this approach.

> > Source/WebCore/animation/WebAnimationUtilities.cpp:54
> > +        if (lhsPseudoElement->hostElement() == rhsPseudoElement->hostElement())
> > +            return lhsPseudoElement->isBeforePseudoElement();
> 
> I think that this function should correctly return *false* when both
> elements are the same, even if the element is a before pseudo element, so I
> suggest adding that check somewhere.

You mean if both a and b are equal (and not their host elements in case they're both pseudos)? That case can't happen as it stands because the only call sites have a check to prevent this. In those cases, the compare function wouldn't be consistent anymore. I'll add a RELEASE_ASSERT() that the function is never called with a == b, just like we do for compareAnimationsByCompositeOrder().

> > Source/WebCore/animation/WebAnimationUtilities.cpp:58
> > +    // Or comparing a pseudo-element that is compared to another non-pseudo element, in which case
> > +    // we want to see if it's hosted on that other element, and if not use its host element to compare.
> 
> This comment says we are comparing to a non-pseudo element, but that’s too
> specific. This is used any time the two elements are not comparing to the
> sam host.
> 
> > Source/WebCore/animation/WebAnimationUtilities.cpp:60
> > +        auto* lhsHostElement = downcast<PseudoElement>(lhsOwningElement)->hostElement();
> 
> Do we have a strong guarantee that we will never be passed a pseudo element
> with a host element of null?

Not within the function itself, only through usage. We only ever sort animations that are "relevant", ie. they're not idle. A CSS-originated animation would be idle as soon as its target is no longer connected to the DOM, and this comparison function is only ever used for CSS-originated animations.

I can add an ASSERT for this case.

> > Source/WebCore/animation/WebAnimationUtilities.cpp:76
> > +inline bool compareCSSTransitions(const CSSTransition& lhsTransition, const CSSTransition& rhsTransition)
> 
> Same thoughts about "a" and "b" and inline.
> 
> > Source/WebCore/animation/WebAnimationUtilities.cpp:79
> > +    auto* lhsOwningElement = lhsTransition.owningElement();
> > +    auto* rhsOwningElement = rhsTransition.owningElement();
> 
> Are these possibly null or guaranteed non-null? If guaranteed non-null,
> please use references. If possibly null, then
> compareDeclarativeAnimationOwningElementPositionsInDocumentTreeOrder has a
> null dereference error where it calls compareDocumentPosition without
> checking for null.

They are guaranteed non-null from the call site, so I'll add an ASSERT() and use references.

> > Source/WebCore/animation/WebAnimationUtilities.cpp:94
> > +inline bool compareCSSAnimations(const CSSAnimation& lhs, const CSSAnimation& rhs)
> 
> Same thoughts about "a" and "b" and inline.
> 
> > Source/WebCore/animation/WebAnimationUtilities.cpp:98
> > +    auto* lhsOwningElement = lhs.owningElement();
> > +    auto* rhsOwningElement = rhs.owningElement();
> 
> Same thoughts about null.
> 
> > Source/WebCore/animation/WebAnimationUtilities.cpp:105
> > +    auto* cssAnimationList = lhsOwningElement->ensureKeyframeEffectStack().cssAnimationList();
> 
> If this can’t be null, then it should return a reference.

That method can definitely return null in case there are no CSS Animations in a keyframe effect stack. However, it should definitely not return null if there is a CSSAnimation with an owning element, hence the ASSERT() right after that call in this context.

> > Source/WebCore/animation/WebAnimationUtilities.cpp:114
> > +        if (animation == lhsBackingAnimation)
> > +            return true;
> 
> This does the wrong thing if both lhsBackingAnimation ==
> rhsBackingAnimation. We need to return false in that case. One way to fix
> that is to do the check for rhsBackingAnimation first. Another is to check
> for == before this function. Of course if this can never happen then my
> comment is silly, but also we could assert they are not the same. If so,
> then guarantees we aren’t ever passed the same CSSAnimation for both a and b.

lhsBackingAnimation and rhsBackingAnimation cannot be the same. This is because one given Animation object can only be associated with one DeclarativeAnimation at any given time, and we have an assertion first thing in this function that checks that lhs and rhs (now a and b) are not the same.

[Antoine Quint]

Committed r261470: <https://trac.webkit.org/changeset/261470>

[Radar WebKit Bug Importer]

<rdar://problem/63081540>