Move gif image decoder to SkCodec

third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp

Index: third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
index 40952951b85fb16987b40f5a4ab85f7c284ea00a..c9c207b434b6543acc31b75f49943b6db2a817e2 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
@@ -103,11 +103,12 @@ bool ImageFrame::takeBitmapDataIfWritable(ImageFrame* other) {
   return true;
 }
 
-bool ImageFrame::setSizeAndColorSpace(int newWidth,
-                                      int newHeight,
-                                      sk_sp<SkColorSpace> colorSpace) {
-  // setSizeAndColorSpace() should only be called once, it leaks memory
-  // otherwise.
+bool ImageFrame::setSizeAndColorSpaceWithoutZeroFilling(
+    int newWidth,
+    int newHeight,
+    sk_sp<SkColorSpace> colorSpace) {

[scroggo_chromium, 2017/03/13 20:36:40]

Why not add a boolean (or an enum) for zeroFill?

[cblume, 2017/03/13 21:53:12]

I am happy to make it 1 function with a parameter if you want.

I actually prefer this way because it keeps functions as small and simple as
possible. It keeps the mental burden low the number of cases small. For example
2 parameters (a bool and an enum with 5 values) have 10 total cases. As separate
functions that would have been 7 cases.

The book Clean Code talks about how a function should do only 1 thing, and
specifically calls out flag parameters as an example where the function no
longer does one thing.

To quote:
"Mousing over the call and seeing render(boolean isSuite) helps a little, but
not that much. We should have split the function into two: renderForSuite() and
renderForSingleTest()."

This sentiment is mirrored in the C++ Core Guidelines in:
- F.2: A function should perform a single logical operation
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-namerf-logicala...
- F.3: Keep functions short and simple
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-namerf-singleaf...



If I am honest though, I think I split these poorly.
setSizeAndColorSpace still does two things. It sets the size/color space and
zero fills.

I think a better option would be to have setSizeAndColorSpace NOT zero fill and
update all the call sites to also call ImageFrame::zeroFillPixelData().

[cblume, 2017/03/16 11:03:30]

I made a separate pull request that I think refactors
ImageFrame::setSizeAndColorSpace better.
https://codereview.chromium.org/2749703002/
I'm happy to add a parameter if you feel that is better. But I think this new PR
fixes the issue where these 2 functions felt weird -- I think this PR feels more
natural.

+  // Either setSizeAndColorSpaceWithoutZeroFilling() or setSizeAndcolorSpace()
+  // should only be called once. It leaks memory otherwise.

[scroggo_chromium, 2017/03/13 20:36:40]

This is a modified version of an existing comment, but I don't think it is true
or has been for a long time. Pixel memory is owned by an SkPixelRef (or in the
case of the ExternalMemoryAllocator, owned externally), so when we unref the old
one, it will delete the pixel memory (or ignored for ExternalMemoryAllocator).
Nothing else looks like it should leak memory. It would be wasteful and get rid
of any existing progress, though.

[cblume, 2017/03/13 21:53:12]

Okay. If it is safe then I think this comment should be deleted.

When we unref the old data it gets ignored for ExternalMemoryAllocator? Does
that mean it would leak in that case? I'm not sure I understood.

[scroggo_chromium, 2017/03/14 16:08:48]

The idea of the ExternalMemoryAllocator is that someone else (externally) owns
that memory. In practice, it is owned by discardable memory somewhere.

   DCHECK(!width() && !height());
 
   m_bitmap.setInfo(SkImageInfo::MakeN32(
@@ -117,6 +118,17 @@ bool ImageFrame::setSizeAndColorSpace(int newWidth,
   if (!m_bitmap.tryAllocPixels(m_allocator, 0))
     return false;

[scroggo_chromium, 2017/03/13 20:36:40]

I don't like having two separate methods for this, but as-is, this can be:

  return m_bitmap.tryAllocPixels(m_allocator, 0);

[cblume, 2017/03/13 21:53:12]

Done.

 
+  return true;
+}
+
+bool ImageFrame::setSizeAndColorSpace(int newWidth,
+                                      int newHeight,
+                                      sk_sp<SkColorSpace> colorSpace) {
+  // Either setSizeAndColorSpaceWithoutZeroFilling() or setSizeAndcolorSpace()
+  // should only be called once. It leaks memory otherwise.
+  if (!setSizeAndColorSpaceWithoutZeroFilling(newWidth, newHeight, colorSpace))
+    return false;
+
   zeroFillPixelData();
   return true;
 }