Bug 108864

Err, wrong bug.

Created attachment 197649 [details] patch

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.

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.

Comment on attachment 197649 [details] patch Oops, didn't mean to change the r=darin status. But please consider the comments.

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.

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

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.

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

Comment on attachment 197680 [details] revised patch, moved to AnimationController AnimationController is just about CSS animations/keyframes. It should not do anything with images.

Created attachment 197683 [details] patch frameview? grasping at straws after talking to simon :)

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.

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