Move gif image decoder to SkCodec

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
index dec862c04dd80bc31c93bb790634b522496d3a4c..7102b74ba2af1dfda12fcd708e95ad395cb3ae9c 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.h
@@ -28,72 +28,56 @@
 
 #include <memory>
 #include "platform/image-decoders/ImageDecoder.h"
+#include "platform/image-decoders/SegmentStream.h"
 #include "platform/wtf/Noncopyable.h"
+#include "platform/wtf/RefPtr.h"
+#include "third_party/skia/include/codec/SkCodec.h"
 
 namespace blink {
 
-class GIFImageReader;
-
-using GIFRow = Vector<unsigned char>;
-
 // This class decodes the GIF image format.
-class PLATFORM_EXPORT GIFImageDecoder final : public ImageDecoder {
+class PLATFORM_EXPORT GIFImageDecoder final : public blink::ImageDecoder {

[scroggo_chromium, 2017/07/18 21:10:10]

Why did you add "blink::"?

[cblume, 2017/07/19 23:27:05]

Hrmmm I'm guessing I accidentally removed the namespace blink { at some point?
Done.

   WTF_MAKE_NONCOPYABLE(GIFImageDecoder);
 
  public:
   GIFImageDecoder(AlphaOption, const ColorBehavior&, size_t max_decoded_bytes);
   ~GIFImageDecoder() override;
 
-  enum GIFParseQuery { kGIFSizeQuery, kGIFFrameCountQuery };
-
   // ImageDecoder:
   String FilenameExtension() const override { return "gif"; }
   void OnSetData(SegmentReader* data) override;
   int RepetitionCount() const override;
   bool FrameIsCompleteAtIndex(size_t) const override;
   float FrameDurationAtIndex(size_t) const override;
-  // CAUTION: SetFailed() deletes |reader_|.  Be careful to avoid
-  // accessing deleted memory, especially when calling this from inside
-  // GIFImageReader!
+  // CAUTION: SetFailed() deletes |codec_|. Be careful to avoid
+  // accessing deleted memory.
   bool SetFailed() override;
 
-  // Callbacks from the GIF reader.
-  bool HaveDecodedRow(size_t frame_index,
-                      GIFRow::const_iterator row_begin,
-                      size_t width,
-                      size_t row_number,
-                      unsigned repeat_count,
-                      bool write_transparent_pixels);
-  bool FrameComplete(size_t frame_index);
-
-  // For testing.
-  bool ParseCompleted() const;
+  size_t ClearCacheExceptFrame(size_t) override;
 
  private:
   // ImageDecoder:
-  void ClearFrameBuffer(size_t frame_index) override;
-  virtual void DecodeSize() { Parse(kGIFSizeQuery); }
+  void DecodeSize() override {}
   size_t DecodeFrameCount() override;
-  void InitializeNewFrame(size_t) override;
-  void Decode(size_t) override;
-
-  // Parses as much as is needed to answer the query, ignoring bitmap
-  // data. If parsing fails, sets the "decode failure" flag.
-  void Parse(GIFParseQuery);
-
-  // Reset the alpha tracker for this frame. Before calling this method, the
-  // caller must verify that the frame exists.
-  void OnInitFrameBuffer(size_t) override;
-
+  void InitializeNewFrame(size_t index) override;

[scroggo_chromium, 2017/07/18 21:10:10]

nit: It's been recommended to me to leave "index" out of these declarations. We
were already inconsistent about this, though.

[cblume, 2017/07/19 23:27:05]

Yeah. For example, a few lines above I leave it out for ClearCacheExceptFrame().
I'm personally not a fan of that. The parameter type is "size_t" -- maybe that
means initialize a new frame of this many bytes. I need to go in and figure out
that it actually means index. This is the exact problem that the variable name
solves. :D

There are times that leaving it out makes the code easier to read. Imagine it
was:
void InitializeNewFrame(ImageFrame& frame);
That name "frame" is useless.

But I'm really torn on the index here.

I'll remove it if that's what we want. But I just want to state that I think it
is useful. :)

[scroggo_chromium, 2017/07/20 13:59:52]

I don't have a strong preference.

[cblume, 2017/07/20 21:53:44]

I'll leave variable name out for now.
I thought about this and it is kind of a mixed bag. I originally thought I might
put it back where it isn't clear.

Sometimes it is clear, for example:
FrameIsReceivedAtIndex(size_t)
FrameDurationAtIndex(size_t)

Sometimes it isn't clear but can be figured out:
ClearCacheExceptFrame(size_t) (it takes a size_t, not a frame)
CanReusePreviousFrameBuffer(size_t)
GetViableReferenceFrameIndex(size_t)

Other times, size_t doesn't always mean index. For example:
InitializeNewFrame(size_t) (like we said before)

I'm prepared to explain if someone questions it, but only adding it to one
location feels weird. Adding it to the not-clear-but-can-be-figured-out
locations could be argued as unimportant. I'll just leave it out.

For future reference, the most amazing solution would be taking typedef size_t
FrameIndex.

+  void Decode(size_t index) override;
   // When the disposal method of the frame is DisposeOverWritePrevious, the
-  // next frame will use the previous frame's buffer as its starting state, so
+  // next frame will use a previous frame's buffer as its starting state, so
   // we can't take over the data in that case. Before calling this method, the
   // caller must verify that the frame exists.
   bool CanReusePreviousFrameBuffer(size_t) const override;
 
-  bool current_buffer_saw_alpha_;
-  mutable int repetition_count_;
-  std::unique_ptr<GIFImageReader> reader_;
+  // When a frame depends on a previous frames's content, there is a list of
+  // candidate reference frames. This function will find a previous frame from
+  // that list which satisfies the requirements of being a reference frame
+  // (kFrameComplete, not kDisposeOverwritePrevious).
+  // If no frame is found, it returns kNotFound.
+  size_t GetViableReferenceFrameIndex(size_t dependent_index) const;
+
+  std::unique_ptr<SkCodec> codec_;
+  // |codec_| owns the SegmentStream, but we need a reference to it to append
+  // more data as it arrives.
+  SegmentStream* segment_stream_;
 };
 
 }  // namespace blink