Refactor ImageFrame::setSizeAndColorSpace()

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..9fde4bd8f3fdc0a717ecaff793dd0d5a466bb91f 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
@@ -78,8 +78,8 @@ void ImageFrame::clearPixelData() {
 }
 
 void ImageFrame::zeroFillPixelData() {
+  setHasAlpha(true);

[Noel Gordon, 2017/03/16 12:01:50]

Harmless, but also redundant.  It sets the m_bitmap alpha type to 

m_premultiplyAlpha ? kPremul_SkAlphaType : kUnpremul_SkAlphaType

aka the bitmap backing store has alpha.  But allocatePixelData already did that
@line 111 below.  No real need to do it twice.

[cblume, 2017/03/17 05:35:14]

Wait....you're right.
I could have sworn it made perfect sense just yesterday.

All the existing call sites call allocatePixelData() before calling
zeroFillPixelData().

The other call site I care about which isn't shown here is the reason I created
this CR: So I can call setSizeAndColorSpace() without zero-filling. In that
case, I am not calling zeroFillPixelData().


I don't think there is a case where we need to do anything with the alpha inside
zeroFillPixelData(). I think we could have just removed the m_hasAlpha = true;
line. And I think we saw it and thought "Probably should be calling the
function."

I'll go ahead and remove it.

[Peter Kasting, 2017/03/17 06:35:50]

Because the BMP decoder needs it, no?

It allocates and zeros the pixel data and then explicitly calls
setHasAlpha(false).

Later when it calls zeroFillPixelData() again it expects that to mark the image
as having alpha.

I don't think this can be removed, because zeroFillPixelData() is not called
only at image creation time.

[cblume, 2017/03/17 18:05:45]

It is now being explicitly set in BMPImageReader.cpp:827 with

m_buffer->zeroFillPixelData();
m_buffer->setHasAlpha(true);

since this was the only location that needed both and the concepts are separate.

[Noel Gordon, 2017/03/20 13:24:52]

Not sure they're separate concepts.  Somewhat odd to me to zero the data and
then not set m_alpha = true.  (And then Peter spotted the BMP bug that would
have resulted).  I think the body should read:

m_bitmap.eraseARGB(0, 0, 0, 0);
m_hasAlpha = true;

as it was in diff-left, since your goal here was to just split the calling of
this function out, without change behavior :)

That way you wouldn't need to touch the BMP decoder, and the comments therein
(near line@127) seem to describe what is expected pretty well?

   m_bitmap.eraseARGB(0, 0, 0, 0);
-  m_hasAlpha = true;
 }
 
 bool ImageFrame::copyBitmapData(const ImageFrame& other) {
@@ -103,22 +103,14 @@ 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.
-  DCHECK(!width() && !height());
-
+bool ImageFrame::allocatePixelData(int newWidth,
+                                   int newHeight,
+                                   sk_sp<SkColorSpace> colorSpace) {
   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;
-
-  zeroFillPixelData();
-  return true;
+  return m_bitmap.tryAllocPixels(m_allocator, 0);
 }
 
 bool ImageFrame::hasAlpha() const {