Refactor ImageFrame::setSizeAndColorSpace()

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  * 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 101 matching lines @@
     DCHECK_LE(endX, width());
     DCHECK_LT(startY, height());
     DCHECK_LE(endY, height());
     const int rowBytes = (endX - startX) * sizeof(PixelData);
     const PixelData* const startAddr = getAddr(startX, startY);
     for (int destY = startY + 1; destY < endY; ++destY)
       memcpy(getAddr(startX, destY), startAddr, rowBytes);
   }
 
   // Allocates space for the pixel data.  Must be called before any pixels
-  // are written.  Must only be called once. The specified color space may
-  // be nullptr if and only if color correct rendering is enabled. Returns
-  // whether allocation succeeded.
-  bool setSizeAndColorSpace(int newWidth, int newHeight, sk_sp<SkColorSpace>);
+  // are written.  The specified color space may be nullptr if and only if
+  // color correct rendering is enabled. Returns whether allocation succeeded.
+  bool allocateBackingStore(int newWidth, int newHeight, sk_sp<SkColorSpace>);

[Noel Gordon, 2017/03/15 13:42:51]

Better, and re-reading the comment here for this API suggests another possible
name: allocatePixelData(). Maybe if  

 - we allocateBackingStore() we should zeroFillBackingStore()
 - we allocatePixelData() we should zeroFillPixelData()

since their use is usually adjacent in the code with this CL.  The reader of
that code should maybe not have to think about: "What's the diff b/w a
BackingStore and a PixelData?" :)  Thoughts?

Comment "Must only be called once." is removed compared to diff-left.  Should
this API really be promoting multiple calls?  The DCHECK in its body seems to
say otherwise: DCHECK(!width() && !height()) [1].

[1] width() and height() here being the SkBitmap width() as height() as Leon
pointed out (to me).

[cblume, 2017/03/16 02:29:25]

I think allocatePixelData() is a much better name. I'll go with that. Good call.


Also, you're right about the DCHECK. I removed the comment because it was not
correct -- it would not leak memory if called multiple times.

We still shouldn't call it multiple times per frame because that would just
deallocate and allocate again. However, I think the DCHECK is now a bit
paranoid. Elsewhere in the codebase we don't have DCHECKs to prevent this sort
of reallocation. So even though protection is good, I think this might be a bit
heavy on the defensive programming. I think we should perhaps get rid of the
DCHECKs now.

[Noel Gordon, 2017/03/16 11:34:04]

I'm good with this part of the change: allocatePixelData() seems fine and fits
well with ZeroFillPixelData(), however, ...
The comment was to be fixed, not removed.  It was wrong about the leaking memory
part and that should go for sure ...
 
So "deallocate and allocate again" is not what we want, so let's document that.
I think the comment from diff-left should be retained in some form, something
like 

// Allocates space for the pixel data. Must be called before any pixels are
// written, and should be only be called once. The specified color space may
// be null if and only if color correct rendering is enabled. Returns true
// if the allocation succeeded.
What other areas of the codebase do is interesting, but the DCHECK in the
ImageFrame .cpp file regarding the width() and height() of the m_bitmap should
be retained to document the code contract.

That is not paranoid imho, it simply states what the .cpp code expects (don't
call me twice, which we seem to agree on as the goal, since any re-allocation
would suggest misuse of this API to me).
Don't agree, see above.

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

Done.
Done.

 
   bool hasAlpha() const;
   const IntRect& originalFrameRect() const { return m_originalFrameRect; }
   Status getStatus() const { return m_status; }
   unsigned duration() const { return m_duration; }
   DisposalMethod getDisposalMethod() const { return m_disposalMethod; }
   AlphaBlendSource getAlphaBlendSource() const { return m_alphaBlendSource; }
   bool premultiplyAlpha() const { return m_premultiplyAlpha; }
   SkBitmap::Allocator* allocator() const { return m_allocator; }
 
@@ skipping 156 matching lines @@
   // The frame that must be decoded before this frame can be decoded.
   // WTF::kNotFound if this frame doesn't require any previous frame.
   // This is used by ImageDecoder::clearCacheExceptFrame(), and will never
   // be read for image formats that do not have multiple frames.
   size_t m_requiredPreviousFrameIndex;
 };
 
 }  // namespace blink
 
 #endif