Bug 210491 – [Web Animations] Store an Element / PseudoId pair to define the KeyframeEffect target

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

[Web Animations] Store an Element / PseudoId pair to define the KeyframeEffect target

[Antoine Quint]

Created attachment 396409 [details]
Patch

[Antti Koivisto]

Comment on attachment 396409 [details]
Patch

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

> Source/WebCore/animation/DeclarativeAnimation.cpp:116
> +        setEffect(KeyframeEffect::create(*m_owningElement, m_owningElement->pseudoId()));

pseudoId() is always None here so you should just pass that.

> Source/WebCore/animation/KeyframeEffect.cpp:763
> -    auto& styleResolver = m_target->styleResolver();
> +    auto& styleResolver = target()->styleResolver();

m_target->styleResolver() is the same.

> Source/WebCore/animation/KeyframeEffect.cpp:800
> -    auto* renderer = m_target->renderer();
> +    auto* renderer = target()->renderer();
>      if (!renderer || !renderer->parent())

You can just use the existing renderer() helper.

> Source/WebCore/animation/KeyframeEffect.cpp:803
> -    auto* frameView = m_target->document().view();
> +    auto* frameView = target()->document().view();

You could add document() helper.

> Source/WebCore/animation/KeyframeEffect.cpp:1075
> +Element* KeyframeEffect::target() const
> +{
> +    if (m_pseudoId == PseudoId::None)
> +        return m_target.get();

Either target() or m_target should be renamed since they don't return the same thing.

I think a good strategy is to call the function something descriptive like targetElementOrPseudoElement() and then try to reduce its usage. Note that not all call sites need to use it (m_target->document() is fine for example).

[Antti Koivisto]

Comment on attachment 396409 [details]
Patch

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

>> Source/WebCore/animation/KeyframeEffect.cpp:1075
>> +        return m_target.get();
> 
> Either target() or m_target should be renamed since they don't return the same thing.
> 
> I think a good strategy is to call the function something descriptive like targetElementOrPseudoElement() and then try to reduce its usage. Note that not all call sites need to use it (m_target->document() is fine for example).

You may still want target() accessor that just returns m_target and matches setTarget()

[Antoine Quint]

Committed r260076: <https://trac.webkit.org/changeset/260076>

[Radar WebKit Bug Importer]

<rdar://problem/61774687>