[webkit-reviews] review requested: [Bug 108864] [wk2] REGRESSION (r138858): GIFs don't start playing when they come out of background tabs : [Attachment 197649] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 11 12:40:08 PDT 2013


Antti Koivisto <koivisto at iki.fi> has asked  for review:
Bug 108864: [wk2] REGRESSION (r138858): GIFs don't start playing when they come
out of background tabs
https://bugs.webkit.org/show_bug.cgi?id=108864

Attachment 197649: patch
https://bugs.webkit.org/attachment.cgi?id=197649&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.


More information about the webkit-reviews mailing list