Changeset 187535 in webkit


Ignore:
Timestamp:
Jul 28, 2015 6:57:30 PM (9 years ago)
Author:
Simon Fraser
Message:

Animations sometimes fail to start
​https://bugs.webkit.org/show_bug.cgi?id=147394
rdar://problem/21852603

Reviewed by Dean Jackson.
Source/WebCore:

When an accelerated animation or transition was started at the same time as
a non-accelerated one, and then the node for the former was removed, we could
never kick off the non-accelerated animation.

AnimationControllerPrivate has logic to synchronize the two types of animation
when they start in the same animation update, which involves setting the
m_waitingForAsyncStartNotification flag, and waiting for a notifyAnimationStarted()
to come in from the graphics system.

However, it failed to handle the case where the accelerated animation was removed
before the callback was received, which left the m_waitingForAsyncStartNotification flag
set to true, preventing the non-accelerated animation from running.

Test: animations/remove-syncing-animation.html

  • page/animation/AnimationBase.h:

(WebCore::AnimationBase::isAccelerated): Make this public.

  • page/animation/AnimationController.cpp:

(WebCore::AnimationControllerPrivate::clear): Add logging.
(WebCore::AnimationControllerPrivate::receivedStartTimeResponse): Add logging.
(WebCore::AnimationControllerPrivate::animationWillBeRemoved): Add logging.
After removing animations from the maps, check to see if we expect any of the
remaining animations are waiting for a notifyAnimationStarted(). If not, clear
the m_waitingForAsyncStartNotification flag.
(WebCore::AnimationController::notifyAnimationStarted): Log the renderer.
(WebCore::AnimationControllerPrivate::AnimationControllerPrivate): Remove unneeded
initializations of HashMaps.

  • page/animation/CompositeAnimation.cpp:

(WebCore::CompositeAnimation::updateTransitions): Log renderers.
(WebCore::CompositeAnimation::updateKeyframeAnimations): Ditto.

LayoutTests:

Test that starts an accelerated and non-accelerated animation, then removes
the node for the accelerated one.

  • animations/remove-syncing-animation-expected.txt: Added.
  • animations/remove-syncing-animation.html: Added.
