Summary: | REGRESSION (WebKit2): CSS animations on pages that use accelerated compositing stop after switching tabs | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||
Component: | Layout and Rendering | Assignee: | Chris Marrin <cmarrin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, ap, cmarrin, simon.fraser, webkit.review.bot | ||||||
Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Windows 7 | ||||||||
URL: | http://www.webkit.org/blog-files/3d-transforms/poster-circle.html | ||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2011-04-05 11:32:24 PDT
Created attachment 90364 [details]
Patch
Comment on attachment 90364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90364&action=review > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:46 > +static CFTimeInterval currentTimeToMediaTime(double t) > +{ > + return CACurrentMediaTime() + t - WTF::currentTime(); > +} This is already in GraphicsLayerCA. Shame to copy it. (In reply to comment #3) > (From update of attachment 90364 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90364&action=review > > > Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:46 > > +static CFTimeInterval currentTimeToMediaTime(double t) > > +{ > > + return CACurrentMediaTime() + t - WTF::currentTime(); > > +} > > This is already in GraphicsLayerCA. Shame to copy it. I'll reuse it. Comment on attachment 90364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90364&action=review > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:248 > +void PlatformCALayer::animationStarted(CFTimeInterval beginTime) > +{ > + if (m_owner) > + m_owner->platformCALayerAnimationStarted(beginTime); > +} > + > +void PlatformCALayer::ensureAnimationsSubmitted() > +{ > +} > + Why are these needed? Are these functions ever called on Mac? > Source/WebCore/platform/graphics/ca/PlatformCAAnimation.h:145 > + void setActualStartTimeIfNeeded(CFTimeInterval t) > + { > + if (beginTime() <= 0) > + setBeginTime(t); > + } Shouldn't we be comparing beginTime() to the current CA media time, rather than 0? > Source/WebKit2/WebProcess/WebPage/ca/win/LayerTreeHostCAWin.h:51 > +protected: > + // LayerTreeHostCA > + virtual void setRootCompositingLayer(WebCore::GraphicsLayer*); This can (and should) be private, even though it's protected at the LayerTreeHostCA level. We don't expect anyone to call this function on a variable with static type LayerTreeHostCAWin*. Comment on attachment 90364 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90364&action=review > Source/WebKit2/WebProcess/WebPage/ca/win/LayerTreeHostCAWin.cpp:304 > + GraphicsLayerCA* graphicsLayerCA = static_cast<GraphicsLayerCA*>(graphicsLayer); > + PlatformCALayer* platformCALayer = graphicsLayerCA->platformCALayer(); > + platformCALayer->ensureAnimationsSubmitted(); You could get away without the locals here, I think. (In reply to comment #5) > (From update of attachment 90364 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90364&action=review > > > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:248 > > +void PlatformCALayer::animationStarted(CFTimeInterval beginTime) > > +{ > > + if (m_owner) > > + m_owner->platformCALayerAnimationStarted(beginTime); > > +} > > + > > +void PlatformCALayer::ensureAnimationsSubmitted() > > +{ > > +} > > + > > Why are these needed? Are these functions ever called on Mac? I will toss them > > > Source/WebCore/platform/graphics/ca/PlatformCAAnimation.h:145 > > + void setActualStartTimeIfNeeded(CFTimeInterval t) > > + { > > + if (beginTime() <= 0) > > + setBeginTime(t); > > + } > > Shouldn't we be comparing beginTime() to the current CA media time, rather than 0? All animations either have a beginTime of 0, meaning they should start ASAP, or non-zero, meaning we want them to start at that actual time. So I only want to replace the ones with the "special" value of 0. The <= is just to be safe. > > > Source/WebKit2/WebProcess/WebPage/ca/win/LayerTreeHostCAWin.h:51 > > +protected: > > + // LayerTreeHostCA > > + virtual void setRootCompositingLayer(WebCore::GraphicsLayer*); > > This can (and should) be private, even though it's protected at the LayerTreeHostCA level. We don't expect anyone to call this function on a variable with static type LayerTreeHostCAWin*. Ok, I thought it would be better to match the parent. But I'll make it private Attachment 90364 [details] did not build on mac: Build output: http://queues.webkit.org/results/8487088 Created attachment 90395 [details]
Patch
Comment on attachment 90395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90395&action=review > Source/WebKit2/WebProcess/WebPage/ca/win/LayerTreeHostCAWin.cpp:303 > + if (graphicsLayer) { > + static_cast<GraphicsLayerCA*>(graphicsLayer)->platformCALayer()->ensureAnimationsSubmitted(); > + } No need for braces here. Attachment 90395 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/ca/win/LayerTreeHostCAWin.cpp:303: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in http://trac.webkit.org/changeset/84416 |