GIF Animations "clear from cache related glitch" fix

third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp

Index: third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
index 09f931bb74b691ffd32a4543fee80a1a121818ab..a9a80e0ea157812d385cf219aaf35d7b8caa53dd 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -294,7 +294,7 @@ SkBitmap ImageFrameGenerator::tryToResumeDecode(size_t index, const SkISize& sca
             if (*it)
                 decodedFrameCount++;
         }
-        removeDecoder = m_frameCount && (decodedFrameCount == m_frameCount);
+        removeDecoder = m_frameCount && (decodedFrameCount == m_frameCount) && (index == m_frameCount - 1);

[Peter Kasting, 2016/01/14 00:17:29]

Why is this additional check needed?  If all frames are complete, it shouldn't
matter what the current frame index is.

[aleksandar.stojiljkovic, 2016/01/14 11:32:53]

This is about different issue (unrelated to DisposeOverwritePrevious), described
here [1].
It is incorrect to clear all frames (which happens if decoder is removed) in the
middle of animation loop as the next frame would need to decode all the way from
0 frame.
i.e. decoded frames for 7 frame animation: sequence: 0,1,2,3,4,<navigate away
from gif so that there is no ImageFrameGenerator to decode frame5>,6,0,1,2,3,4,5
<now decoder gets removed and all the frames>,6<decoding 6 would require
decoding all the frames 0-5>.  


[1]
https://code.google.com/p/chromium/issues/detail?id=507273#c7

[Peter Kasting, 2016/01/14 22:58:39]

I didn't write any of this code, so I'm having a hard time properly reviewing
it.  Note that I'm not in the OWNERS list for this directory.  I suggest you
split this change out and get it reviewed separately.

When you do, you need to update the comments here to reflect what you're
actually doing and why.  And the explanation you gave me isn't really clear
enough; in particular, I'm not clear on precisely what objects are being cleared
where and who holds copies of SkBitmaps or other decoded image data.  While
deleting a decoder and then asking to decode frame <n> would indeed be
problematic, it looks to me as if the code here assumes that once all frames are
complete, someone will have cached all the decoded frames and we won't ever need
to decode again.  If that's not actually happening, I wonder if deleting the
decoder here is the right thing to do at all.

     } else {
         removeDecoder = complete;
     }