[Web Animations] Make KeyframeEffect target nullable and read-write
Created attachment 333722 [details] Patch
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?
(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.
Committed r228437: <https://trac.webkit.org/changeset/228437>
<rdar://problem/37514127>
Follow-Up build fix in <https://trac.webkit.org/changeset/228439>.
Committed r228440: <https://trac.webkit.org/changeset/228440>
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.
Reopening to attach new patch.
Created attachment 333750 [details] Patch
Created attachment 333766 [details] Patch
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
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
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
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
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).
(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.
Created attachment 333788 [details] Patch
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
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
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
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
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
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
(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.
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
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
Created attachment 333800 [details] Patch
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.
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.
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.
Created attachment 334186 [details] Patch
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
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
Created attachment 334191 [details] Patch
Created attachment 334192 [details] Patch
Created attachment 334193 [details] Patch
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
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
Created attachment 334196 [details] Patch
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.
(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.
Committed r228717: <https://trac.webkit.org/changeset/228717>