- Revision
- 177302
- Author
- [email protected]
- Date
- 2014-12-15 12:24:45 -0800 (Mon, 15 Dec 2014)
Log 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:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (177301 => 177302)
--- trunk/Source/WebCore/ChangeLog 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/ChangeLog 2014-12-15 20:24:45 UTC (rev 177302)
@@ -1,3 +1,78 @@
+2014-12-12 Simon Fraser <[email protected]>
+
+ 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().
+
+ Identical to the patch that was rolled out in r177269 with the addition of a
+ Ref<Frame> protector(m_frame) in AnimationControllerPrivate::animationTimerFired()
+ that ensures that the AnimationControllerPrivate is kept alive for the scope of
+ the AnimationPrivateUpdateBlock, when a transitionEnd event destroys an iframe.
+
+ 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:
+
+2014-12-12 Simon Fraser <[email protected]>
+
+ REGRESSION (r168217): Images are cropped out during animation at jetblue.com
+ https://bugs.webkit.org/show_bug.cgi?id=136410
+
+ Reviewed by Dean Jackson.
+
+ We were hitting the new assertion under Page::setPageScaleFactor(), which
+ calls recalcStyle(), so move the AnimationUpdateBlock from updateStyleIfNeeded()
+ to recalcStyle().
+
+ * dom/Document.cpp:
+ (WebCore::Document::recalcStyle):
+ (WebCore::Document::updateStyleIfNeeded):
+
2014-12-15 Myles C. Maxfield <[email protected]>
Addressing post-review comments in r177035
Modified: trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp (177301 => 177302)
--- trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp 2014-12-15 20:24:45 UTC (rev 177302)
@@ -1705,7 +1705,6 @@
RenderObject* renderer = styledNode->renderer();
if (renderer && renderer->isComposited() && AnimationController::supportsAcceleratedAnimationOfProperty(propertyID)) {
- AnimationUpdateBlock animationUpdateBlock(&renderer->animation());
RefPtr<RenderStyle> style = renderer->animation().getAnimatedStyleForRenderer(downcast<RenderElement>(*renderer));
if (pseudoElementSpecifier && !styledNode->isPseudoElement()) {
// FIXME: This cached pseudo style will only exist if the animation has been run at least once.
Modified: trunk/Source/WebCore/dom/Document.cpp (177301 => 177302)
--- trunk/Source/WebCore/dom/Document.cpp 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/dom/Document.cpp 2014-12-15 20:24:45 UTC (rev 177302)
@@ -1763,6 +1763,7 @@
return; // Guard against re-entrancy. -dwh
RenderView::RepaintRegionAccumulator repaintRegionAccumulator(renderView());
+ AnimationUpdateBlock animationUpdateBlock(&m_frame->animation());
// FIXME: We should update style on our ancestor chain before proceeding (especially for seamless),
// however doing so currently causes several tests to crash, as Frame::setDocument calls Document::attach
@@ -1835,7 +1836,6 @@
if ((!m_pendingStyleRecalcShouldForce && !childNeedsStyleRecalc()) || inPageCache())
return;
- AnimationUpdateBlock animationUpdateBlock(m_frame ? &m_frame->animation() : nullptr);
recalcStyle(Style::NoChange);
}
Modified: trunk/Source/WebCore/page/FrameView.cpp (177301 => 177302)
--- trunk/Source/WebCore/page/FrameView.cpp 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/page/FrameView.cpp 2014-12-15 20:24:45 UTC (rev 177302)
@@ -1153,7 +1153,8 @@
return;
InspectorInstrumentationCookie cookie = InspectorInstrumentation::willLayout(&frame());
-
+ AnimationUpdateBlock animationUpdateBlock(&frame().animation());
+
if (!allowSubtree && m_layoutRoot) {
m_layoutRoot->markContainingBlocksForLayout(false);
m_layoutRoot = 0;
@@ -3940,6 +3941,8 @@
// region but then become included later by the second frame adding rects to the dirty region
// when it lays out.
+ AnimationUpdateBlock animationUpdateBlock(&frame().animation());
+
frame().document()->updateStyleIfNeeded();
if (needsLayout())
Modified: trunk/Source/WebCore/page/animation/AnimationController.cpp (177301 => 177302)
--- trunk/Source/WebCore/page/animation/AnimationController.cpp 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/page/animation/AnimationController.cpp 2014-12-15 20:24:45 UTC (rev 177302)
@@ -51,6 +51,22 @@
static const double cAnimationTimerDelay = 0.025;
static const double cBeginAnimationUpdateTimeNotSet = -1;
+class AnimationPrivateUpdateBlock {
+public:
+ AnimationPrivateUpdateBlock(AnimationControllerPrivate& animationController)
+ : m_animationController(animationController)
+ {
+ m_animationController.beginAnimationUpdate();
+ }
+
+ ~AnimationPrivateUpdateBlock()
+ {
+ m_animationController.endAnimationUpdate();
+ }
+
+ AnimationControllerPrivate& m_animationController;
+};
+
AnimationControllerPrivate::AnimationControllerPrivate(Frame& frame)
: m_animationTimer(*this, &AnimationControllerPrivate::animationTimerFired)
, m_updateStyleIfNeededDispatcher(*this, &AnimationControllerPrivate::updateStyleIfNeededDispatcherFired)
@@ -58,6 +74,7 @@
, m_beginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet)
, m_animationsWaitingForStyle()
, m_animationsWaitingForStartTimeResponse()
+ , m_beginAnimationUpdateCount(0)
, m_waitingForAsyncStartNotification(false)
, m_isSuspended(false)
, m_allowsNewAnimationsWhileSuspended(false)
@@ -228,9 +245,12 @@
void AnimationControllerPrivate::animationTimerFired()
{
+ // We need to keep the frame alive, since it owns us.
+ Ref<Frame> protector(m_frame);
+
// Make sure animationUpdateTime is updated, so that it is current even if no
// styleChange has happened (e.g. accelerated animations)
- setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
+ AnimationPrivateUpdateBlock updateBlock(*this);
// When the timer fires, all we do is call setChanged on all DOM nodes with running animations and then do an immediate
// updateStyleIfNeeded. It will then call back to us with new information.
@@ -287,7 +307,7 @@
void AnimationControllerPrivate::suspendAnimationsForDocument(Document* document)
{
- setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
+ AnimationPrivateUpdateBlock updateBlock(*this);
for (auto it = m_compositeAnimations.begin(), end = m_compositeAnimations.end(); it != end; ++it) {
if (&it->key->document() == document)
@@ -299,7 +319,7 @@
void AnimationControllerPrivate::resumeAnimationsForDocument(Document* document)
{
- setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
+ AnimationPrivateUpdateBlock updateBlock(*this);
for (auto it = m_compositeAnimations.begin(), end = m_compositeAnimations.end(); it != end; ++it) {
if (&it->key->document() == document)
@@ -352,16 +372,29 @@
double AnimationControllerPrivate::beginAnimationUpdateTime()
{
+ ASSERT(m_beginAnimationUpdateCount);
if (m_beginAnimationUpdateTime == cBeginAnimationUpdateTimeNotSet)
m_beginAnimationUpdateTime = monotonicallyIncreasingTime();
+
return m_beginAnimationUpdateTime;
}
+void AnimationControllerPrivate::beginAnimationUpdate()
+{
+ if (!m_beginAnimationUpdateCount)
+ setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
+ ++m_beginAnimationUpdateCount;
+}
+
void AnimationControllerPrivate::endAnimationUpdate()
{
- styleAvailable();
- if (!m_waitingForAsyncStartNotification)
- startTimeResponse(beginAnimationUpdateTime());
+ ASSERT(m_beginAnimationUpdateCount > 0);
+ if (m_beginAnimationUpdateCount == 1) {
+ styleAvailable();
+ if (!m_waitingForAsyncStartNotification)
+ startTimeResponse(beginAnimationUpdateTime());
+ }
+ --m_beginAnimationUpdateCount;
}
void AnimationControllerPrivate::receivedStartTimeResponse(double time)
@@ -372,6 +405,8 @@
PassRefPtr<RenderStyle> AnimationControllerPrivate::getAnimatedStyleForRenderer(RenderElement& renderer)
{
+ AnimationPrivateUpdateBlock animationUpdateBlock(*this);
+
ASSERT(renderer.isCSSAnimating());
ASSERT(m_compositeAnimations.contains(&renderer));
const CompositeAnimation& rendererAnimations = *m_compositeAnimations.get(&renderer);
@@ -469,7 +504,6 @@
AnimationController::AnimationController(Frame& frame)
: m_data(std::make_unique<AnimationControllerPrivate>(frame))
- , m_beginAnimationUpdateCount(0)
{
}
@@ -545,11 +579,13 @@
void AnimationController::notifyAnimationStarted(RenderElement&, double startTime)
{
+ AnimationUpdateBlock animationUpdateBlock(this);
m_data->receivedStartTimeResponse(startTime);
}
bool AnimationController::pauseAnimationAtTime(RenderElement* renderer, const AtomicString& name, double t)
{
+ AnimationUpdateBlock animationUpdateBlock(this);
return m_data->pauseAnimationAtTime(renderer, name, t);
}
@@ -560,6 +596,7 @@
bool AnimationController::pauseTransitionAtTime(RenderElement* renderer, const String& property, double t)
{
+ AnimationUpdateBlock animationUpdateBlock(this);
return m_data->pauseTransitionAtTime(renderer, property, t);
}
@@ -616,28 +653,26 @@
void AnimationController::resumeAnimationsForDocument(Document* document)
{
LOG(Animations, "resuming animations for document %p", document);
+ AnimationUpdateBlock animationUpdateBlock(this);
m_data->resumeAnimationsForDocument(document);
}
void AnimationController::startAnimationsIfNotSuspended(Document* document)
{
LOG(Animations, "animations may start for document %p", document);
+
+ AnimationUpdateBlock animationUpdateBlock(this);
m_data->startAnimationsIfNotSuspended(document);
}
void AnimationController::beginAnimationUpdate()
{
- if (!m_beginAnimationUpdateCount)
- m_data->setBeginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet);
- ++m_beginAnimationUpdateCount;
+ m_data->beginAnimationUpdate();
}
void AnimationController::endAnimationUpdate()
{
- ASSERT(m_beginAnimationUpdateCount > 0);
- --m_beginAnimationUpdateCount;
- if (!m_beginAnimationUpdateCount)
- m_data->endAnimationUpdate();
+ m_data->endAnimationUpdate();
}
bool AnimationController::supportsAcceleratedAnimationOfProperty(CSSPropertyID property)
Modified: trunk/Source/WebCore/page/animation/AnimationController.h (177301 => 177302)
--- trunk/Source/WebCore/page/animation/AnimationController.h 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/page/animation/AnimationController.h 2014-12-15 20:24:45 UTC (rev 177302)
@@ -82,7 +82,6 @@
private:
const std::unique_ptr<AnimationControllerPrivate> m_data;
- int m_beginAnimationUpdateCount;
};
class AnimationUpdateBlock {
Modified: trunk/Source/WebCore/page/animation/AnimationControllerPrivate.h (177301 => 177302)
--- trunk/Source/WebCore/page/animation/AnimationControllerPrivate.h 2014-12-15 20:15:05 UTC (rev 177301)
+++ trunk/Source/WebCore/page/animation/AnimationControllerPrivate.h 2014-12-15 20:24:45 UTC (rev 177302)
@@ -97,6 +97,8 @@
double beginAnimationUpdateTime();
void setBeginAnimationUpdateTime(double t) { m_beginAnimationUpdateTime = t; }
+
+ void beginAnimationUpdate();
void endAnimationUpdate();
void receivedStartTimeResponse(double);
@@ -141,6 +143,9 @@
typedef HashSet<RefPtr<AnimationBase>> WaitingAnimationsSet;
WaitingAnimationsSet m_animationsWaitingForStyle;
WaitingAnimationsSet m_animationsWaitingForStartTimeResponse;
+
+ int m_beginAnimationUpdateCount;
+
bool m_waitingForAsyncStartNotification;
bool m_isSuspended;
