Do not delete asynchronously decoded frames for large images if their clients are in the viewport

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

*** Bug 170408 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

Created attachment 306930 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 306955 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/31469922>

[Said Abou-Hallawa]

Created attachment 306958 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 306997 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307023 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307139 [details]
Patch

[Said Abou-Hallawa]

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

[Simon Fraser (smfr)]

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

[Antti Koivisto]

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

[Saam Barati]

Note:
This patch also fixes a 6-8% JetStream regression for me on iOS due to the flickering.

[Simon Fraser (smfr)]

Comment on attachment 307139 [details]
Patch

r- because of MultipleValuesHashMap

[Darin Adler]

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

[Said Abou-Hallawa]

Created attachment 309570 [details]
Patch

[Simon Fraser (smfr)]

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.

[zalan]

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.

[zalan]

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

[Antti Koivisto]

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.

[Said Abou-Hallawa]

Created attachment 309667 [details]
Patch

[Said Abou-Hallawa]

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

[zalan]

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&

[Said Abou-Hallawa]

Created attachment 309730 [details]
Patch

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 309736 [details]
Patch

[Said Abou-Hallawa]

Created attachment 309750 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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?

[Said Abou-Hallawa]

Created attachment 309826 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 310204 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 310204 [details]
Patch

Clearing flags on attachment: 310204

Committed r216901: <http://trac.webkit.org/changeset/216901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

*** Bug 169943 has been marked as a duplicate of this bug. ***