Diff
Modified: trunk/LayoutTests/ChangeLog (232184 => 232185)
--- trunk/LayoutTests/ChangeLog 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/LayoutTests/ChangeLog 2018-05-25 13:45:15 UTC (rev 232185)
@@ -1,3 +1,21 @@
+2018-05-25 Antoine Quint <grao...@apple.com>
+
+ [Web Animations] WebAnimation objects never get destroyed
+ https://bugs.webkit.org/show_bug.cgi?id=185917
+ <rdar://problem/39539371>
+
+ Reviewed by Dean Jackson and Antti Koivisto.
+
+ Add a new test that would fail before this fix since the Document would leak. We also remove a homegrown test that was not correct
+ and is no longer relevant thanks to the tests under imported/mozilla.
+
+ * animations/leak-document-with-css-animation-expected.txt: Added.
+ * animations/leak-document-with-css-animation.html: Added.
+ * animations/resources/animation-leak-iframe.html: Added.
+ * platform/win/TestExpectations:
+ * webanimations/css-transitions-expected.txt: Removed.
+ * webanimations/css-transitions.html: Removed.
+
2018-05-24 Frederic Wang <fw...@igalia.com>
Import Web Platform Tests for WOFF2
Added: trunk/LayoutTests/animations/leak-document-with-css-animation-expected.txt (0 => 232185)
--- trunk/LayoutTests/animations/leak-document-with-css-animation-expected.txt (rev 0)
+++ trunk/LayoutTests/animations/leak-document-with-css-animation-expected.txt 2018-05-25 13:45:15 UTC (rev 232185)
@@ -0,0 +1,13 @@
+This test asserts that Document doesn't leak when a CSS Animation is created.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+The iframe has finished loading.
+The iframe has been destroyed.
+PASS internals.numberOfLiveDocuments() is numberOfLiveDocumentsAfterIframeLoaded - 1
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/animations/leak-document-with-css-animation.html (0 => 232185)
--- trunk/LayoutTests/animations/leak-document-with-css-animation.html (rev 0)
+++ trunk/LayoutTests/animations/leak-document-with-css-animation.html 2018-05-25 13:45:15 UTC (rev 232185)
@@ -0,0 +1,46 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<html>
+<body _onload_="runTest()">
+<script src=""
+<script>
+description("This test asserts that Document doesn't leak when a CSS Animation is created.");
+
+if (window.internals)
+ jsTestIsAsync = true;
+
+gc();
+
+var numberOfLiveDocumentsAfterIframeLoaded = 0;
+
+function runTest() {
+ if (!window.internals)
+ return;
+
+ var frame = document.body.appendChild(document.createElement("iframe"));
+
+ frame._onload_ = function() {
+ if (frame.src ="" 'about:blank')
+ return true;
+
+ numberOfLiveDocumentsAfterIframeLoaded = internals.numberOfLiveDocuments();
+ debug("The iframe has finished loading.");
+
+ frame.remove();
+ frame = null;
+
+ gc();
+ setTimeout(function () {
+ debug("The iframe has been destroyed.");
+ shouldBe("internals.numberOfLiveDocuments()", "numberOfLiveDocumentsAfterIframeLoaded - 1");
+ debug("");
+ finishJSTest();
+ });
+ }
+
+ frame.src = '';
+}
+
+</script>
+<script src=""
+</body>
+</html>
\ No newline at end of file
Added: trunk/LayoutTests/animations/resources/animation-leak-iframe.html (0 => 232185)
--- trunk/LayoutTests/animations/resources/animation-leak-iframe.html (rev 0)
+++ trunk/LayoutTests/animations/resources/animation-leak-iframe.html 2018-05-25 13:45:15 UTC (rev 232185)
@@ -0,0 +1,25 @@
+<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
+<html>
+<head>
+ <title></title>
+ <style type="text/css" media="screen">
+
+ div {
+ width: 100px;
+ height: 100px;
+ background-color: black;
+ animation: move 1s infinite;
+ }
+
+ @keyframes move {
+ 50% { margin-left: 60px; }
+ }
+
+ </style>
+</head>
+<body>
+
+<div></div>
+
+</body>
+</html>
Modified: trunk/LayoutTests/platform/win/TestExpectations (232184 => 232185)
--- trunk/LayoutTests/platform/win/TestExpectations 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/LayoutTests/platform/win/TestExpectations 2018-05-25 13:45:15 UTC (rev 232185)
@@ -4023,7 +4023,6 @@
webkit.org/b/183393 fast/loader/window-open-to-invalid-url-disallowed.html [ Skip ]
webkit.org/b/183569 webanimations/css-animations.html [ Failure ]
-webkit.org/b/183569 webanimations/css-transitions.html [ Failure ]
webkit.org/b/183953 imported/mozilla/css-animations/test_animation-cancel.html [ Failure ]
webkit.org/b/183953 imported/mozilla/css-animations/test_animation-finish.html [ Failure ]
Deleted: trunk/LayoutTests/webanimations/css-transitions-expected.txt (232184 => 232185)
--- trunk/LayoutTests/webanimations/css-transitions-expected.txt 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/LayoutTests/webanimations/css-transitions-expected.txt 2018-05-25 13:45:15 UTC (rev 232185)
@@ -1,11 +0,0 @@
-
-PASS A CSS Transition should be reflected entirely as a CSSTransition object on the timeline.
-PASS Web Animations should reflect the transition-delay property.
-PASS Web Animations should reflect the transition-duration property.
-PASS Web Animations should reflect the transition-property property.
-PASS Web Animations should not reflect the transition-timing-function property on the effect's timing.
-PASS Calling finish() on the animation no longer lists the animation after it has been running.
-PASS Seeking the animation to its end time no longer lists the animation after it has been running.
-PASS Setting the target's transition-property to none no longer lists the animation after it has been running.
-PASS Seeking the target's transition-duration to 0 no longer lists the animation after it has been running.
-
Deleted: trunk/LayoutTests/webanimations/css-transitions.html (232184 => 232185)
--- trunk/LayoutTests/webanimations/css-transitions.html 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/LayoutTests/webanimations/css-transitions.html 2018-05-25 13:45:15 UTC (rev 232185)
@@ -1,188 +0,0 @@
-<!DOCTYPE html><!-- webkit-test-runner [ enableWebAnimationsCSSIntegration=true ] -->
-<meta charset=utf-8>
-<title>CSS Transitions</title>
-<body>
-<script src=""
-<script src=""
-<script>
-
-'use strict';
-
-function targetTest(testCallback, description)
-{
- test(() => {
- const target = document.body.appendChild(document.createElement("div"));
- testCallback(target);
- target.remove();
- }, description);
-}
-
-function forceStyleUpdate(element)
-{
- element.offsetLeft;
-}
-
-targetTest(target => {
- target.style.left = "50px";
-
- assert_array_equals(target.getAnimations(), [], "An element should not have any animations initially.");
-
- target.style.transitionProperty = "left";
- target.style.transitionDelay = "-1s";
- target.style.transitionDuration = "2s";
- target.style.transitionTimingFunction = "linear";
-
- assert_array_equals(target.getAnimations(), [], "Setting CSS transitions properties should not yield a CSSTransition until a new target value is specified.");
-
- forceStyleUpdate(target);
- target.style.left = "100px";
-
- const animation = target.getAnimations()[0];
- assert_true(animation instanceof CSSTransition, "The animation is a CSSTransition.");
- assert_true(animation instanceof Animation, "The animation is an Animation.");
- assert_equals(animation.timeline, target.ownerDocument.timeline, "The animation's timeline is set to the element's document timeline.");
- assert_equals(animation.playState, "running", "The animation is running as soon as it's created.");
- assert_equals(animation.transitionProperty, "left", "The animation's transitionProperty is set.");
-
- const effect = animation.effect;
- assert_true(effect instanceof KeyframeEffectReadOnly, "The animation's effect is a KeyframeEffectReadOnly.");
- assert_false(effect instanceof KeyframeEffect, "The animation's effect is not a KeyframeEffect.");
- assert_equals(effect.target, target, "The animation's effect is targeting the target.");
-
- const timing = animation.effect.timing;
- assert_equals(timing.delay, -1000, "The animation's delay property matches the transition-delay property.");
- assert_equals(timing.duration, 2000, "The animation's duration property matches the transition-duration property.");
-
- const computedTiming = animation.effect.getComputedTiming();
- assert_equals(computedTiming.activeDuration, 2000, "The animations's computed timing activeDuration property matches the properties set by CSS");
- assert_equals(computedTiming.currentIteration, 0, "The animations's computed timing currentIteration property matches the properties set by CSS");
- assert_equals(computedTiming.delay, -1000, "The animations's computed timing delay property matches the properties set by CSS");
- assert_equals(computedTiming.duration, 2000, "The animations's computed timing duration property matches the properties set by CSS");
- assert_equals(computedTiming.endDelay, 0, "The animations's computed timing endDelay property matches the properties set by CSS");
- assert_equals(computedTiming.endTime, 1000, "The animations's computed timing endTime property matches the properties set by CSS");
- assert_equals(computedTiming.iterationStart, 0, "The animations's computed timing iterationStart property matches the properties set by CSS");
- assert_equals(computedTiming.iterations, 1, "The animations's computed timing iterations property matches the properties set by CSS");
- assert_equals(computedTiming.localTime, 0, "The animations's computed timing localTime property matches the properties set by CSS");
- assert_equals(computedTiming.progress, 0.5, "The animations's computed timing progress property matches the properties set by CSS");
- assert_equals(computedTiming.fill, "backwards", "The animations's computed timing fill property matches the default value set for transitions");
- assert_equals(computedTiming.easing, "linear", "The animations's computed timing easing property matches the properties set by CSS");
- assert_equals(computedTiming.direction, "normal", "The animations's computed timing direction property matches the properties set by CSS");
-
- const keyframes = animation.effect.getKeyframes();
- assert_equals(keyframes.length, 2, "The animation's effect has two keyframes.");
- assert_equals(keyframes[0].offset, 0, "The animation's effect's first keyframe has a 0 offset.");
- assert_equals(keyframes[0].left, "50px", "The animation's effect's first keyframe has its left property set to 50px.");
- assert_equals(keyframes[1].offset, 1, "The animation's effect's first keyframe has a 1 offset.");
- assert_equals(keyframes[1].left, "100px", "The animation's effect's first keyframe has its left property set to 100px.");
-
- assert_equals(getComputedStyle(effect.target).left, "75px", "The animation's target's computed style reflects the animation state.");
-}, "A CSS Transition should be reflected entirely as a CSSTransition object on the timeline.");
-
-function transitionProperty(target, propertyName, from, to)
-{
- target.style[propertyName] = from;
- forceStyleUpdate(target);
- target.style[propertyName] = to;
-}
-
-function transitionPropertyUpdateTest(testCallback, description)
-{
- targetTest(target => {
- target.style.transitionProperty = "left";
- target.style.transitionDelay = "-500ms";
- target.style.transitionDuration = "2s";
- transitionProperty(target, "left", "50px", "100px");
- testCallback(target);
- }, description);
-}
-
-transitionPropertyUpdateTest(target => {
- const initialAnimation = target.getAnimations()[0];
- assert_equals(initialAnimation.effect.timing.delay, -500, "The animation's delay matches the initial transition-delay property.");
-
- target.style.transitionDelay = 0;
- const updatedAnimation = target.getAnimations()[0];
- assert_equals(updatedAnimation.effect.timing.delay, 0, "The animation's delay matches the updated transition-delay property.");
-
- assert_not_equals(initialAnimation, updatedAnimation, "The animations before and after updating the transition-delay property differ.");
-}, "Web Animations should reflect the transition-delay property.");
-
-transitionPropertyUpdateTest(target => {
- const initialAnimation = target.getAnimations()[0];
- assert_equals(initialAnimation.effect.timing.duration, 2000, "The animation's duration matches the initial transition-duration property.");
-
- target.style.transitionDuration = "1s";
- const updatedAnimation = target.getAnimations()[0];
- assert_equals(updatedAnimation.effect.timing.duration, 1000, "The animation's duration matches the updated transition-duration property.");
-
- assert_not_equals(initialAnimation, updatedAnimation, "The animations before and after updating the transition-duration property differ.");
-}, "Web Animations should reflect the transition-duration property.");
-
-targetTest(target => {
- target.style.transitionDuration = "2s";
-
- target.style.transitionProperty = "left";
- transitionProperty(target, "left", "50px", "100px");
-
- const initialAnimation = target.getAnimations()[0];
- assert_equals(target.getAnimations()[0].transitionProperty, "left", "The animation's property matches the initial transition-property CSS property.");
-
- const initialKeyframes = initialAnimation.effect.getKeyframes();
- assert_equals(initialKeyframes.length, 2);
- assert_equals(initialKeyframes[0].offset, 0);
- assert_equals(initialKeyframes[0].left, "50px");
- assert_equals(initialKeyframes[1].offset, 1);
- assert_equals(initialKeyframes[1].left, "100px");
-
- target.style.transitionProperty = "top";
- transitionProperty(target, "top", "50px", "100px");
-
- const updatedAnimation = target.getAnimations()[0];
- assert_equals(updatedAnimation.transitionProperty, "top", "The animation's property matches the updated transition-property CSS property.");
-
- const updatedKeyframes = updatedAnimation.effect.getKeyframes();
- assert_equals(updatedKeyframes.length, 2);
- assert_equals(updatedKeyframes[0].offset, 0);
- assert_equals(updatedKeyframes[0].top, "50px");
- assert_equals(updatedKeyframes[1].offset, 1);
- assert_equals(updatedKeyframes[1].top, "100px");
-
- assert_not_equals(updatedAnimation, initialAnimation, "Changing the transition-property property generates a different CSSTransition object.");
-}, "Web Animations should reflect the transition-property property.");
-
-transitionPropertyUpdateTest(target => {
- const initialAnimation = target.getAnimations()[0];
- assert_equals(initialAnimation.effect.timing.easing, "linear", "The animation's easing does not match the default transition-timing-function property.");
-
- target.style.transitionTimingFunction = "ease-in";
- const updatedAnimation = target.getAnimations()[0];
- assert_equals(updatedAnimation.effect.timing.easing, "linear", "The animation's easing does not match the updated transition-timing-function property.");
-
- target.style.removeProperty("transition-timing-function");
- const finalAnimation = target.getAnimations()[0];
- assert_equals(target.getAnimations()[0].effect.timing.easing, "linear", "The animation's easing does not match the default transition-timing-function value when the property is not set.");
-}, "Web Animations should not reflect the transition-timing-function property on the effect's timing.");
-
-function runAnimationCompletionTest(finalAssertionCallback, description)
-{
- targetTest(target => {
- target.style.transitionProperty = "left";
- target.style.transitionDuration = "1s";
- transitionProperty(target, "left", "50px", "100px");
-
- assert_equals(target.getAnimations().length, 1, "Seting the transition-duration property on top of the transition-property yields a CSSTransition.");
- assert_equals(target.getAnimations()[0].playState, "running", "Seting the transition-duration property on top of the transition-property yields a running CSSTransition.");
-
- finalAssertionCallback(target.getAnimations()[0]);
-
- assert_array_equals(target.getAnimations(), [], `${description} no longer lists the animation.`);
- }, `${description} no longer lists the animation after it has been running.`);
-}
-
-runAnimationCompletionTest(animation => animation.finish(), "Calling finish() on the animation");
-runAnimationCompletionTest(animation => animation.currentTime = animation.effect.timing.duration, "Seeking the animation to its end time");
-runAnimationCompletionTest(animation => animation.effect.target.style.transitionProperty = "none", "Setting the target's transition-property to none");
-runAnimationCompletionTest(animation => animation.effect.target.style.transitionDuration = 0, "Seeking the target's transition-duration to 0");
-
-</script>
-</body>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (232184 => 232185)
--- trunk/Source/WebCore/ChangeLog 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/ChangeLog 2018-05-25 13:45:15 UTC (rev 232185)
@@ -1,3 +1,77 @@
+2018-05-25 Antoine Quint <grao...@apple.com>
+
+ [Web Animations] WebAnimation objects never get destroyed
+ https://bugs.webkit.org/show_bug.cgi?id=185917
+ <rdar://problem/39539371>
+
+ Reviewed by Dean Jackson and Antti Koivisto.
+
+ The AnimationTimeline class keeps references to WebAnimation objects organized in various ways. First, there
+ are three main maps across which all animations are stored, one for non-subclass WebAnimation objects
+ (m_elementToAnimationsMap), one for CSSSAnimation objects (m_elementToCSSAnimationsMap) and one for CSSTranstion
+ objects (m_elementToCSSTransitionsMap). On top of that, we also keep a map to access CSSAnimation objects for
+ a given element by CSS animation name (m_elementToCSSAnimationByName) and another map to access CSSTransition
+ objects for a given element by CSS property (m_elementToCSSTransitionByCSSPropertyID).
+
+ None of the RefPtr<WebAnimation> stored in these maps would get cleared when the document would get torn down,
+ which would also prevent the AnimationTimeline (and its DocumentTimeline subclass) from being destroyed.
+
+ We now ensure that element and document tear-down correctly removes animations and clears those maps, which
+ in turn allows the DocumentTimeline to be destroyed, fixing the significant memory leak introduced by Web Animations
+ so far.
+
+ Finally, we change the collection type for those maps to be ListHashRef instead of Vector to guarantee we only
+ add an animation once per collection due to changes in how setEffect() and setTimeline() operate.
+
+ Test: animations/leak-document-with-css-animation.html
+
+ * animation/AnimationTimeline.cpp:
+ (WebCore::AnimationTimeline::~AnimationTimeline): There is no need to clear those tables as they'll need to be empty
+ for the AnimationTimeline to even be destroyed.
+ (WebCore::AnimationTimeline::relevantMapForAnimation): Change to use ListHashRef instead of Vector.
+ (WebCore::AnimationTimeline::animationWasAddedToElement): Change to use ListHashRef instead of Vector.
+ (WebCore::AnimationTimeline::animationWasRemovedFromElement): When an animation is removed from an element, ensure that
+ references to this animation stored in the m_elementToCSSAnimationByName and m_elementToCSSTransitionByCSSPropertyID maps
+ are cleared.
+ (WebCore::AnimationTimeline::animationsForElement const): Change to use ListHashRef instead of Vector.
+ (WebCore::AnimationTimeline::removeAnimationsForElement): Instead of just calling cancel() on all known declarative animations
+ (this method used to be called cancelDeclarativeAnimationsForElement()), we now set the effect of known animations, declarative
+ or not, for the provided element which will in turn call animationWasRemovedFromElement() and remove the animation from all
+ maps that might keep a reference to it.
+ (WebCore::AnimationTimeline::updateCSSTransitionsForElement): Replace call to removeDeclarativeAnimation() with a simple call
+ to removeAnimation() which will remove references for this animation from the relevant maps.
+ (WebCore::AnimationTimeline::cancelOrRemoveDeclarativeAnimation): Ditto.
+ (WebCore::AnimationTimeline::cancelDeclarativeAnimationsForElement): Deleted.
+ (WebCore::AnimationTimeline::removeDeclarativeAnimation): Deleted.
+ * animation/AnimationTimeline.h:
+ (WebCore::AnimationTimeline::elementToAnimationsMap): Change to use ListHashRef instead of Vector.
+ (WebCore::AnimationTimeline::elementToCSSAnimationsMap): Change to use ListHashRef instead of Vector.
+ (WebCore::AnimationTimeline::elementToCSSTransitionsMap): Change to use ListHashRef instead of Vector.
+ * animation/WebAnimation.cpp:
+ (WebCore::WebAnimation::setEffect): In the case of a declarative animation, we don't want to remove the animation from the relevant
+ maps because while the effect was set via the API, the element still has a transition or animation set up and we must not break the
+ timeline-to-animation relationship.
+ (WebCore::WebAnimation::setEffectInternal): Factor parts of setEffect() out into a new method that can be called from
+ AnimationTimeline::removeAnimationsForElement() to reset the m_effect member and correctly call animationWasRemovedFromElement()
+ without all the Web Animations machinery of setEffect(), which is a public API that has unwanted side effects (such as rejecting
+ promises).
+ (WebCore::WebAnimation::setTimeline): In the case of a declarative animation, we don't want to remove the animation from the
+ relevant maps because, while the timeline was set via the API, the element still has a transition or animation set up and we must
+ not break the relationship.
+ * animation/DocumentTimeline.cpp:
+ (WebCore::DocumentTimeline::~DocumentTimeline):
+ (WebCore::DocumentTimeline::detachFromDocument): Close the GenericTaskQueues when detaching from the document as it's too late to
+ perform this work in the destructor. We also cancel the schedule timer which we had forgotten to do before.
+ * animation/WebAnimation.h:
+ * dom/Document.cpp:
+ (WebCore::Document::prepareForDestruction):
+ * dom/Element.cpp:
+ (WebCore::Element::removedFromAncestor):
+ * dom/PseudoElement.cpp:
+ (WebCore::PseudoElement::clearHostElement):
+ * rendering/updating/RenderTreeUpdater.cpp:
+ (WebCore::RenderTreeUpdater::tearDownRenderers):
+
2018-05-24 Chris Dumez <cdu...@apple.com>
Avoid doing unnecessary work in Document::shouldEnforceContentDispositionAttachmentSandbox() when setting is disabled
Modified: trunk/Source/WebCore/animation/AnimationTimeline.cpp (232184 => 232185)
--- trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/animation/AnimationTimeline.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -51,12 +51,6 @@
AnimationTimeline::~AnimationTimeline()
{
- m_animations.clear();
- m_elementToAnimationsMap.clear();
- m_elementToCSSAnimationsMap.clear();
- m_elementToCSSTransitionsMap.clear();
- m_elementToCSSAnimationByName.clear();
- m_elementToCSSTransitionByCSSPropertyID.clear();
}
void AnimationTimeline::addAnimation(Ref<WebAnimation>&& animation)
@@ -85,7 +79,7 @@
timingModelDidChange();
}
-HashMap<Element*, Vector<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapForAnimation(WebAnimation& animation)
+HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& AnimationTimeline::relevantMapForAnimation(WebAnimation& animation)
{
if (animation.isCSSAnimation())
return m_elementToCSSAnimationsMap;
@@ -96,14 +90,15 @@
void AnimationTimeline::animationWasAddedToElement(WebAnimation& animation, Element& element)
{
- auto result = relevantMapForAnimation(animation).ensure(&element, [] {
- return Vector<RefPtr<WebAnimation>> { };
- });
- result.iterator->value.append(&animation);
+ relevantMapForAnimation(animation).ensure(&element, [] {
+ return ListHashSet<RefPtr<WebAnimation>> { };
+ }).iterator->value.add(&animation);
}
void AnimationTimeline::animationWasRemovedFromElement(WebAnimation& animation, Element& element)
{
+ // First, we clear this animation from one of the m_elementToCSSAnimationsMap, m_elementToCSSTransitionsMap
+ // or m_elementToAnimationsMap map, whichever is relevant to this type of animation.
auto& map = relevantMapForAnimation(animation);
auto iterator = map.find(&element);
if (iterator == map.end())
@@ -110,28 +105,56 @@
return;
auto& animations = iterator->value;
- animations.removeFirst(&animation);
+ animations.remove(&animation);
if (!animations.size())
map.remove(iterator);
+
+ // Now, if we're dealing with a declarative animation, we remove it from either the m_elementToCSSAnimationByName
+ // or the m_elementToCSSTransitionByCSSPropertyID map, whichever is relevant to this type of animation.
+ if (is<CSSAnimation>(animation)) {
+ auto iterator = m_elementToCSSAnimationByName.find(&element);
+ if (iterator != m_elementToCSSAnimationByName.end()) {
+ auto& cssAnimationsByName = iterator->value;
+ auto& name = downcast<CSSAnimation>(animation).animationName();
+ cssAnimationsByName.remove(name);
+ if (cssAnimationsByName.isEmpty())
+ m_elementToCSSAnimationByName.remove(&element);
+ }
+ } else if (is<CSSTransition>(animation)) {
+ auto iterator = m_elementToCSSTransitionByCSSPropertyID.find(&element);
+ if (iterator != m_elementToCSSTransitionByCSSPropertyID.end()) {
+ auto& cssTransitionsByProperty = iterator->value;
+ auto property = downcast<CSSTransition>(animation).property();
+ cssTransitionsByProperty.remove(property);
+ if (cssTransitionsByProperty.isEmpty())
+ m_elementToCSSTransitionByCSSPropertyID.remove(&element);
+ }
+ }
}
Vector<RefPtr<WebAnimation>> AnimationTimeline::animationsForElement(Element& element) const
{
Vector<RefPtr<WebAnimation>> animations;
- if (m_elementToCSSAnimationsMap.contains(&element))
- animations.appendVector(m_elementToCSSAnimationsMap.get(&element));
- if (m_elementToCSSTransitionsMap.contains(&element))
- animations.appendVector(m_elementToCSSTransitionsMap.get(&element));
- if (m_elementToAnimationsMap.contains(&element))
- animations.appendVector(m_elementToAnimationsMap.get(&element));
+ if (m_elementToCSSAnimationsMap.contains(&element)) {
+ const auto& cssAnimations = m_elementToCSSAnimationsMap.get(&element);
+ animations.appendRange(cssAnimations.begin(), cssAnimations.end());
+ }
+ if (m_elementToCSSTransitionsMap.contains(&element)) {
+ const auto& cssTransitions = m_elementToCSSTransitionsMap.get(&element);
+ animations.appendRange(cssTransitions.begin(), cssTransitions.end());
+ }
+ if (m_elementToAnimationsMap.contains(&element)) {
+ const auto& webAnimations = m_elementToAnimationsMap.get(&element);
+ animations.appendRange(webAnimations.begin(), webAnimations.end());
+ }
return animations;
}
-void AnimationTimeline::cancelDeclarativeAnimationsForElement(Element& element)
+void AnimationTimeline::removeAnimationsForElement(Element& element)
{
- for (const auto& animation : animationsForElement(element)) {
- if (is<DeclarativeAnimation>(animation))
- animation->cancel();
+ for (auto& animation : animationsForElement(element)) {
+ animation->setEffectInternal(nullptr);
+ removeAnimation(animation.releaseNonNull());
}
}
@@ -289,7 +312,7 @@
if (cssTransitionsByProperty.contains(property)) {
if (cssTransitionsByProperty.get(property)->matchesBackingAnimationAndStyles(backingAnimation, oldStyle, newStyle))
continue;
- removeDeclarativeAnimation(cssTransitionsByProperty.take(property));
+ removeAnimation(cssTransitionsByProperty.take(property).releaseNonNull());
}
// Now we can create a new CSSTransition with the new backing animation provided it has a valid
// duration and the from and to values are distinct.
@@ -315,12 +338,6 @@
m_elementToCSSTransitionByCSSPropertyID.remove(&element);
}
-void AnimationTimeline::removeDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
-{
- animation->setEffect(nullptr);
- removeAnimation(animation.releaseNonNull());
-}
-
void AnimationTimeline::cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation> animation)
{
auto phase = animation->effect()->phase();
@@ -327,7 +344,7 @@
if (phase != AnimationEffectReadOnly::Phase::Idle && phase != AnimationEffectReadOnly::Phase::After)
animation->cancel();
else
- removeDeclarativeAnimation(animation);
+ removeAnimation(animation.releaseNonNull());
}
String AnimationTimeline::description()
Modified: trunk/Source/WebCore/animation/AnimationTimeline.h (232184 => 232185)
--- trunk/Source/WebCore/animation/AnimationTimeline.h 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/animation/AnimationTimeline.h 2018-05-25 13:45:15 UTC (rev 232185)
@@ -59,7 +59,7 @@
const ListHashSet<RefPtr<WebAnimation>>& animations() const { return m_animations; }
Vector<RefPtr<WebAnimation>> animationsForElement(Element&) const;
- void cancelDeclarativeAnimationsForElement(Element&);
+ void removeAnimationsForElement(Element&);
void animationWasAddedToElement(WebAnimation&, Element&);
void animationWasRemovedFromElement(WebAnimation&, Element&);
@@ -79,21 +79,20 @@
bool hasElementAnimations() const { return !m_elementToAnimationsMap.isEmpty() || !m_elementToCSSAnimationsMap.isEmpty() || !m_elementToCSSTransitionsMap.isEmpty(); }
- const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToAnimationsMap() { return m_elementToAnimationsMap; }
- const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToCSSAnimationsMap() { return m_elementToCSSAnimationsMap; }
- const HashMap<Element*, Vector<RefPtr<WebAnimation>>>& elementToCSSTransitionsMap() { return m_elementToCSSTransitionsMap; }
- void removeDeclarativeAnimation(RefPtr<DeclarativeAnimation>);
+ const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToAnimationsMap() { return m_elementToAnimationsMap; }
+ const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToCSSAnimationsMap() { return m_elementToCSSAnimationsMap; }
+ const HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& elementToCSSTransitionsMap() { return m_elementToCSSTransitionsMap; }
private:
- HashMap<Element*, Vector<RefPtr<WebAnimation>>>& relevantMapForAnimation(WebAnimation&);
+ HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>>& relevantMapForAnimation(WebAnimation&);
void cancelOrRemoveDeclarativeAnimation(RefPtr<DeclarativeAnimation>);
RefPtr<WebAnimation> cssAnimationForElementAndProperty(Element&, CSSPropertyID);
ClassType m_classType;
std::optional<Seconds> m_currentTime;
- HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
- HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
- HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
+ HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
+ HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
+ HashMap<Element*, ListHashSet<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
ListHashSet<RefPtr<WebAnimation>> m_animations;
HashMap<Element*, HashMap<String, RefPtr<CSSAnimation>>> m_elementToCSSAnimationByName;
Modified: trunk/Source/WebCore/animation/DocumentTimeline.cpp (232184 => 232185)
--- trunk/Source/WebCore/animation/DocumentTimeline.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/animation/DocumentTimeline.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -61,12 +61,13 @@
DocumentTimeline::~DocumentTimeline()
{
- m_invalidationTaskQueue.close();
- m_eventDispatchTaskQueue.close();
}
void DocumentTimeline::detachFromDocument()
{
+ m_invalidationTaskQueue.close();
+ m_eventDispatchTaskQueue.close();
+ m_animationScheduleTimer.stop();
m_document = nullptr;
}
Modified: trunk/Source/WebCore/animation/WebAnimation.cpp (232184 => 232185)
--- trunk/Source/WebCore/animation/WebAnimation.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/animation/WebAnimation.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -119,30 +119,44 @@
newEffect->animation()->setEffect(nullptr);
// 7. Let the target effect of animation be new effect.
- m_effect = WTFMove(newEffect);
+ // In the case of a declarative animation, we don't want to remove the animation from the relevant maps because
+ // while the effect was set via the API, the element still has a transition or animation set up and we must
+ // not break the timeline-to-animation relationship.
+ setEffectInternal(WTFMove(newEffect), isDeclarativeAnimation());
// 8. Run the procedure to update an animation’s finished state for animation with the did seek flag set to false,
// and the synchronously notify flag set to false.
updateFinishedState(DidSeek::No, SynchronouslyNotify::No);
+ timingModelDidChange();
+}
+
+void WebAnimation::setEffectInternal(RefPtr<AnimationEffectReadOnly>&& newEffect, bool doNotRemoveFromTimeline)
+{
+ auto oldEffect = m_effect;
+
+ m_effect = WTFMove(newEffect);
+
+ Element* previousTarget = nullptr;
+ if (is<KeyframeEffectReadOnly>(oldEffect))
+ previousTarget = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target();
+
+ Element* newTarget = nullptr;
+ if (is<KeyframeEffectReadOnly>(m_effect))
+ newTarget = downcast<KeyframeEffectReadOnly>(m_effect.get())->target();
+
// Update the effect-to-animation relationships and the timeline's animation map.
if (oldEffect) {
oldEffect->setAnimation(nullptr);
- if (m_timeline && is<KeyframeEffectReadOnly>(oldEffect)) {
- if (auto* target = downcast<KeyframeEffectReadOnly>(oldEffect.get())->target())
- m_timeline->animationWasRemovedFromElement(*this, *target);
- }
+ if (!doNotRemoveFromTimeline && m_timeline && previousTarget && previousTarget != newTarget)
+ m_timeline->animationWasRemovedFromElement(*this, *previousTarget);
}
if (m_effect) {
m_effect->setAnimation(this);
- if (m_timeline && is<KeyframeEffectReadOnly>(m_effect)) {
- if (auto* target = downcast<KeyframeEffectReadOnly>(m_effect.get())->target())
- m_timeline->animationWasAddedToElement(*this, *target);
- }
+ if (m_timeline && newTarget && previousTarget != newTarget)
+ m_timeline->animationWasAddedToElement(*this, *newTarget);
}
-
- timingModelDidChange();
}
void WebAnimation::setTimeline(RefPtr<AnimationTimeline>&& timeline)
@@ -168,7 +182,10 @@
auto* keyframeEffect = downcast<KeyframeEffectReadOnly>(m_effect.get());
auto* target = keyframeEffect->target();
if (target) {
- if (m_timeline)
+ // In the case of a declarative animation, we don't want to remove the animation from the relevant maps because
+ // while the timeline was set via the API, the element still has a transition or animation set up and we must
+ // not break the relationship.
+ if (m_timeline && !isDeclarativeAnimation())
m_timeline->animationWasRemovedFromElement(*this, *target);
if (timeline)
timeline->animationWasAddedToElement(*this, *target);
Modified: trunk/Source/WebCore/animation/WebAnimation.h (232184 => 232185)
--- trunk/Source/WebCore/animation/WebAnimation.h 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/animation/WebAnimation.h 2018-05-25 13:45:15 UTC (rev 232185)
@@ -63,6 +63,7 @@
AnimationEffectReadOnly* effect() const { return m_effect.get(); }
void setEffect(RefPtr<AnimationEffectReadOnly>&&);
+ void setEffectInternal(RefPtr<AnimationEffectReadOnly>&&, bool = false);
AnimationTimeline* timeline() const { return m_timeline.get(); }
virtual void setTimeline(RefPtr<AnimationTimeline>&&);
Modified: trunk/Source/WebCore/dom/Document.cpp (232184 => 232185)
--- trunk/Source/WebCore/dom/Document.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/dom/Document.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -2336,11 +2336,6 @@
if (m_hasPreparedForDestruction)
return;
- if (m_timeline) {
- m_timeline->detachFromDocument();
- m_timeline = nullptr;
- }
-
if (m_frame)
m_frame->animation().detachFromDocument(this);
@@ -2433,6 +2428,11 @@
detachFromFrame();
+ if (m_timeline) {
+ m_timeline->detachFromDocument();
+ m_timeline = nullptr;
+ }
+
m_hasPreparedForDestruction = true;
// Note that m_pageCacheState can be Document::AboutToEnterPageCache if our frame
Modified: trunk/Source/WebCore/dom/Element.cpp (232184 => 232185)
--- trunk/Source/WebCore/dom/Element.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/dom/Element.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -1797,7 +1797,7 @@
RefPtr<Frame> frame = document().frame();
if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
if (auto* timeline = document().existingTimeline())
- timeline->cancelDeclarativeAnimationsForElement(*this);
+ timeline->removeAnimationsForElement(*this);
} else if (frame)
frame->animation().cancelAnimations(*this);
Modified: trunk/Source/WebCore/dom/PseudoElement.cpp (232184 => 232185)
--- trunk/Source/WebCore/dom/PseudoElement.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/dom/PseudoElement.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -92,7 +92,7 @@
if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
if (auto* timeline = document().existingTimeline())
- timeline->cancelDeclarativeAnimationsForElement(*this);
+ timeline->removeAnimationsForElement(*this);
} else if (auto* frame = document().frame())
frame->animation().cancelAnimations(*this);
Modified: trunk/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp (232184 => 232185)
--- trunk/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp 2018-05-25 05:09:33 UTC (rev 232184)
+++ trunk/Source/WebCore/rendering/updating/RenderTreeUpdater.cpp 2018-05-25 13:45:15 UTC (rev 232185)
@@ -555,7 +555,7 @@
if (teardownType == TeardownType::Full || teardownType == TeardownType::RendererUpdateCancelingAnimations) {
if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
if (timeline)
- timeline->cancelDeclarativeAnimationsForElement(element);
+ timeline->removeAnimationsForElement(element);
} else
animationController.cancelAnimations(element);
}
