Changeset 177302 in webkit


Ignore:
Timestamp:
Dec 15, 2014 12:24:45 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:
7 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r177301 r177302  
     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        Identical to the patch that was rolled out in r177269 with the addition of a
     28        Ref<Frame> protector(m_frame) in AnimationControllerPrivate::animationTimerFired()
     29        that ensures that the AnimationControllerPrivate is kept alive for the scope of
     30        the AnimationPrivateUpdateBlock, when a transitionEnd event destroys an iframe.
     31       
     32        No test because it's timing-dependent. Existing tests exercise the new assertion.
     33
     34        * css/CSSComputedStyleDeclaration.cpp:
     35        (WebCore::computeRenderStyleForProperty):
     36        * page/FrameView.cpp:
     37        (WebCore::FrameView::layout):
     38        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
     39        * page/animation/AnimationController.cpp:
     40        (WebCore::AnimationPrivateUpdateBlock::AnimationPrivateUpdateBlock):
     41        (WebCore::AnimationPrivateUpdateBlock::~AnimationPrivateUpdateBlock):
     42        (WebCore::AnimationControllerPrivate::AnimationControllerPrivate):
     43        (WebCore::AnimationControllerPrivate::animationTimerFired):
     44        (WebCore::AnimationControllerPrivate::suspendAnimationsForDocument):
     45        (WebCore::AnimationControllerPrivate::resumeAnimationsForDocument):
     46        (WebCore::AnimationControllerPrivate::beginAnimationUpdateTime):
     47        (WebCore::AnimationControllerPrivate::beginAnimationUpdate):
     48        (WebCore::AnimationControllerPrivate::endAnimationUpdate):
     49        (WebCore::AnimationControllerPrivate::getAnimatedStyleForRenderer):
     50        (WebCore::AnimationController::AnimationController):
     51        (WebCore::AnimationController::notifyAnimationStarted):
     52        (WebCore::AnimationController::pauseAnimationAtTime):
     53        (WebCore::AnimationController::pauseTransitionAtTime):
     54        (WebCore::AnimationController::resumeAnimationsForDocument):
     55        (WebCore::AnimationController::startAnimationsIfNotSuspended):
     56        (WebCore::AnimationController::beginAnimationUpdate):
     57        (WebCore::AnimationController::endAnimationUpdate):
     58        * page/animation/AnimationController.h:
     59        * page/animation/AnimationControllerPrivate.h:
     60
     612014-12-12  Simon Fraser  <simon.fraser@apple.com>
     62
     63        REGRESSION (r168217): Images are cropped out during animation at jetblue.com
     64        https://bugs.webkit.org/show_bug.cgi?id=136410
     65
     66        Reviewed by Dean Jackson.
     67
     68        We were hitting the new assertion under Page::setPageScaleFactor(), which
     69        calls recalcStyle(), so move the AnimationUpdateBlock from updateStyleIfNeeded()
     70        to recalcStyle().
     71
     72        * dom/Document.cpp:
     73        (WebCore::Document::recalcStyle):
     74        (WebCore::Document::updateStyleIfNeeded):
     75
    1762014-12-15  Myles C. Maxfield  <mmaxfield@apple.com>
    277
  • trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

    r177269 r177302  
    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/dom/Document.cpp

    r177297 r177302  
    17641764
    17651765    RenderView::RepaintRegionAccumulator repaintRegionAccumulator(renderView());
     1766    AnimationUpdateBlock animationUpdateBlock(&m_frame->animation());
    17661767
    17671768    // FIXME: We should update style on our ancestor chain before proceeding (especially for seamless),
     
    18361837        return;
    18371838
    1838     AnimationUpdateBlock animationUpdateBlock(m_frame ? &m_frame->animation() : nullptr);
    18391839    recalcStyle(Style::NoChange);
    18401840}
  • trunk/Source/WebCore/page/FrameView.cpp

    r177301 r177302  
    11541154
    11551155    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willLayout(&frame());
    1156 
     1156    AnimationUpdateBlock animationUpdateBlock(&frame().animation());
     1157   
    11571158    if (!allowSubtree && m_layoutRoot) {
    11581159        m_layoutRoot->markContainingBlocksForLayout(false);
     
    39413942    // when it lays out.
    39423943
     3944    AnimationUpdateBlock animationUpdateBlock(&frame().animation());
     3945
    39433946    frame().document()->updateStyleIfNeeded();
    39443947
  • trunk/Source/WebCore/page/animation/AnimationController.cpp

    r177269 r177302  
    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)
     
    229246void AnimationControllerPrivate::animationTimerFired()
    230247{
     248    // We need to keep the frame alive, since it owns us.
     249    Ref<Frame> protector(m_frame);
     250
    231251    // Make sure animationUpdateTime is updated, so that it is current even if no
    232252    // styleChange has happened (e.g. accelerated animations)
    233     setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     253    AnimationPrivateUpdateBlock updateBlock(*this);
    234254
    235255    // When the timer fires, all we do is call setChanged on all DOM nodes with running animations and then do an immediate
     
    288308void AnimationControllerPrivate::suspendAnimationsForDocument(Document* document)
    289309{
    290     setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     310    AnimationPrivateUpdateBlock updateBlock(*this);
    291311
    292312    for (auto it = m_compositeAnimations.begin(), end = m_compositeAnimations.end(); it != end; ++it) {
     
    300320void AnimationControllerPrivate::resumeAnimationsForDocument(Document* document)
    301321{
    302     setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     322    AnimationPrivateUpdateBlock updateBlock(*this);
    303323
    304324    for (auto it = m_compositeAnimations.begin(), end = m_compositeAnimations.end(); it != end; ++it) {
     
    353373double AnimationControllerPrivate::beginAnimationUpdateTime()
    354374{
     375    ASSERT(m_beginAnimationUpdateCount);
    355376    if (m_beginAnimationUpdateTime == cBeginAnimationUpdateTimeNotSet)
    356377        m_beginAnimationUpdateTime = monotonicallyIncreasingTime();
     378
    357379    return m_beginAnimationUpdateTime;
    358380}
    359381
     382void AnimationControllerPrivate::beginAnimationUpdate()
     383{
     384    if (!m_beginAnimationUpdateCount)
     385        setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
     386    ++m_beginAnimationUpdateCount;
     387}
     388
    360389void AnimationControllerPrivate::endAnimationUpdate()
    361390{
    362     styleAvailable();
    363     if (!m_waitingForAsyncStartNotification)
    364         startTimeResponse(beginAnimationUpdateTime());
     391    ASSERT(m_beginAnimationUpdateCount > 0);
     392    if (m_beginAnimationUpdateCount == 1) {
     393        styleAvailable();
     394        if (!m_waitingForAsyncStartNotification)
     395            startTimeResponse(beginAnimationUpdateTime());
     396    }
     397    --m_beginAnimationUpdateCount;
    365398}
    366399
     
    373406PassRefPtr<RenderStyle> AnimationControllerPrivate::getAnimatedStyleForRenderer(RenderElement& renderer)
    374407{
     408    AnimationPrivateUpdateBlock animationUpdateBlock(*this);
     409
    375410    ASSERT(renderer.isCSSAnimating());
    376411    ASSERT(m_compositeAnimations.contains(&renderer));
     
    470505AnimationController::AnimationController(Frame& frame)
    471506    : m_data(std::make_unique<AnimationControllerPrivate>(frame))
    472     , m_beginAnimationUpdateCount(0)
    473507{
    474508}
     
    546580void AnimationController::notifyAnimationStarted(RenderElement&, double startTime)
    547581{
     582    AnimationUpdateBlock animationUpdateBlock(this);
    548583    m_data->receivedStartTimeResponse(startTime);
    549584}
     
    551586bool AnimationController::pauseAnimationAtTime(RenderElement* renderer, const AtomicString& name, double t)
    552587{
     588    AnimationUpdateBlock animationUpdateBlock(this);
    553589    return m_data->pauseAnimationAtTime(renderer, name, t);
    554590}
     
    561597bool AnimationController::pauseTransitionAtTime(RenderElement* renderer, const String& property, double t)
    562598{
     599    AnimationUpdateBlock animationUpdateBlock(this);
    563600    return m_data->pauseTransitionAtTime(renderer, property, t);
    564601}
     
    617654{
    618655    LOG(Animations, "resuming animations for document %p", document);
     656    AnimationUpdateBlock animationUpdateBlock(this);
    619657    m_data->resumeAnimationsForDocument(document);
    620658}
     
    623661{
    624662    LOG(Animations, "animations may start for document %p", document);
     663
     664    AnimationUpdateBlock animationUpdateBlock(this);
    625665    m_data->startAnimationsIfNotSuspended(document);
    626666}
     
    628668void AnimationController::beginAnimationUpdate()
    629669{
    630     if (!m_beginAnimationUpdateCount)
    631         m_data->setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
    632     ++m_beginAnimationUpdateCount;
     670    m_data->beginAnimationUpdate();
    633671}
    634672
    635673void AnimationController::endAnimationUpdate()
    636674{
    637     ASSERT(m_beginAnimationUpdateCount > 0);
    638     --m_beginAnimationUpdateCount;
    639     if (!m_beginAnimationUpdateCount)
    640         m_data->endAnimationUpdate();
     675    m_data->endAnimationUpdate();
    641676}
    642677
  • trunk/Source/WebCore/page/animation/AnimationController.h

    r177269 r177302  
    8383private:
    8484    const std::unique_ptr<AnimationControllerPrivate> m_data;
    85     int m_beginAnimationUpdateCount;
    8685};
    8786
  • trunk/Source/WebCore/page/animation/AnimationControllerPrivate.h

    r177269 r177302  
    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.