[webkit-reviews] review denied: [Bug 97866] Memory increase by uncontrolled decoded Image data after user's clearing cache : [Attachment 166182] patch for uncontrolled decoded image data

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Keunyong Lee
<tros07 at gmail.com>'s request for review:
Bug 97866: Memory increase by uncontrolled decoded Image data after user's
clearing cache
https://bugs.webkit.org/show_bug.cgi?id=97866

Attachment 166182: patch for uncontrolled decoded image data
https://bugs.webkit.org/attachment.cgi?id=166182&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166182&action=review


No opinion on the correctness at this time. Why is there no test?

> Source/WebCore/ChangeLog:14
> +	   The cause of this problem is that all elements are removed from
> +	   resource list after calling evictResources() although some resources

> +	   of current page cannot be deleted. As you know, decoded Image Data
> +	   should be controlled by prune() or any other method.However, because

> +	   there is no elements in resource list after clear cache, memoryCache

> +	   cannot remove old decoded data and stack all decoded data of
> +	   current page.

You should either add a test, or give a very good reason no to have one.

> Source/WebCore/loader/cache/MemoryCache.cpp:752
> +    // In case of memory cache clearing, MemoryCache::setDisabled cause
uncontrolled decoded data problem.
> +    // See <https://bugs.webkit.org/show_bug.cgi?id=97866>.

Such comments should not be here, it does not help understanding the following
code.

Giving "why" comment is good, you should not just cite a particular problem in
comments.

> Source/WebCore/loader/cache/MemoryCache.cpp:755
> +    CachedResourceMap::iterator e = m_resources.end();

"e" is a bad name.

> Source/WebCore/loader/cache/MemoryCache.cpp:756
> +    for (CachedResourceMap::iterator i = m_resources.begin(); i != e; ++i) {


"i" is also a bad name.