Bug 182836 - [Web Animations] Ensure that changing the timing model updates styles synchronously
Summary: [Web Animations] Ensure that changing the timing model updates styles synchro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-15 12:56 PST by Antoine Quint
Modified: 2018-02-15 15:34 PST (History)
3 users (show)

See Also:

Attachments
Patch (44.36 KB, patch)
2018-02-15 13:23 PST, Antoine Quint 		no flags Details | Formatted Diff | Diff
Patch for landing (44.30 KB, patch)
2018-02-15 14:06 PST, Antoine Quint 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-02-15 12:56:26 PST 
[Web Animations] Ensure that changing the timing model updates styles synchronously
Comment 1 Antoine Quint 2018-02-15 13:23:16 PST 
Created attachment 333936 [details]
Patch
Comment 2 Dean Jackson 2018-02-15 14:04:09 PST 
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
Comment 3 Antoine Quint 2018-02-15 14:05:26 PST 
(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.
Comment 4 Antoine Quint 2018-02-15 14:06:29 PST 
Created attachment 333940 [details]
Patch for landing
Comment 5 Antoine Quint 2018-02-15 15:33:49 PST 
Committed r228537: <https://trac.webkit.org/changeset/228537>
Comment 6 Radar WebKit Bug Importer 2018-02-15 15:34:47 PST 
<rdar://problem/37586577>