[webkit-reviews] review granted: [Bug 170640] Do not delete asynchronously decoded frames for large images if their clients are in the viewport : [Attachment 309750] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 170640: Do not delete asynchronously decoded frames for large images if
their clients are in the viewport
https://bugs.webkit.org/show_bug.cgi?id=170640

Attachment 309750: Patch

https://bugs.webkit.org/attachment.cgi?id=309750&action=review




--- Comment #46 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 309750
  --> https://bugs.webkit.org/attachment.cgi?id=309750
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?