Location:
trunk
Files:
2 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r187527 r187535  
     12015-07-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Animations sometimes fail to start
     4        https://bugs.webkit.org/show_bug.cgi?id=147394
     5        rdar://problem/21852603
     6
     7        Reviewed by Dean Jackson.
     8       
     9        Test that starts an accelerated and non-accelerated animation, then removes
     10        the node for the accelerated one.
     11
     12        * animations/remove-syncing-animation-expected.txt: Added.
     13        * animations/remove-syncing-animation.html: Added.
     14
    1152015-07-28  Michael Catanzaro  <mcatanzaro@igalia.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r187534 r187535  
     12015-07-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Animations sometimes fail to start
     4        https://bugs.webkit.org/show_bug.cgi?id=147394
     5        rdar://problem/21852603
     6
     7        Reviewed by Dean Jackson.
     8
     9        When an accelerated animation or transition was started at the same time as
     10        a non-accelerated one, and then the node for the former was removed, we could
     11        never kick off the non-accelerated animation.
     12       
     13        AnimationControllerPrivate has logic to synchronize the two types of animation
     14        when they start in the same animation update, which involves setting the
     15        m_waitingForAsyncStartNotification flag, and waiting for a notifyAnimationStarted()
     16        to come in from the graphics system.
     17       
     18        However, it failed to handle the case where the accelerated animation was removed
     19        before the callback was received, which left the m_waitingForAsyncStartNotification flag
     20        set to true, preventing the non-accelerated animation from running.
     21       
     22        Test: animations/remove-syncing-animation.html
     23
     24        * page/animation/AnimationBase.h:
     25        (WebCore::AnimationBase::isAccelerated): Make this public.
     26        * page/animation/AnimationController.cpp:
     27        (WebCore::AnimationControllerPrivate::clear): Add logging.
     28        (WebCore::AnimationControllerPrivate::receivedStartTimeResponse): Add logging.
     29        (WebCore::AnimationControllerPrivate::animationWillBeRemoved): Add logging.
     30        After removing animations from the maps, check to see if we expect any of the
     31        remaining animations are waiting for a notifyAnimationStarted(). If not, clear
     32        the m_waitingForAsyncStartNotification flag.
     33        (WebCore::AnimationController::notifyAnimationStarted): Log the renderer.
     34        (WebCore::AnimationControllerPrivate::AnimationControllerPrivate): Remove unneeded
     35        initializations of HashMaps.
     36        * page/animation/CompositeAnimation.cpp:
     37        (WebCore::CompositeAnimation::updateTransitions): Log renderers.
     38        (WebCore::CompositeAnimation::updateKeyframeAnimations): Ditto.
     39
    1402015-07-28  Dean Jackson  <dino@apple.com>
    241

  • trunk/Source/WebCore/page/animation/AnimationBase.h

    r187461 r187535  
    132132    bool waitingForStyleAvailable() const { return m_animationState == AnimationState::StartWaitStyleAvailable; }
    133133
     134    bool isAccelerated() const { return m_isAccelerated; }
     135
    134136    virtual double timeToNextService();
    135137
    … …  
    231233    void goIntoEndingOrLoopingState();
    232234
    233     bool isAccelerated() const { return m_isAccelerated; }
    234235    AnimationState state() const { return m_animationState; }
    235236

  • trunk/Source/WebCore/page/animation/AnimationController.cpp

    r185232 r187535  
    7373    , m_frame(frame)
    7474    , m_beginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet)
    75     , m_animationsWaitingForStyle()
    76     , m_animationsWaitingForStartTimeResponse()
    7775    , m_beginAnimationUpdateCount(0)
    7876    , m_waitingForAsyncStartNotification(false)
    … …  
    9997bool AnimationControllerPrivate::clear(RenderElement& renderer)
    10098{
     99    LOG(Animations, "AnimationControllerPrivate %p clear: %p", this, &renderer);
     100
    101101    ASSERT(renderer.isCSSAnimating());
    102102    ASSERT(m_compositeAnimations.contains(&renderer));
    … …  
    411411void AnimationControllerPrivate::receivedStartTimeResponse(double time)
    412412{
     413    LOG(Animations, "AnimationControllerPrivate %p receivedStartTimeResponse %f", this, time);
     414
    413415    m_waitingForAsyncStartNotification = false;
    414416    startTimeResponse(time);
    … …  
    522524void AnimationControllerPrivate::animationWillBeRemoved(AnimationBase* animation)
    523525{
     526    LOG(Animations, "AnimationControllerPrivate %p animationWillBeRemoved: %p", this, animation);
     527
    524528    removeFromAnimationsWaitingForStyle(animation);
    525529    removeFromAnimationsWaitingForStartTimeResponse(animation);
    … …  
    527531    removeFromAnimationsDependentOnScroll(animation);
    528532#endif
     533
     534    bool anyAnimationsWaitingForAsyncStart = false;
     535    for (auto& animation : m_animationsWaitingForStartTimeResponse) {
     536        if (animation->waitingForStartTime() && animation->isAccelerated()) {
     537            anyAnimationsWaitingForAsyncStart = true;
     538            break;
     539        }
     540    }
     541
     542    if (!anyAnimationsWaitingForAsyncStart)
     543        m_waitingForAsyncStartNotification = false;
    529544}
    530545
    … …  
    634649}
    635650
    636 void AnimationController::notifyAnimationStarted(RenderElement&, double startTime)
    637 {
     651void AnimationController::notifyAnimationStarted(RenderElement& renderer, double startTime)
     652{
     653    LOG(Animations, "AnimationController %p notifyAnimationStarted on renderer %p, time=%f", this, &renderer, startTime);
     654    UNUSED_PARAM(renderer);
     655
    638656    AnimationUpdateBlock animationUpdateBlock(this);
    639657    m_data->receivedStartTimeResponse(startTime);

  • trunk/Source/WebCore/page/animation/CompositeAnimation.cpp

    r183454 r187535  
    169169                    // Add the new transition
    170170                    RefPtr<ImplicitAnimation> implicitAnimation = ImplicitAnimation::create(animation, prop, renderer, this, modifiedCurrentStyle ? modifiedCurrentStyle.get() : fromStyle);
    171                     LOG(Animations, "Created ImplicitAnimation %p for property %s duration %.2f delay %.2f", implicitAnimation.get(), getPropertyName(prop), animation.duration(), animation.delay());
     171                    LOG(Animations, "Created ImplicitAnimation %p on renderer %p for property %s duration %.2f delay %.2f", implicitAnimation.get(), renderer, getPropertyName(prop), animation.duration(), animation.delay());
    172172                    m_transitions.set(prop, implicitAnimation.release());
    173173                }
    … …  
    186186            animationController().animationWillBeRemoved(transition.get());
    187187            toBeRemoved.append(transition->animatingProperty());
    188             LOG(Animations, "Removing ImplicitAnimation %p for property %s", transition.get(), getPropertyName(transition->animatingProperty()));
     188            LOG(Animations, "Removing ImplicitAnimation %p from renderer %p for property %s", transition.get(), renderer, getPropertyName(transition->animatingProperty()));
    189189        }
    190190    }
    … …  
    257257                } else if ((animation.duration() || animation.delay()) && animation.iterationCount() && animationName != none) {
    258258                    keyframeAnim = KeyframeAnimation::create(animation, renderer, i, this, targetStyle);
    259                     LOG(Animations, "Creating KeyframeAnimation %p with keyframes %s, duration %.2f, delay %.2f, iterations %.2f", keyframeAnim.get(), animation.name().utf8().data(), animation.duration(), animation.delay(), animation.iterationCount());
     259                    LOG(Animations, "Creating KeyframeAnimation %p on renderer %p with keyframes %s, duration %.2f, delay %.2f, iterations %.2f", keyframeAnim.get(), renderer, animation.name().utf8().data(), animation.duration(), animation.delay(), animation.iterationCount());
    260260                    if (m_suspended) {
    261261                        keyframeAnim->updatePlayState(AnimPlayStatePaused);
    … …  
    289289            animationController().animationWillBeRemoved(animation.get());
    290290            animation->clear();
    291             LOG(Animations, "Removing KeyframeAnimation %p", animation.get());
     291            LOG(Animations, "Removing KeyframeAnimation %p from renderer %p", animation.get(), renderer);
    292292        }
    293293    }
Note: See TracChangeset for help on using the changeset viewer.