Allow frames to be free'd even for static images

Allow frames to be free'd even for static images

r151702 added limit for pruning in ImageDecoded::clearCacheExceptFrame,
which would disallow pruning single-frame/static images altogether.

Allow such images to be pruned in their entirety by dropping the
condition for <= 1 frames - the for-loop will handle the kNotFound case.

(Also related is r156651, which re-added the destroyAll flag.)

Modify the ImageFrameGeneratorTest.frameHasAlpha unittest to use frame 0
instead of frame 1, to avoid clearing frame 1 (MockImageDecoder
maps all indices to frame 0.)

R=hclam@chromium.org,pkasting@chromium.org,senorblanco@chromium.org
BUG=302851
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165127

[Stephen White, 2014-01-13 19:25:17]

I'll happily rubberstamp this, but Peter or Alpha should weigh in on the
correctness.

[Stephen White, 2014-01-13 19:25:17]

I'll happily rubberstamp this, but Peter or Alpha should weigh in on the
correctness.

[Peter Kasting, 2014-01-14 00:46:18]

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageDecoder.cpp:140: return 0;
Nit: Can't we just implement this by removing this block entirely?  It seems
like the code below will already do what we want in this case, if only we didn't
have this condition.

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageDecoderTest.cpp:235: Vector<ImageFrame, 1>&
frameBuffers = decoder->frameBufferCache();
Nit: What about:

ImageFrame* frame = &decoder->frameBufferCache().first();

Then you can use "frame->" in both places below, which seems clearer.

[fs, 2014-01-14 08:32:09]

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageDecoder.cpp:140: return 0;
On 2014/01/14 00:46:18, Peter Kasting wrote:
> Nit: Can't we just implement this by removing this block entirely?  It seems
> like the code below will already do what we want in this case, if only we
didn't
> have this condition.

Yes, true. Will drop this block.

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/136503002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageDecoderTest.cpp:235: Vector<ImageFrame, 1>&
frameBuffers = decoder->frameBufferCache();
On 2014/01/14 00:46:18, Peter Kasting wrote:
> Nit: What about:
> 
> ImageFrame* frame = &decoder->frameBufferCache().first();
> 
> Then you can use "frame->" in both places below, which seems clearer.

Agreed. Will change.

[Peter Kasting, 2014-01-14 20:04:49]

LGTM

https://codereview.chromium.org/136503002/diff/140001/Source/platform/graphic...
File Source/platform/graphics/ImageFrameGeneratorTest.cpp (right):

https://codereview.chromium.org/136503002/diff/140001/Source/platform/graphic...
Source/platform/graphics/ImageFrameGeneratorTest.cpp:409:
ImageDecodingStore::instance()->unlockCache(m_generator.get(),
m_generator->decodeAndScale(fullSize(), 0));
Out of curiosity, why the changes in this file?

https://codereview.chromium.org/136503002/diff/140001/Source/platform/image-d...
File Source/platform/image-decoders/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/136503002/diff/140001/Source/platform/image-d...
Source/platform/image-decoders/ImageDecoderTest.cpp:239: 
Nit: Extra blank line (line 224 could probably also be nuked)

[fs, 2014-01-15 09:48:24]

https://codereview.chromium.org/136503002/diff/140001/Source/platform/graphic...
File Source/platform/graphics/ImageFrameGeneratorTest.cpp (right):

https://codereview.chromium.org/136503002/diff/140001/Source/platform/graphic...
Source/platform/graphics/ImageFrameGeneratorTest.cpp:409:
ImageDecodingStore::instance()->unlockCache(m_generator.get(),
m_generator->decodeAndScale(fullSize(), 0));
On 2014/01/14 20:04:49, Peter Kasting wrote:
> Out of curiosity, why the changes in this file?

It previously asked to decode frame 1, but since MockImageDecoder always only
keep one frame, and ImageFrameGenerator(::decode) always calls
clearCacheExceptFrame (with the decoded frame) after decoding, this would lead
to "all" frames (==the single frame MockImageDecoder keeps) being cleared (since
'1' is not a valid index and hence the condition in the loop always true.)

I added a comment along the lines of the above (but more terse) to the CL
description.

https://codereview.chromium.org/136503002/diff/140001/Source/platform/image-d...
File Source/platform/image-decoders/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/136503002/diff/140001/Source/platform/image-d...
Source/platform/image-decoders/ImageDecoderTest.cpp:239: 
On 2014/01/14 20:04:49, Peter Kasting wrote:
> Nit: Extra blank line (line 224 could probably also be nuked)

Will prune.

[jochen (gone - plz use gerrit), 2014-01-15 13:19:57]

rslgtm

[commit-bot: I haz the power, 2014-01-15 13:46:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/136503002/210001

[commit-bot: I haz the power, 2014-01-15 14:13:52]

Change committed as 165127

[fs, 2014-03-17 11:47:36]

A revert of this CL has been created in
https://codereview.chromium.org/201793002/ by fs@opera.com.

The reason for reverting is: May have caused issue 350653..