Summary: | [wk2] REGRESSION (r138858): GIFs don't start playing when they come out of background tabs | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||
Component: | WebKit2 | Assignee: | Tim Horton <thorton> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, dino, dstockwell, esprehn+autocc, koivisto, ojan.autocc, simon.fraser | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 114680, 119217 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Tim Horton
2013-02-04 14:53:41 PST
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. |