Bug 182836 – [Web Animations] Ensure that changing the timing model updates styles synchronously

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Web Animations] Ensure that changing the timing model updates styles synchronously

[Antoine Quint]

Created attachment 333936 [details]
Patch

[Dean Jackson]

Comment on attachment 333936 [details]
Patch

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

> Source/WebCore/animation/AnimationEffectReadOnly.cpp:47
> +    m_timing->setEffect(nullptr);

Won't m_timing go away and stop referencing the effect anyway?

> Source/WebCore/animation/AnimationEffectTimingReadOnly.cpp:114
> -    m_iterationStart = iterationStart;
> +    if (m_iterationStart != iterationStart) {
> +        m_iterationStart = iterationStart;
> +        propertyDidChange();
> +    }
> +

While it is more core, we usually write it as an early return:

if (m_iterationStart == iterationStart)
  return { };

m_iterationStart = iterationStart;
propertyDidChange();

return { };

> Source/WebCore/animation/AnimationEffectTimingReadOnly.cpp:127
> +        m_iterations = iterations;

DItto

[Antoine Quint]

(In reply to Dean Jackson from comment #2)
> Comment on attachment 333936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333936&action=review
> 
> > Source/WebCore/animation/AnimationEffectReadOnly.cpp:47
> > +    m_timing->setEffect(nullptr);
> 
> Won't m_timing go away and stop referencing the effect anyway?

Since both effect and timing objects can be referenced from JS, the timing may outlive the effect.

> > Source/WebCore/animation/AnimationEffectTimingReadOnly.cpp:114
> > -    m_iterationStart = iterationStart;
> > +    if (m_iterationStart != iterationStart) {
> > +        m_iterationStart = iterationStart;
> > +        propertyDidChange();
> > +    }
> > +
> 
> While it is more core, we usually write it as an early return:
> 
> if (m_iterationStart == iterationStart)
>   return { };
> 
> m_iterationStart = iterationStart;
> propertyDidChange();
> 
> return { };

I wanted to avoid the double returns, but I'll adopt that style.

> > Source/WebCore/animation/AnimationEffectTimingReadOnly.cpp:127
> > +        m_iterations = iterations;
> 
> DItto

OK.

[Antoine Quint]

Created attachment 333940 [details]
Patch for landing

[Antoine Quint]

Committed r228537: <https://trac.webkit.org/changeset/228537>

[Radar WebKit Bug Importer]

<rdar://problem/37586577>