Refactor ImageFrame::setSizeAndColorSpace()

third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
index 641012756bc353bc3cce7b30e28e29cc6acd17a1..3e8750ebbecfd1f115a263205f99f395cb9586b4 100644
--- a/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
@@ -941,6 +941,8 @@ bool JPEGImageDecoder::outputScanlines() {
                                      colorSpaceForSkImages()))
       return setFailed();
 
+    buffer.zeroFillPixelData();

[Noel Gordon, 2017/03/14 06:30:41]

Here and elsewhere you made this change, consider using

buffer.zeroFillPixelData();
DCHECK(buffer.hasAlpha());

to maybe remind everyone of that important contract?  (Since it so often comes
as a discussion point in image decoder changes).

[Peter Kasting, 2017/03/14 07:34:40]

Hmm.

It worries me that zeroFillPixelData() sets m_hasAlpha = true, but setHasAlpha()
also calls m_bitmap.setAlphaType().  DCHECK(buffer.hasAlpha()) implies one
doesn't need to call setHasAlpha(true), as we currently do e.g. below.  But
unless the eraseARGB() call in zeroFillPixelData() does this automatically,
omitting that will subtly modify the effect of the code.

I wonder if zeroFillPixelData() should be calling setHasAlpha(true) instead of
just twiddling m_hasAlpha.

[cblume, 2017/03/14 17:30:50]

I think both of you had really good points.
I would be happy to remind people that zero filling means we're also talking
about alpha.
But I also don't want to mislead people into thinking they don't need to call a
function which is required.

I don't immediately see a reason why we set m_hasAlpha = true (inside
zeroFillPixelData()) and don't just call setHasAlpha(true). Is there a reason to
set m_hasAlpha = true without also calling m_bitmap.setAlphaType() ?

I feel like it would be lovely if zeroFillPixelData() called
m_bitmap.setAlphaType() and we could get rid of the call to
buffer.setHasAlpha(true) below. But there might be a reason we cannot do that.

+
     // The buffer is transparent outside the decoded area while the image is
     // loading. The image will be marked fully opaque in complete().
     buffer.setStatus(ImageFrame::FramePartial);