[webkit-reviews] review granted: [Bug 211695] [Web Animations] Refactor animation comparison by composite order in a single utility function : [Attachment 398972] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 211695: [Web Animations] Refactor animation comparison by composite order
in a single utility function
https://bugs.webkit.org/show_bug.cgi?id=211695

Attachment 398972: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 398972
  --> https://bugs.webkit.org/attachment.cgi?id=398972
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.