Enabling the async image decoding for large images causes screen flickering in many web pages and during interacting with the page. The main reason for this flickering is destroying the decoded frames by the MemoryCache because of memory pressure. A common example is drawing an image which spans multiple tiles. Since drawing to tiles happens in different drawing phases, MemoryCache can destroy decoded frames between two drawing phases. In this example this can lead to Infinitis repaint-draw loop. Assume the following concrete example: An image M is drawn on multiple tiles T1 and T2 1. T1 draws M, no decoded frame is found, request async decoding, nothing is drawn. 2. T2 draws M, no decoded frame is found but it is already being decoded, nothing is drawn. 3. M finishes decoded, the split rectangles of M in T1 and T2 are repainted. 4. T1 draws M, decoded frame is found, part of the image is drawn. 5. MemoryCache asks M to destroy its decoded frame. 6. T2 draws M, no decoded frame is found, request async decoding, nothing is drawn. 7. M finishes decoded, the rectangle of M in T1 and T2 is repainted. 8. Go to step 1. To fix this problem the decoded frames of the large images in the current viewport should not be deleted by the MemoryCache. This makes sense because the viewport is drawn, the same images will be re-decoded. We need to implement the opposite of the pausing the animated images. When the large image decoded its frame it is going to notify its CachedImageClient which is RenderElement in the case of image element and css background image. The RenderElement will check whether it's in the viewport and if is, it is going to add itself to the list of RendererWithPermanentImageFrame and setHasPermanentImageFrame(true). When the RenderView::updateVisibleViewportRect(), we are going to check if any of the RendererWithPermanentImageFrame went outside the viewport and setHasPermanentImageFrame(false) of the image so the decoded can be deleted if it is requested to.
*** Bug 170408 has been marked as a duplicate of this bug. ***
Created attachment 306930 [details] Patch
Comment on attachment 306930 [details] Patch Attachment 306930 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3525665 New failing tests: fast/repaint/no-animation-outside-viewport-subframe.html fast/images/composited-animated-gif-outside-viewport.html fast/images/animated-gif-webkit-transform.html fast/images/animated-gif-iframe-webkit-transform.html
Created attachment 306938 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 306930 [details] Patch Attachment 306930 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3525696 New failing tests: fast/images/animated-gif-webkit-transform.html fast/images/composited-animated-gif-outside-viewport.html fast/images/animated-gif-iframe-webkit-transform.html
Created attachment 306942 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 306930 [details] Patch Attachment 306930 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3525731 New failing tests: fast/images/animated-gif-window-resizing.html fast/images/composited-animated-gif-outside-viewport.html svg/animations/animated-svg-image-outside-viewport-paused.html svg/animations/animated-svg-image-removed-from-document-paused.html fast/images/animated-gif-iframe-webkit-transform.html fast/repaint/no-animation-outside-viewport.html svg/animations/animations-paused-in-background-page.html fast/images/animated-gif-no-layout.html fast/images/animated-gif-zooming.html svg/animations/animations-paused-in-background-page-iframe.html fast/repaint/no-animation-outside-viewport-subframe.html fast/images/animated-gif-body-outside-viewport.html
Created attachment 306943 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 306930 [details] Patch Attachment 306930 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3526228 New failing tests: svg/animations/animated-svg-image-outside-viewport-paused.html svg/animations/animated-svg-image-removed-from-document-paused.html fast/images/animated-gif-zooming.html
Created attachment 306954 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 306955 [details] Patch
<rdar://problem/31469922>
Created attachment 306958 [details] Patch
Comment on attachment 306958 [details] Patch Attachment 306958 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3526857 New failing tests: fast/images/async-retained-cached-image-frame.html
Created attachment 306964 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 306958 [details] Patch Attachment 306958 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3526894 New failing tests: svg/animations/animated-svg-image-outside-viewport-paused.html svg/animations/animations-paused-in-background-page-iframe.html svg/animations/animated-svg-image-removed-from-document-paused.html webrtc/no-port-zero-in-upd-candidates.html svg/animations/animations-paused-in-background-page.html
Created attachment 306965 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 306958 [details] Patch Attachment 306958 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3526903 New failing tests: fast/images/async-retained-cached-image-frame.html
Created attachment 306966 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 306958 [details] Patch Attachment 306958 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3527688 New failing tests: svg/animations/animated-svg-image-outside-viewport-paused.html svg/animations/animated-svg-image-removed-from-document-paused.html webrtc/no-port-zero-in-upd-candidates.html
Created attachment 306977 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 306997 [details] Patch
Created attachment 307023 [details] Patch
Created attachment 307139 [details] Patch
(In reply to Said Abou-Hallawa from comment #24) > Created attachment 307139 [details] > Patch Zalan pointed out that I should use references instead of pointers in the functions of RenderElementCachedImagesMap. I made this change and I changed RenderElementCachedImagesMap to be a template instead and named it MultipleValuesHashMap.
Comment on attachment 307139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307139&action=review > Source/WebCore/ChangeLog:44 > + * WebCore.xcodeproj/project.pbxproj: Add MultipleValuesHashMap.h which is > + included in RenderView.h. > + * loader/cache/CachedImage.cpp: > + (WebCore::CachedImage::CachedImageObserver::newImageFrameAvailable): Rename > + animationAdvanced() to newImageFrameAvailable() so it can be used for animated and > + static large images when a new frame is available. For static large images, this > + happens only when an image frame finishes decoding. > + (WebCore::CachedImage::newImageFrameAvailable): Rename animationAdvanced() to > + newImageFrameAvailable(). Replace canPause by isVisible which is now returned from > + CachedImageClient::newImageFrameAvailable(). > + (WebCore::CachedImage::CachedImageObserver::animationAdvanced): Deleted. > + (WebCore::CachedImage::animationAdvanced): Deleted. > + * loader/cache/CachedImage.h: > + * loader/cache/CachedImageClient.h: For future reference, you don't need to describe all the small changes here. Just highlight the major algorithmic changes. > Source/WebCore/loader/cache/CachedImage.cpp:524 > + while (CachedImageClient* client = clientWalker.next()) auto* > Source/WebCore/loader/cache/CachedImage.cpp:525 > + visible |= client->newImageFrameAvailable(*this, isAnimating, changeRect); I'm concerned that his will short-circuit and not call newImageFrameAvailable() on some clients? > Source/WebCore/loader/cache/MemoryCache.cpp:295 > + CachedImage& image = downcast<CachedImage>(resource); auto& > Source/WebCore/platform/graphics/Image.h:134 > + virtual void newImageFrameAvailableAtIndex(size_t) { } I'm not sure the "new" adds anything to this name. > Source/WebCore/platform/graphics/Image.h:136 > + virtual void setRetainedCachedImageFrame(bool) { } I don't like "retained cached": it sounds redundant. What's the difference between retained and cached? > Source/WebCore/rendering/MultipleValuesHashMap.h:33 > +class MultipleValuesHashMap { I don't think this file belongs in rendering/. Maybe platform somewhere. > Source/WebCore/rendering/RenderElement.cpp:1153 > + if (imageVisibleInViewportState() == VisibleInViewportState::VisibleInViewport) > + view().removeRendererWithRetainedCachedImageFrame(*this); > + if (imageVisibleInViewportState() == VisibleInViewportState::NotVisibleInViewport) > + view().removeRendererWithDiscardedCachedImageFrame(*this); switch? > Source/WebCore/rendering/RenderElement.h:358 > + unsigned m_imageVisibleInViewportState : 2; Please add a comment like // VisibleInViewportState
Comment on attachment 307139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307139&action=review >> Source/WebCore/rendering/MultipleValuesHashMap.h:33 >> +template<typename KeyType, typename ValueType> >> +class MultipleValuesHashMap { > > I don't think this file belongs in rendering/. Maybe platform somewhere. As a generic type this is pretty undercooked and dangerous. Our generic containers store values but this stores pointers, something that is completely invisible from the signature. Nothing in this patch requires a generic container, all use of this type is MultipleValuesHashMap<RenderElement, CachedImage>. Please implement that specific concrete type or simply add a few helper functions that operate on HashMap<RenderElement*, Vector<CachedImage*>>.
Note: This patch also fixes a 6-8% JetStream regression for me on iOS due to the flickering.
Comment on attachment 307139 [details] Patch r- because of MultipleValuesHashMap
(In reply to Antti Koivisto from comment #27) > simply add a few helper functions that operate on > HashMap<RenderElement*, Vector<CachedImage*>> I think this is the right approach.
Created attachment 309570 [details] Patch
Comment on attachment 309570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309570&action=review > Source/WebCore/loader/cache/CachedImage.h:71 > + void startAnimation() { return m_image ? m_image->startAnimation() : void(); } Why return here? void() is weird. > Source/WebCore/loader/cache/CachedImage.h:72 > + void setImagePurgeable(bool purgeable) { return m_image ? m_image->setPurgeable(purgeable) : void(); } Ditto. > Source/WebCore/loader/cache/MemoryCache.cpp:299 > + downcast<BitmapImage>(*image.image()).destroyDecodedData(); Why not just make destroyDecodedData() virtual and have stubs for SVGImage etc? > Source/WebCore/platform/graphics/BitmapImage.cpp:481 > + if (m_purgeable) > + LOG(Images, "BitmapImage::%s - %p - url: %s [decoded current frame can be purged]", __FUNCTION__, this, sourceURL().string().utf8().data()); > + else > + LOG(Images, "BitmapImage::%s - %p - url: %s [decoded current frame can't be purged]", __FUNCTION__, this, sourceURL().string().utf8().data()); LOG(Images, "BitmapImage::%s - %p - url: %s [decoded current frame can%s be purged]", __FUNCTION__, this, sourceURL().string().utf8().data(), m_purgeable ? "" : "'n"); > Source/WebCore/platform/graphics/Image.h:138 > + virtual void setPurgeable(bool) { } I'm not sure we want to use the term "Purgeable" here. It has specific connotations for IOSurfaces etc, and we don't use the terminology for destroyDecodedData. > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:312 > + if (index == frameCount() - 1) > + return CGImageSourceGetStatus(m_nativeDecoder.get()) == kCGImageStatusComplete; > return CGImageSourceGetStatusAtIndex(m_nativeDecoder.get(), index) == kCGImageStatusComplete; Weird, why is the last frame special? > Source/WebCore/rendering/RenderElement.cpp:1510 > + if (shouldRepaint && animatingState == ImageAnimatingState::No) > + view().addRendererWithLargeImages(*this, image); I don't like how this is not symmetrical with the removeRendererWithLargeImages(). That tests imageVisibleInViewportState(), but here we don't set imageVisibleInViewportState directly; we just return it and hope the caller sets it? > Source/WebCore/rendering/RenderElement.h:351 > + unsigned m_imageVisibleInViewportState : 2; Please add a comment with the enum name. > Source/WebCore/rendering/RenderView.cpp:1401 > +static void addRendererImageToMap(HashMap<RenderElement*, HashSet<CachedImage*>>& map, RenderElement& renderer, CachedImage& image) This would be cleaner if you added a typedef for HashMap<RenderElement*, HashSet<CachedImage*>> > Source/WebCore/rendering/RenderView.cpp:1482 > + m_renderersWithPausedImageAnimations.removeIf( > + [&toRemove = toRemove](HashMap<RenderElement*, HashSet<CachedImage*>>::KeyValuePairType& entry) -> bool { > + return toRemove.find(entry.key) != toRemove.end(); > + }); This is pretty hard to read. > Source/WebCore/rendering/RenderView.cpp:1485 > +void RenderView::setRendererImageVisibleInViewportState(RenderElement& renderer, VisibleInViewportState state) This says RendererImage but you're passing a RenderElement. > Source/WebCore/rendering/RenderView.cpp:1494 > + auto renderers = findRenderersForImageInMap(m_renderersWithLargeImages, *image); Does this lookup make the function O(n^2)? > Source/WebCore/rendering/RenderView.cpp:1529 > + if (renderer->shouldRepaintInVisibleRect(visibleRect)) > + continue; > + > + setRendererImageVisibleInViewportState(*renderer, VisibleInViewportState::No); The fact that shouldRepaintInVisibleRect and setRendererImageVisibleInViewportState use different terminology hurts readability here.
Comment on attachment 309570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309570&action=review > Source/WebCore/rendering/RenderElement.cpp:1151 > + if (imageVisibleInViewportState() != VisibleInViewportState::Unknown) > + view().removeRendererWithLargeImages(*this); A simple test case with swapping bckg images on an element could result in dangling CachedImage pointers in m_renderersWithLargeImages, since we only remove the HashMap entry when the renderer is being destroyed -however, the renderer can easily outlive the CachedImage afaict.
(In reply to zalan from comment #33) > Comment on attachment 309570 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309570&action=review > > > Source/WebCore/rendering/RenderElement.cpp:1151 > > + if (imageVisibleInViewportState() != VisibleInViewportState::Unknown) > > + view().removeRendererWithLargeImages(*this); > > A simple test case with swapping bckg images on an element could result in > dangling CachedImage pointers in m_renderersWithLargeImages, since we only > remove the HashMap entry when the renderer is being destroyed -however, the > renderer can easily outlive the CachedImage afaict. I actually added a bit of a logging and here is a case where the renderer outlives the image. ctor: 0x1287c8480 <- div with background image CachedImage ctor: 0x12db4d000. <-1st background image (small) CachedImage ctor: 0x12860a700. <-2nd background image (large image over 2MB) didRemoveCachedImageClient: 0x1287c8480 image: 0x12db4d000 <- 1st image gets swapped out. didRemoveCachedImageClient: 0x1287c8480 image: 0x12860a700 <- 2nd image gets swapped out. CachedImage ctor: 0x12860a000 <- 3rd bckg image CachedImage dtor: 0x12860a700 <- 2nd image is getting destroy, renderer is still alive.
Comment on attachment 309570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309570&action=review > Source/WebCore/ChangeLog:20 > + To fix this issue we need to protect the decoded frames of all the images > + in the view port from being destroyed. Once the image is moved out of the > + view port, its decoded frame can be destroyed. If the image moves back to > + the view port its decoded will be protected again. We have to keep track > + of all the images which are currently in the view port and all the images > + which were in the view port but moved out of it. I don't quite see why all the complexity in this patch is needed to solve the problem. As I understand you want to avoid destroying frames for images that have any visible clients. CachedImageClient::imageFrameAvailable already returns visibility so this information is already available. If that is somehow inconvenient then you could add a new isVisible virtual method or factor imageFrameAvailable to be more suitable. > Source/WebCore/rendering/RenderView.cpp:1492 > + if (state == VisibleInViewportState::Yes) > + image->setImagePurgeable(false); What prevents another client for the same image that happens to be outside of viewport from later calling setImagePurgeable(true) and overriding this? I can't figure out the exact logic of this patch but I think it is wrong. Renderer does not know about other image clients so this can't be implemented correctly here. Instead CachedImage should be querying its client for visibility and if any of them is visible disallow purging.
Created attachment 309667 [details] Patch
Comment on attachment 309570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309570&action=review >> Source/WebCore/loader/cache/MemoryCache.cpp:299 >> + downcast<BitmapImage>(*image.image()).destroyDecodedData(); > > Why not just make destroyDecodedData() virtual and have stubs for SVGImage etc? I found out Image::destroyDecodedData() is already defined to be a virtual function. MemoryCache::destroyDecodedDataForAllImages() was changed to be not specific about BitmapImage. >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:312 >> return CGImageSourceGetStatusAtIndex(m_nativeDecoder.get(), index) == kCGImageStatusComplete; > > Weird, why is the last frame special? Because CGImageSourceGetStatusAtIndex() changes the return status value from kCGImageStatusIncomplete to kCGImageStatusComplete only if (index > 1 && index < frameCount()-1). So calling CGImageSourceGetStatusAtIndex() for a static image returns always kCGImageStatusIncomplete. >> Source/WebCore/rendering/RenderView.cpp:1529 >> + setRendererImageVisibleInViewportState(*renderer, VisibleInViewportState::No); > > The fact that shouldRepaintInVisibleRect and setRendererImageVisibleInViewportState use different terminology hurts readability here. RenderElement::shouldRepaintInVisibleRect() was renamed to RenderElement::isVisibleInViewport()
Comment on attachment 309667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309667&action=review > Source/WebCore/loader/cache/CachedImage.cpp:345 > +bool CachedImage::CachedImageObserver::isAnyClientVisible(const Image* image) const Image& > Source/WebCore/loader/cache/CachedImage.cpp:512 > +bool CachedImage::isAnyClientVisible(const Image* image) > +{ > + if (!image || image != m_image) > + return false; const Image& if (&image != m_image) > Source/WebCore/loader/cache/CachedImageClient.h:45 > + virtual VisibleInViewportState calcVisibleInViewportState() { return VisibleInViewportState::No; } Please do not abbreviate in function names; use computeVisibleInViewportState instead unless you (or someone) has a better name. > Source/WebCore/platform/graphics/BitmapImage.cpp:323 > + return !(imageObserver() && imageObserver()->isAnyClientVisible(this)); (*this) > Source/WebCore/platform/graphics/ImageObserver.h:48 > + virtual bool isAnyClientVisible(const Image*) = 0; const Image&
Created attachment 309730 [details] Patch
Comment on attachment 309667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309667&action=review >> Source/WebCore/loader/cache/CachedImage.cpp:345 >> +bool CachedImage::CachedImageObserver::isAnyClientVisible(const Image* image) > > const Image& Yes this makes sense. I converted all the const Image* in CachedImage and CachedImageObserver to const Image&.
I implemented Antti's suggestion. It made sense and it turned out to be much simpler and cleaner than the RenderView tracking patch. BitmapImage::destroyDecodedData() will check, through the ImageObserver, whether any of its clients is visible. And if at least one of them is visible and the current decoded frame has to be asynchronously decoded, it won't be destroyed.
Comment on attachment 309730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309730&action=review > Source/WebCore/platform/graphics/ImageObserver.h:48 > + virtual bool isAnyClientVisible(const Image&) = 0; I don't think client visibility should be the thing being asked about in this API. This should be more like "is it OK to discard decoded data". It's the RenderElement's job to decide that throwing away is related to visibility. > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:311 > + if (index == frameCount() - 1) > + return CGImageSourceGetStatus(m_nativeDecoder.get()) == kCGImageStatusComplete; This needs a comment.
Created attachment 309736 [details] Patch
Created attachment 309750 [details] Patch
Comment on attachment 309730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309730&action=review >> Source/WebCore/platform/graphics/ImageObserver.h:48 >> + virtual bool isAnyClientVisible(const Image&) = 0; > > I don't think client visibility should be the thing being asked about in this API. This should be more like "is it OK to discard decoded data". It's the RenderElement's job to decide that throwing away is related to visibility. Function was renamed to canDestroyDecodedData(). >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:311 >> + return CGImageSourceGetStatus(m_nativeDecoder.get()) == kCGImageStatusComplete; > > This needs a comment. Added.
Comment on attachment 309750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309750&action=review > Source/WebCore/platform/graphics/BitmapImage.h:56 > + friend class MemoryCache; Why? We normally make methods public rather than do this. > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:313 > + // CGImageSourceGetStatusAtIndex() changes the return status value from kCGImageStatusIncomplete > + // to kCGImageStatusComplete only if (index > 1 && index < frameCount() - 1). To get an accurate > + // result for the last frame (or the single frame of the static image) use CGImageSourceGetStatus() > + // instead for this frame. Is this a bug or correct behavior? > Source/WebCore/rendering/RenderElement.cpp:1436 > +bool RenderElement::isVisibleInViewport(const IntRect& visibleRect) const The new function name implies that 'visibleRect' is the viewport, but can it be an arbitrary rect? What coordinate system is visibleRect in (e.g. with zooming?). > LayoutTests/fast/images/async-image-background-image-repeated.html:61 > + // Ensure the cached image frames won't be destroyed because they're inside the viewport. > + internals.destroyDecodedDataForAllImages(); The comment doesn't match the next line. Or is the comment trying to say that the relevant part of the test is that internals.destroyDecodedDataForAllImages() will not destroy the image's decoded data?
Created attachment 309826 [details] Patch
Comment on attachment 309750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309750&action=review >> Source/WebCore/platform/graphics/BitmapImage.h:56 >> + friend class MemoryCache; > > Why? We normally make methods public rather than do this. I added it because I wanted to call BitmapImage::destroyDecodedData() from MemoryCache::destroyDecodedDataForAllImages(). But downcast<BitmapImage> was removed and Image:: destroyDecodedData() is public, so no additional change is needed after removing the friend statement. >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:313 >> + // instead for this frame. > > Is this a bug or correct behavior? I don't know. Maybe it is not expected to call CGImageSourceGetStatusAtIndex() for an image with a single frame. The documentation of these APIs does not say whether this is allowed or not. I will file a bug. >> Source/WebCore/rendering/RenderElement.cpp:1436 >> +bool RenderElement::isVisibleInViewport(const IntRect& visibleRect) const > > The new function name implies that 'visibleRect' is the viewport, but can it be an arbitrary rect? What coordinate system is visibleRect in (e.g. with zooming?). This function was renamed to isVisibleInDocumentRect(). > Source/WebCore/rendering/RenderElement.cpp:1497 > +VisibleInViewportState RenderElement::computeVisibleInViewportState() This function was renamed to isVisibleInViewport() which returns bool. >> LayoutTests/fast/images/async-image-background-image-repeated.html:61 >> + internals.destroyDecodedDataForAllImages(); > > The comment doesn't match the next line. Or is the comment trying to say that the relevant part of the test is that internals.destroyDecodedDataForAllImages() will not destroy the image's decoded data? Yes it is. The comment was changed.
Created attachment 310204 [details] Patch
Comment on attachment 310204 [details] Patch Clearing flags on attachment: 310204 Committed r216901: <http://trac.webkit.org/changeset/216901>
All reviewed patches have been landed. Closing bug.
*** Bug 169943 has been marked as a duplicate of this bug. ***