Bug 57868

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 RenderingAssignee: 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 Flags
Patch
cmarrin: review-
Patch aroben: review+

Description Adam Roben (:aroben) 2011-04-05 11:32:24 PDT
To reproduce:

1. Go to http://www.webkit.org/blog-files/3d-transforms/poster-circle.html
2. Open a new tab
3. Switch back to the first tab

The page isn't animating anymore. Reloading the page fixes the problem.

This did not happen with WebKit1.
Comment 1 Adam Roben (:aroben) 2011-04-05 11:32:51 PDT
<rdar://problem/9236832>
Comment 2 Chris Marrin 2011-04-20 11:03:48 PDT
Created attachment 90364 [details]
Patch
Comment 3 Simon Fraser (smfr) 2011-04-20 11:13:58 PDT
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.
Comment 4 Chris Marrin 2011-04-20 11:15:23 PDT
(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 5 Adam Roben (:aroben) 2011-04-20 11:25:02 PDT
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 6 Adam Roben (:aroben) 2011-04-20 11:26:12 PDT
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.
Comment 7 Chris Marrin 2011-04-20 11:43:29 PDT
(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
Comment 8 WebKit Review Bot 2011-04-20 12:33:42 PDT
Attachment 90364 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8487088
Comment 9 Chris Marrin 2011-04-20 13:21:11 PDT
Created attachment 90395 [details]
Patch
Comment 10 Adam Roben (:aroben) 2011-04-20 13:23:23 PDT
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.
Comment 11 WebKit Review Bot 2011-04-20 13:23:54 PDT
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.
Comment 12 Chris Marrin 2011-04-20 14:21:01 PDT
Landed in http://trac.webkit.org/changeset/84416