[webkit-reviews] review denied: [Bug 90721] Don't destory the decoded data of an image if WebKit is about to render the image. : [Attachment 151136] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 90721: Don't destory the decoded data of an image if WebKit is about to
render the image.
https://bugs.webkit.org/show_bug.cgi?id=90721

Attachment 151136: Patch
https://bugs.webkit.org/attachment.cgi?id=151136&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151136&action=review


> Source/WebCore/ChangeLog:8
> +	   When the cache capacity of MemoryCache is exceeded, decoded data of
all

... the MemoryCache
... the decoded data of all the CachedImages

> Source/WebCore/ChangeLog:15
> +	   Therefore, it is better not to destory the decoded data of an image
if the image

better to not destroy

> Source/WebCore/ChangeLog:37
> +	   No new tests - no behavior changes. Only space-time tradeoff change.


There is a behavior change, which you note above. GIFs outside the viewport
won't keep animating. Not sure if that's testable though.

> Source/WebCore/loader/cache/CachedImage.cpp:420
> +	   CachedResourceClientWalker<CachedImageClient> w(m_clients);

Please use a longer variable name than 'w'.

> Source/WebCore/loader/cache/CachedImage.cpp:423
> +	       if (c->willRenderImage(this))
> +		   return;

It's odd that a method called destroyDecodedData() doesn't always do so. Maybe
rename to destroyDecodedDataIfPossible(), or add
CachedImage::likelyToRenderSoon() and call it somewhere.