[webkit-reviews] review denied: [Bug 182741] [Web Animations] Make KeyframeEffect target nullable and read-write : [Attachment 333766] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Antoine Quint <graouts at apple.com>'s
request for review:
Bug 182741: [Web Animations] Make KeyframeEffect target nullable and read-write
https://bugs.webkit.org/show_bug.cgi?id=182741

Attachment 333766: Patch

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




--- Comment #16 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 333766
  --> https://bugs.webkit.org/attachment.cgi?id=333766
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:61
> +static inline CSSPropertyID IDLAttributeNameToAnimationPropertyName(String
idlAttributeName)

I know that you are just moving code here. This function should take a "const
String&" instead of String because the latter may unnecessarily ref the
underlying StringImpl (if the caller passed an lvalue) and this function does
not take ownership of the passed string (if the caller passed an rvalue).

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:645
> +    auto previousTarget = m_target;
> +    if (previousTarget == newTarget)
> +	   return;

This is causing unnecessary ref-count churn of m_target when previousTarget ==
newTarget since we ref m_target when we do the copy assignment on line 643 only
to deref it on early return. We can avoid the ref-count churn when m_target ==
newTarget by explicitly comparing m_target and newTarget:

if (m_target == newTarget)
    return;

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:647
> +    m_target = newTarget;

Then we can write this as:

auto previousTarget = WTFMove(m_target);
m_target = WTFMove(newTarget);

OR take advantage of std::exchange() to simplify this to:

auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> +    if (auto effectAnimation = animation())

For your consideration, I suggest that we use auto* instead of auto to convey
to the reader that animation() returns a pointer. I know that this can be
inferred from the context, but C++ has all sorts of craziness and it saves me a
second to see the '*' instead of having to read the line below.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:660
> +	   previousTarget->invalidateStyleAndLayerComposition();
> +	   previousTarget->document().updateStyleIfNeeded();

Can we find some way to avoid duplicating this logic here and in
KeyframeEffectReadOnly::invalidate()? One idea is to extract this logic into a
file-level static function F that take an Element&. Then have this function and
KeyframeEffectReadOnly::invalidate() invoke F.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:704
> +    m_target->invalidateStyleAndLayerComposition();
> +    m_target->document().updateStyleIfNeeded();

This duplicates the logic in KeyframeEffectReadOnly::setTarget(). Can we find
some to share it? See my remark above for more details.

> Source/WebCore/animation/WebAnimation.cpp:164
> +void WebAnimation::effectTargetDidChange(RefPtr<Element>&& previousTarget,
RefPtr<Element>&& newTarget)

This function does not take ownership of previousTarget or newTarget. So, these
arguments should be raw pointers.

In general, it is only beneficial to pass by rvalue reference when the
function- or a function it calls- takes ownership of the passed value (by
WTFMove()'ing it into something).