Changeset 177238 in webkit


Ignore:
Timestamp:
Dec 12, 2014 2:52:36 PM (9 years ago)
Author:
Simon Fraser
Message:

REGRESSION (r168217): Images are cropped out during animation at jetblue.com
​https://bugs.webkit.org/show_bug.cgi?id=136410
rdar://problem/18188533

Reviewed by Dean Jackson.

During GraphicsLayer flushing, for tiled layers we can compute a visible rect using
the current state of an animation, which is obtained via the AnimationController.
If that animation was running in a subframe, AnimationController could use a stale
beginAnimationUpdateTime since no-one called its beginAnimationUpdate(). That
resulted in an incorrect computation of the visible rect, resulting in missing tiles.

There are two parts to this fix. First, add an assertion that beginAnimationUpdateTime()
is being called inside an animation update block. This required moving m_beginAnimationUpdateCount
into AnimationControllerPrivate, and changes to endAnimationUpdate().

The second is adding a AnimationUpdateBlock to getAnimatedStyleForRenderer(), which
can be called outside of style resolution. We also need some in other API functions.

Testing revealed that layout can call via layoutOverflowRectForPropagation(), suggesting
that we should have an animation batch inside FrameView::layout(). In addition, a single
resolveStyle/layout should use the same animationBeginTime, so we add a batch to
updateLayoutAndStyleIfNeededRecursive().

No test because it's timing-dependent. Existing tests exercise the new assertion.

  • css/CSSComputedStyleDeclaration.cpp:

(WebCore::computeRenderStyleForProperty):

  • page/FrameView.cpp:

(WebCore::FrameView::layout):
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):

  • page/animation/AnimationController.cpp:

