Bug 57868 – REGRESSION (WebKit2): CSS animations on pages that use accelerated compositing stop after switching tabs

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

<rdar://problem/9236832>

[Chris Marrin]

Created attachment 90364 [details]
Patch

[Simon Fraser (smfr)]

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.

[Chris Marrin]

(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.

[Adam Roben (:aroben)]

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*.

[Adam Roben (:aroben)]

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.

[Chris Marrin]

(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

[WebKit Review Bot]

Attachment 90364 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8487088

[Chris Marrin]

Created attachment 90395 [details]
Patch

[Adam Roben (:aroben)]

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.

[WebKit Review Bot]

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.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/84416