[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
bugzilla-daemon at webkit.org
Tue Apr 14 07:05:12 PDT 2020
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).
More information about the webkit-reviews
mailing list