Refactor ImageFrame::setSizeAndColorSpace()

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
index 5b5eb8aa93cbe7a65d594895364f740b71297ded..077406cefee5d0ca72e10994b47894d56751bb3b 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.h
@@ -119,10 +119,9 @@ class PLATFORM_EXPORT ImageFrame final {
   }
 
   // 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; }