Bug 182741 – [Web Animations] Make KeyframeEffect target nullable and read-write

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

[Antoine Quint]

[Web Animations] Make KeyframeEffect target nullable and read-write

[Antoine Quint]

Created attachment 333722 [details]
Patch

[Dean Jackson]

Comment on attachment 333722 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        We used to completely disregard null targets, for instance not parsing keyframes, but targets
> +        can be null and are also supposed to be read-write for KeyframeEffect. We now update the IDL

This sentence is weird.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
> +    if (!scriptExecutionContext.isDocument())
> +        return Exception { TypeError };

Is there any way to create a keyframe animation in a worker that operates on a non-element?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:558
> +    auto& document = downcast<Document>(scriptExecutionContext);
> +    auto documentElement = document.documentElement();

No need for the document local variable.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:586
> +        renderStyle->fontCascade().update(nullptr);

Maybe put a comment here?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:656
> +        previousTarget->invalidateStyleAndLayerComposition();
> +        previousTarget->document().updateStyleIfNeeded();

Why do you need both?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:700
> +    m_target->invalidateStyleAndLayerComposition();
> +    m_target->document().updateStyleIfNeeded();

Ditto?

[Antoine Quint]

(In reply to Dean Jackson from comment #2)
> Comment on attachment 333722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333722&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
> > +    if (!scriptExecutionContext.isDocument())
> > +        return Exception { TypeError };
> 
> Is there any way to create a keyframe animation in a worker that operates on
> a non-element?

No, it's only exposed on Window.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:558
> > +    auto& document = downcast<Document>(scriptExecutionContext);
> > +    auto documentElement = document.documentElement();
> 
> No need for the document local variable.

Will remove.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:586
> > +        renderStyle->fontCascade().update(nullptr);
> 
> Maybe put a comment here?

Will do.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:656
> > +        previousTarget->invalidateStyleAndLayerComposition();
> > +        previousTarget->document().updateStyleIfNeeded();
> 
> Why do you need both?
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:700
> > +    m_target->invalidateStyleAndLayerComposition();
> > +    m_target->document().updateStyleIfNeeded();
> 
> Ditto?

I'll add comments there.

[Antoine Quint]

Committed r228437: <https://trac.webkit.org/changeset/228437>

[Radar WebKit Bug Importer]

<rdar://problem/37514127>

[Chris Dumez]

Follow-Up build fix in <https://trac.webkit.org/changeset/228439>.

[Antoine Quint]

Committed r228440: <https://trac.webkit.org/changeset/228440>

[Chris Dumez]

Comment on attachment 333722 [details]
Patch

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

>>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
>>> +        return Exception { TypeError };
>> 
>> Is there any way to create a keyframe animation in a worker that operates on a non-element?
> 
> No, it's only exposed on Window.

We prefer is<Document>(scriptExecutionContext) nowadays. Looks nice with the downcast<Document>() below.

> Source/WebCore/animation/KeyframeEffectReadOnly.h:82
> +    void setTarget(RefPtr<Element>);

RefPtr<Element>&& ?

> Source/WebCore/animation/WebAnimation.cpp:164
> +void WebAnimation::effectTargetDidChange(RefPtr<Element> previousTarget, RefPtr<Element> newTarget)

RefPtr<Element>&& ? This is doing refcounting churn. or Element* if you're now transferring ownership.

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 333750 [details]
Patch

[Antoine Quint]

Created attachment 333766 [details]
Patch

[EWS Watchlist]

Comment on attachment 333766 [details]
Patch

Attachment 333766 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6495570

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 333770 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 333766 [details]
Patch

Attachment 333766 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6495772

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 333771 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Daniel Bates]

Comment on attachment 333766 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:61
> +static inline CSSPropertyID IDLAttributeNameToAnimationPropertyName(String idlAttributeName)

I know that you are just moving code here. This function should take a "const String&" instead of String because the latter may unnecessarily ref the underlying StringImpl (if the caller passed an lvalue) and this function does not take ownership of the passed string (if the caller passed an rvalue).

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:645
> +    auto previousTarget = m_target;
> +    if (previousTarget == newTarget)
> +        return;

This is causing unnecessary ref-count churn of m_target when previousTarget == newTarget since we ref m_target when we do the copy assignment on line 643 only to deref it on early return. We can avoid the ref-count churn when m_target == newTarget by explicitly comparing m_target and newTarget:

if (m_target == newTarget)
    return;

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:647
> +    m_target = newTarget;

Then we can write this as:

auto previousTarget = WTFMove(m_target);
m_target = WTFMove(newTarget);

OR take advantage of std::exchange() to simplify this to:

auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> +    if (auto effectAnimation = animation())

For your consideration, I suggest that we use auto* instead of auto to convey to the reader that animation() returns a pointer. I know that this can be inferred from the context, but C++ has all sorts of craziness and it saves me a second to see the '*' instead of having to read the line below.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:660
> +        previousTarget->invalidateStyleAndLayerComposition();
> +        previousTarget->document().updateStyleIfNeeded();

Can we find some way to avoid duplicating this logic here and in KeyframeEffectReadOnly::invalidate()? One idea is to extract this logic into a file-level static function F that take an Element&. Then have this function and KeyframeEffectReadOnly::invalidate() invoke F.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:704
> +    m_target->invalidateStyleAndLayerComposition();
> +    m_target->document().updateStyleIfNeeded();

This duplicates the logic in KeyframeEffectReadOnly::setTarget(). Can we find some to share it? See my remark above for more details.

> Source/WebCore/animation/WebAnimation.cpp:164
> +void WebAnimation::effectTargetDidChange(RefPtr<Element>&& previousTarget, RefPtr<Element>&& newTarget)

This function does not take ownership of previousTarget or newTarget. So, these arguments should be raw pointers.

In general, it is only beneficial to pass by rvalue reference when the function- or a function it calls- takes ownership of the passed value (by WTFMove()'ing it into something).

[Antoine Quint]

(In reply to Daniel Bates from comment #16)
> Comment on attachment 333766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333766&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:61
> > +static inline CSSPropertyID IDLAttributeNameToAnimationPropertyName(String idlAttributeName)
> 
> I know that you are just moving code here. This function should take a
> "const String&" instead of String because the latter may unnecessarily ref
> the underlying StringImpl (if the caller passed an lvalue) and this function
> does not take ownership of the passed string (if the caller passed an
> rvalue).

As a matter of fact, I did make a change in IDLAttributeNameToAnimationPropertyName, so I'll make the suggested change.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:645
> > +    auto previousTarget = m_target;
> > +    if (previousTarget == newTarget)
> > +        return;
> 
> This is causing unnecessary ref-count churn of m_target when previousTarget
> == newTarget since we ref m_target when we do the copy assignment on line
> 643 only to deref it on early return. We can avoid the ref-count churn when
> m_target == newTarget by explicitly comparing m_target and newTarget:
> 
> if (m_target == newTarget)
>     return;
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:647
> > +    m_target = newTarget;
> 
> Then we can write this as:
> 
> auto previousTarget = WTFMove(m_target);
> m_target = WTFMove(newTarget);
> 
> OR take advantage of std::exchange() to simplify this to:
> 
> auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

Actually, this sets m_target to a null value in both cases. This works:

auto previousTarget = std::exchange(m_target, newTarget);

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> > +    if (auto effectAnimation = animation())
> 
> For your consideration, I suggest that we use auto* instead of auto to
> convey to the reader that animation() returns a pointer. I know that this
> can be inferred from the context, but C++ has all sorts of craziness and it
> saves me a second to see the '*' instead of having to read the line below.

Will fix in updated patch.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:660
> > +        previousTarget->invalidateStyleAndLayerComposition();
> > +        previousTarget->document().updateStyleIfNeeded();
> 
> Can we find some way to avoid duplicating this logic here and in
> KeyframeEffectReadOnly::invalidate()? One idea is to extract this logic into
> a file-level static function F that take an Element&. Then have this
> function and KeyframeEffectReadOnly::invalidate() invoke F.
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:704
> > +    m_target->invalidateStyleAndLayerComposition();
> > +    m_target->document().updateStyleIfNeeded();
> 
> This duplicates the logic in KeyframeEffectReadOnly::setTarget(). Can we
> find some to share it? See my remark above for more details.

I'll add `static inline void invalidateElement(Element* element)` in an updated patch.

> > Source/WebCore/animation/WebAnimation.cpp:164
> > +void WebAnimation::effectTargetDidChange(RefPtr<Element>&& previousTarget, RefPtr<Element>&& newTarget)
> 
> This function does not take ownership of previousTarget or newTarget. So,
> these arguments should be raw pointers.
> 
> In general, it is only beneficial to pass by rvalue reference when the
> function- or a function it calls- takes ownership of the passed value (by
> WTFMove()'ing it into something).

Sure, will fix in new patch.

[Antoine Quint]

Created attachment 333788 [details]
Patch

[EWS Watchlist]

Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6499523

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 333790 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6499475

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 333793 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6499662

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 333794 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

(In reply to Antoine Quint from comment #17)
> (In reply to Daniel Bates from comment #16)
> > Comment on attachment 333766 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333766&action=review
> > 
> > OR take advantage of std::exchange() to simplify this to:
> > 
> > auto previousTarget = std::exchange(m_target, WTFMove(newTarget));
> 
> Actually, this sets m_target to a null value in both cases. This works:

That was my mistake, this works fine. I'll have a new patch up shortly.

[EWS Watchlist]

Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6500337

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 333799 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 333800 [details]
Patch

[Daniel Bates]

Comment on attachment 333800 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:78
> +    // FIXME: we don't support the CSS "offset" property

The remark after the FIXME should be complete sentences that begin with a capital letter and ends with a period.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:516
>      // 1. If object is null, return an empty sequence of keyframes.

This comment no longer matches what the code does OR I am unclear what "object" refers to in this comment. Is the code correct?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:517
> +    if (!keyframesInput.get())

No need for calling get(). It is sufficient to write this as:

if (!keyframesInput)

(Assuming this change is correct. See my remark above about the discrepancy with the comment).

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:564
> +    if (!is<Document>(scriptExecutionContext))
> +        return Exception { TypeError };

From talking with you today in person, you clarified that this code would never run in a worker. So, I suggest we turn this into an assertion and assert it at the top of the function.

[Chris Dumez]

Comment on attachment 333800 [details]
Patch

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

>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:564
>> +        return Exception { TypeError };
> 
> From talking with you today in person, you clarified that this code would never run in a worker. So, I suggest we turn this into an assertion and assert it at the top of the function.

If this is true then you may want to consider using CallWith=Document instead in the IDL.

[Antoine Quint]

Comment on attachment 333800 [details]
Patch

After discussions with Ryosuke, Chris and Antti, we've decided there is some preliminary work to be made before we can implement this elegantly. Specifically, we should decouple parsing keyframes from a JS object and actually building the data structure that contains resolved styles. The JS object parsing should be done in custom bindings code and the styles resolution once we have a target to work with.

[Antoine Quint]

Created attachment 334186 [details]
Patch

[EWS Watchlist]

Comment on attachment 334186 [details]
Patch

Attachment 334186 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6578143

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 334190 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 334191 [details]
Patch

[Antoine Quint]

Created attachment 334192 [details]
Patch

[Antoine Quint]

Created attachment 334193 [details]
Patch

[EWS Watchlist]

Comment on attachment 334193 [details]
Patch

Attachment 334193 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6579038

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html

[EWS Watchlist]

Created attachment 334195 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 334196 [details]
Patch

[Dean Jackson]

Comment on attachment 334196 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:53
> +    element->invalidateStyleAndLayerComposition();
> +    element->document().updateStyleIfNeeded();

Did you ever work out why we need to call both here? Shouldn't Element::invalidateStyleAndLayerComposition call document().updateStyleIfNeeded()?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:72
> +    // 1. If attribute conforms to the <custom-property-name> production, return attribute.

Add a comment as to why this isn't being handled.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:76
> +    // 3. If attribute is the string "cssOffset", then return an animation property representing the CSS offset property.

Add a blank line here.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:167
> +                easing = convert<IDLDOMString>(state, keyframe->get(&state, ownPropertyName));

Why do you have to pass the ExecState to the keyframe?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> +    auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

std::exchange, nice.

[Antoine Quint]

(In reply to Dean Jackson from comment #41)
> Comment on attachment 334196 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334196&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:53
> > +    element->invalidateStyleAndLayerComposition();
> > +    element->document().updateStyleIfNeeded();
> 
> Did you ever work out why we need to call both here? Shouldn't
> Element::invalidateStyleAndLayerComposition call
> document().updateStyleIfNeeded()?

Zalan explained that invalidateStyleAndLayerComposition() only sets the dirty flags and the document needs to be updated manually.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:72
> > +    // 1. If attribute conforms to the <custom-property-name> production, return attribute.
> 
> Add a comment as to why this isn't being handled.

Will fix in commit.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:76
> > +    // 3. If attribute is the string "cssOffset", then return an animation property representing the CSS offset property.
> 
> Add a blank line here.

Will fix in commit.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:167
> > +                easing = convert<IDLDOMString>(state, keyframe->get(&state, ownPropertyName));
> 
> Why do you have to pass the ExecState to the keyframe?

The keyframe is a JSC object and to obtain its value you must pass the state.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> > +    auto previousTarget = std::exchange(m_target, WTFMove(newTarget));
> 
> std::exchange, nice.

I can't take credit for that, dbates suggested that in a previous review.

[Antoine Quint]

Committed r228717: <https://trac.webkit.org/changeset/228717>