Summary: | [Web Animations] Store an Element / PseudoId pair to define the KeyframeEffect target | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||
Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, dino, esprehn+autocc, ews-watchlist, graouts, koivisto, kondapallykalyan, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=207290 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 217936 | ||||||
Attachments: |
|
Description
Antoine Quint
2020-04-14 06:35:15 PDT
Created attachment 396409 [details]
Patch
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 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() Committed r260076: <https://trac.webkit.org/changeset/260076> |