Bug 108864 – [wk2] REGRESSION (r138858): GIFs don't start playing when they come out of background tabs

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

Steps to Reproduce:

1. Open a tab to an animated gif.
2. Open a tab to about:blank.
3. Switch between them, sometimes waiting a few seconds.

Eventually you'll manage to freeze the GIF.

<rdar://problem/13140489>

[Tim Horton]

http://trac.webkit.org/changeset/148143

[Tim Horton]

Err, wrong bug.

[Tim Horton]

Created attachment 197649 [details]
patch

[Darin Adler]

Comment on attachment 197649 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197649&action=review

> Source/WebCore/dom/Document.cpp:6171
> +        CachedResourceHandle<CachedResource> resource = it->value;

Can this be a const CachedResourceHandle& instead to avoid copying? Or is copying the handle valuable in some way?

> Source/WebCore/dom/Document.h:1218
> +    void advanceAllAnimatedImages();

Is this the best class for this function? For example, should it be in the cached resource loader instead? Seems to be mission-creep for Document.

> Source/WebCore/platform/graphics/BitmapImage.h:186
>      void reportMemoryUsage(MemoryObjectInfo*) const OVERRIDE;

Yuck, missing virtual keyword.

> Source/WebCore/platform/graphics/BitmapImage.h:188
> +    bool canAnimate();

Should this be const?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2006
> +            for (Frame* frame = m_page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {

I think this needs a why comment. A very brief one, and one that answers the questions the code itself doesn’t.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2007
> +                if (Document* document = frame->document())

Is this null check really needed? I don’t think we allow frames in the frame tree that have null documents. But obviously if I am wrong we will regret it later.

[Antti Koivisto]

Comment on attachment 197649 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=197649&action=review

> Source/WebCore/dom/Document.cpp:6167
> +void Document::advanceAllAnimatedImages()
> +{

Something like resumeAnimatingImages() would be more clear about the purpose of this function.

Perhaps this could go to AnimationController? It seems we already use resumeAnimations() for other animation types. I feel this doesn't belong to the Document as it generally does not deal with rendering. The amount of includes you need to add points to this as well.

> Source/WebCore/dom/Document.cpp:6188
> +        CachedResourceHandle<CachedResource> resource = it->value;
> +
> +        if (!resource || !resource->isImage())
> +            continue;
> +
> +        CachedImage* cachedImage = static_cast<CachedImage*>(resource.get());
> +        if (!cachedImage->hasImage())
> +            continue;
> +
> +        Image* image = cachedImage->image();
> +        if (!image->isBitmapImage())
> +            continue;
> +
> +        BitmapImage* bitmapImage = static_cast<BitmapImage*>(image);
> +        if (!bitmapImage->canAnimate())
> +            continue;
> +
> +        cachedImage->animationAdvanced(bitmapImage);

I would remove the empty lines.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2010
>          m_page->didMoveOnscreen();
>          
> -        if (!pageWasInWindow)
> +        if (!pageWasInWindow) {
>              WebProcess::shared().pageDidEnterWindow(this);
> +            
> +            for (Frame* frame = m_page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> +                if (Document* document = frame->document())
> +                    document->advanceAllAnimatedImages();
> +            }
> +        }

This code should probably done in WebCore side under Page::didMoveOnscreen(). Or maybe it should use the MediaCanStartListener mechanism. I don't think it belongs to WebKit2 in any case.

[Antti Koivisto]

Comment on attachment 197649 [details]
patch

Oops, didn't mean to change the r=darin status. But please consider the comments.

[Darin Adler]

I think it’s fine to remove my review flag. Our comments were pointing in the same direction and I feel confident Tim wants to do another cut at this.

[Tim Horton]

Created attachment 197680 [details]
revised patch, moved to AnimationController

[WebKit Commit Bot]

Attachment 197680 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/platform/graphics/BitmapImage.cpp', u'Source/WebCore/platform/graphics/BitmapImage.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
Source/WebCore/page/animation/AnimationControllerPrivate.h:86:  The parameter name "document" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

(In reply to comment #4)
> > Source/WebCore/platform/graphics/BitmapImage.h:188
> > +    bool canAnimate();
> 
> Should this be const?

I wish, but one of the things it calls can mutate the BitmapImage because of caching. Probably a use for 'mutable', but I'd rather not touch it.

[Simon Fraser (smfr)]

Comment on attachment 197680 [details]
revised patch, moved to AnimationController

AnimationController is just about CSS animations/keyframes. It should not do anything with images.

[Tim Horton]

Created attachment 197683 [details]
patch

frameview? grasping at straws after talking to simon :)

[Antti Koivisto]

Comment on attachment 197683 [details]
patch

Ok, though FrameView is oversized and not a great place for this either. One option would be to add

static void CachedImage::resumeAnimationsForLoader(CachedResourceLoader*)

which would be called by FrameView::resumeAnimatingImages().

But really, it would be good to have one place for controlling all animation types. AnimationController would be a fine name for one.

[Simon Fraser (smfr)]

(In reply to comment #13)
> (From update of attachment 197683 [details])
> Ok, though FrameView is oversized and not a great place for this either. One option would be to add
> 
> static void CachedImage::resumeAnimationsForLoader(CachedResourceLoader*)
> 
> which would be called by FrameView::resumeAnimatingImages().
> 
> But really, it would be good to have one place for controlling all animation types. AnimationController would be a fine name for one.

So a long term plan might be to rename AnimationController to CSSAnimationController. However, it's not clear to me that it makes sense for GIF animations to be lumped in with CSS/SVG animations. GIF animations are just images with slightly special behavior.

[Tim Horton]

http://trac.webkit.org/changeset/148300