(WebCore::AnimationPrivateUpdateBlock::AnimationPrivateUpdateBlock):
(WebCore::AnimationPrivateUpdateBlock::~AnimationPrivateUpdateBlock):
(WebCore::AnimationControllerPrivate::AnimationControllerPrivate):
(WebCore::AnimationControllerPrivate::animationTimerFired):
(WebCore::AnimationControllerPrivate::suspendAnimationsForDocument):
(WebCore::AnimationControllerPrivate::resumeAnimationsForDocument):
(WebCore::AnimationControllerPrivate::beginAnimationUpdateTime):
(WebCore::AnimationControllerPrivate::beginAnimationUpdate):
(WebCore::AnimationControllerPrivate::endAnimationUpdate):
(WebCore::AnimationControllerPrivate::getAnimatedStyleForRenderer):
(WebCore::AnimationController::AnimationController):
(WebCore::AnimationController::notifyAnimationStarted):
(WebCore::AnimationController::pauseAnimationAtTime):
(WebCore::AnimationController::pauseTransitionAtTime):
(WebCore::AnimationController::resumeAnimationsForDocument):
(WebCore::AnimationController::startAnimationsIfNotSuspended):
(WebCore::AnimationController::beginAnimationUpdate):
(WebCore::AnimationController::endAnimationUpdate):

  • page/animation/AnimationController.h:
  • page/animation/AnimationControllerPrivate.h:
Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r177236 r177238  
     12014-12-12  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r168217): Images are cropped out during animation at jetblue.com
     4        https://bugs.webkit.org/show_bug.cgi?id=136410
     5        rdar://problem/18188533
     6
     7        Reviewed by Dean Jackson.
     8       
     9        During GraphicsLayer flushing, for tiled layers we can compute a visible rect using
     10        the current state of an animation, which is obtained via the AnimationController.
     11        If that animation was running in a subframe, AnimationController could use a stale
     12        beginAnimationUpdateTime since no-one called its beginAnimationUpdate(). That
     13        resulted in an incorrect computation of the visible rect, resulting in missing tiles.
     14       
     15        There are two parts to this fix. First, add an assertion that beginAnimationUpdateTime()
     16        is being called inside an animation update block. This required moving m_beginAnimationUpdateCount
     17        into AnimationControllerPrivate, and changes to endAnimationUpdate().
     18       
     19        The second is adding a AnimationUpdateBlock to getAnimatedStyleForRenderer(), which
     20        can be called outside of style resolution. We also need some in other API functions.
     21       
     22        Testing revealed that layout can call via layoutOverflowRectForPropagation(), suggesting
     23        that we should have an animation batch inside FrameView::layout(). In addition, a single
     24        resolveStyle/layout should use the same animationBeginTime, so we add a batch to
     25        updateLayoutAndStyleIfNeededRecursive().
     26       
     27        No test because it's timing-dependent. Existing tests exercise the new assertion.
     28
     29        * css/CSSComputedStyleDeclaration.cpp:
     30        (WebCore::computeRenderStyleForProperty):
     31        * page/FrameView.cpp:
     32        (WebCore::FrameView::layout):
     33        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
     34        * page/animation/AnimationController.cpp:
     35        (WebCore::AnimationPrivateUpdateBlock::AnimationPrivateUpdateBlock):
     36        (WebCore::AnimationPrivateUpdateBlock::~AnimationPrivateUpdateBlock):
     37        (WebCore::AnimationControllerPrivate::AnimationControllerPrivate):
     38        (WebCore::AnimationControllerPrivate::animationTimerFired):
     39        (WebCore::AnimationControllerPrivate::suspendAnimationsForDocument):
     40        (WebCore::AnimationControllerPrivate::resumeAnimationsForDocument):
     41        (WebCore::AnimationControllerPrivate::beginAnimationUpdateTime):
     42        (WebCore::AnimationControllerPrivate::beginAnimationUpdate):
     43        (WebCore::AnimationControllerPrivate::endAnimationUpdate):
     44        (WebCore::AnimationControllerPrivate::getAnimatedStyleForRenderer):
     45        (WebCore::AnimationController::AnimationController):
     46        (WebCore::AnimationController::notifyAnimationStarted):
     47        (WebCore::AnimationController::pauseAnimationAtTime):
     48        (WebCore::AnimationController::pauseTransitionAtTime):
     49        (WebCore::AnimationController::resumeAnimationsForDocument):
     50        (WebCore::AnimationController::startAnimationsIfNotSuspended):
     51        (WebCore::AnimationController::beginAnimationUpdate):
     52        (WebCore::AnimationController::endAnimationUpdate):
     53        * page/animation/AnimationController.h:
     54        * page/animation/AnimationControllerPrivate.h:
     55
    1562014-12-12  Roger Fong  <roger_fong@apple.com>
    257

  • trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

    r177223 r177238  
    17061706
    17071707    if (renderer && renderer->isComposited() && AnimationController::supportsAcceleratedAnimationOfProperty(propertyID)) {
    1708         AnimationUpdateBlock animationUpdateBlock(&renderer->animation());
    17091708        RefPtr<RenderStyle> style = renderer->animation().getAnimatedStyleForRenderer(downcast<RenderElement>(*renderer));
    17101709        if (pseudoElementSpecifier && !styledNode->isPseudoElement()) {

  • trunk/Source/WebCore/page/FrameView.cpp

    r177223 r177238  
    11541154
    11551155    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willLayout(&frame());
    1156 
     1156    AnimationUpdateBlock animationUpdateBlock(&frame().animation());
     1157   
    11571158    if (!allowSubtree && m_layoutRoot) {
    11581159        m_layoutRoot->markContainingBlocksForLayout(false);
    … …  
    39383939    // when it lays out.
    39393940
     3941    AnimationUpdateBlock animationUpdateBlock(&frame().animation());
     3942
    39403943    frame().document()->updateStyleIfNeeded();
    39413944

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

    r176459 r177238  
    5252static const double cBeginAnimationUpdateTimeNotSet = -1;
    5353
     54class AnimationPrivateUpdateBlock {
     55public:
     56    AnimationPrivateUpdateBlock(AnimationControllerPrivate& animationController)
     57        : m_animationController(animationController)
     58    {
     59        m_animationController.beginAnimationUpdate();
     60    }
     61   
     62    ~AnimationPrivateUpdateBlock()
     63    {
     64        m_animationController.endAnimationUpdate();
     65    }
     66   
     67    AnimationControllerPrivate& m_animationController;
     68};
     69
    5470AnimationControllerPrivate::AnimationControllerPrivate(Frame& frame)
    5571    : m_animationTimer(*this, &AnimationControllerPrivate::animationTimerFired)
    … …  
    5975    , m_animationsWaitingForStyle()
    6076    , m_animationsWaitingForStartTimeResponse()
     77    , m_beginAnimationUpdateCount(0)
    6178    , m_waitingForAsyncStartNotification(false)
    6279    , m_isSuspended(false)
    … …  
    231248    // Make sure animationUpdateTime is updated, so that it is current even if no
    232249    // styleChange has happened (e.g. accelerated animations)
    233     setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     250    AnimationPrivateUpdateBlock updateBlock(*this);
    234251
    235252    // When the timer fires, all we do is call setChanged on all DOM nodes with running animations and then do an immediate
    … …  
    288305void AnimationControllerPrivate::suspendAnimationsForDocument(Document* document)
    289306{
    290     setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     307    AnimationPrivateUpdateBlock updateBlock(*this);
    291308
    292309    for (auto it = m_compositeAnimations.begin(), end = m_compositeAnimations.end(); it != end; ++it) {
    … …  
    300317void AnimationControllerPrivate::resumeAnimationsForDocument(Document* document)
    301318{
    302     setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     319    AnimationPrivateUpdateBlock updateBlock(*this);
    303320
    304321    for (auto it = m_compositeAnimations.begin(), end = m_compositeAnimations.end(); it != end; ++it) {
    … …  
    353370double AnimationControllerPrivate::beginAnimationUpdateTime()
    354371{
     372    ASSERT(m_beginAnimationUpdateCount);
    355373    if (m_beginAnimationUpdateTime == cBeginAnimationUpdateTimeNotSet)
    356374        m_beginAnimationUpdateTime = monotonicallyIncreasingTime();
     375
    357376    return m_beginAnimationUpdateTime;
    358377}
    359378
     379void AnimationControllerPrivate::beginAnimationUpdate()
     380{
     381    if (!m_beginAnimationUpdateCount)
     382        setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     383    ++m_beginAnimationUpdateCount;
     384}
     385
    360386void AnimationControllerPrivate::endAnimationUpdate()
    361387{
    362     styleAvailable();
    363     if (!m_waitingForAsyncStartNotification)
    364         startTimeResponse(beginAnimationUpdateTime());
     388    ASSERT(m_beginAnimationUpdateCount > 0);
     389    if (m_beginAnimationUpdateCount == 1) {
     390        styleAvailable();
     391        if (!m_waitingForAsyncStartNotification)
     392            startTimeResponse(beginAnimationUpdateTime());
     393    }
     394    --m_beginAnimationUpdateCount;
    365395}
    366396
    … …  
    373403PassRefPtr<RenderStyle> AnimationControllerPrivate::getAnimatedStyleForRenderer(RenderElement& renderer)
    374404{
     405    AnimationPrivateUpdateBlock animationUpdateBlock(*this);
     406
    375407    ASSERT(renderer.isCSSAnimating());
    376408    ASSERT(m_compositeAnimations.contains(&renderer));
    … …  
    470502AnimationController::AnimationController(Frame& frame)
    471503    : m_data(std::make_unique<AnimationControllerPrivate>(frame))
    472     , m_beginAnimationUpdateCount(0)
    473504{
    474505}
    … …  
    546577void AnimationController::notifyAnimationStarted(RenderElement&, double startTime)
    547578{
     579    AnimationUpdateBlock animationUpdateBlock(this);
    548580    m_data->receivedStartTimeResponse(startTime);
    549581}
    … …  
    551583bool AnimationController::pauseAnimationAtTime(RenderElement* renderer, const AtomicString& name, double t)
    552584{
     585    AnimationUpdateBlock animationUpdateBlock(this);
    553586    return m_data->pauseAnimationAtTime(renderer, name, t);
    554587}
    … …  
    561594bool AnimationController::pauseTransitionAtTime(RenderElement* renderer, const String& property, double t)
    562595{
     596    AnimationUpdateBlock animationUpdateBlock(this);
    563597    return m_data->pauseTransitionAtTime(renderer, property, t);
    564598}
    … …  
    617651{
    618652    LOG(Animations, "resuming animations for document %p", document);
     653    AnimationUpdateBlock animationUpdateBlock(this);
    619654    m_data->resumeAnimationsForDocument(document);
    620655}
    … …  
    623658{
    624659    LOG(Animations, "animations may start for document %p", document);
     660
     661    AnimationUpdateBlock animationUpdateBlock(this);
    625662    m_data->startAnimationsIfNotSuspended(document);
    626663}
    … …  
    628665void AnimationController::beginAnimationUpdate()
    629666{
    630     if (!m_beginAnimationUpdateCount)
    631         m_data->setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
    632     ++m_beginAnimationUpdateCount;
     667    m_data->beginAnimationUpdate();
    633668}
    634669
    635670void AnimationController::endAnimationUpdate()
    636671{
    637     ASSERT(m_beginAnimationUpdateCount > 0);
    638     --m_beginAnimationUpdateCount;
    639     if (!m_beginAnimationUpdateCount)
    640         m_data->endAnimationUpdate();
     672    m_data->endAnimationUpdate();
    641673}
    642674

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

    r174804 r177238  
    8383private:
    8484    const std::unique_ptr<AnimationControllerPrivate> m_data;
    85     int m_beginAnimationUpdateCount;
    8685};
    8786

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

    r176459 r177238  
    9898    double beginAnimationUpdateTime();
    9999    void setBeginAnimationUpdateTime(double t) { m_beginAnimationUpdateTime = t; }
     100   
     101    void beginAnimationUpdate();
    100102    void endAnimationUpdate();
    101103    void receivedStartTimeResponse(double);
    … …  
    142144    WaitingAnimationsSet m_animationsWaitingForStyle;
    143145    WaitingAnimationsSet m_animationsWaitingForStartTimeResponse;
     146
     147    int m_beginAnimationUpdateCount;
     148
    144149    bool m_waitingForAsyncStartNotification;
    145150    bool m_isSuspended;
Note: See TracChangeset for help on using the changeset viewer.