Move gif image decoder to SkCodec

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) 2008, 2009 Google, Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 85 matching lines @@
   DCHECK_NE(this, other);
   if (other->m_bitmap.isImmutable())
     return false;
   m_hasAlpha = other->m_hasAlpha;
   m_bitmap.reset();
   m_bitmap.swap(other->m_bitmap);
   other->m_status = FrameEmpty;
   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(
       newWidth, newHeight,
       m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,
       std::move(colorSpace)));
   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;
 }
 
 bool ImageFrame::hasAlpha() const {
   return m_hasAlpha;
 }
 
 sk_sp<SkImage> ImageFrame::finalizePixelsAndGetImage() {
   DCHECK_EQ(FrameComplete, m_status);
@@ skipping 77 matching lines @@
   // If the frame is not fully loaded, there will be transparent pixels,
   // so we can't tell skia we're opaque, even for image types that logically
   // always are (e.g. jpeg).
   if (!m_hasAlpha && m_status == FrameComplete)
     return kOpaque_SkAlphaType;
 
   return m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType;
 }
 
 }  // namespace blink