Bug 210491 - [Web Animations] Store an Element / PseudoId pair to define the KeyframeEffect target
Summary: [Web Animations] Store an Element / PseudoId pair to define the KeyframeEffec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks: 217936
  Show dependency treegraph
 
Reported: 2020-04-14 06:35 PDT by Antoine Quint
Modified: 2020-10-19 18:10 PDT (History)
8 users (show)

See Also:


Attachments
Patch (17.41 KB, patch)
2020-04-14 06:38 PDT, Antoine Quint
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-04-14 06:35:15 PDT
[Web Animations] Store an Element / PseudoId pair to define the KeyframeEffect target
Comment 1 Antoine Quint 2020-04-14 06:38:25 PDT
Created attachment 396409 [details]
Patch
Comment 2 Antti Koivisto 2020-04-14 07:05:12 PDT
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).
Comment 3 Antti Koivisto 2020-04-14 07:07:24 PDT
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()
Comment 4 Antoine Quint 2020-04-14 09:19:54 PDT
Committed r260076: <https://trac.webkit.org/changeset/260076>
Comment 5 Radar WebKit Bug Importer 2020-04-14 09:20:14 PDT
<rdar://problem/61774687>