[webkit-reviews] review granted: [Bug 210491] [Web Animations] Store an Element / PseudoId pair to define the KeyframeEffect target : [Attachment 396409] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 210491: [Web Animations] Store an Element / PseudoId pair to define the
KeyframeEffect target
https://bugs.webkit.org/show_bug.cgi?id=210491

Attachment 396409: Patch

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




--- Comment #2 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 396409
  --> https://bugs.webkit.org/attachment.cgi?id=396409
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).