Bug 90445 – Add a comment in order to clarify why BitmapImage::frameHasAlphaAtIndex returns true as default.

• Component: Images
• Status: RESOLVED FIXED

[Dongseong Hwang]

Add an assertion and a comment in order to clarify the states of BitmapImage.

[Dongseong Hwang]

Created attachment 150577 [details]
patch

[Dongseong Hwang]

Comment on attachment 150577 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150577&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:312
> +    ASSERT(m_frames[index].m_isComplete);

All case that call this method is guarded by if(isComplete).

> Source/WebCore/platform/graphics/BitmapImage.cpp:326
> +    // black.

The comment was copied from ImageSource::frameHasAlphaAtIndex(size_t index)

[Dongseong Hwang]

Created attachment 150579 [details]
patch v.2

[Dongseong Hwang]

Comment on attachment 150579 [details]
patch v.2

View in context: https://bugs.webkit.org/attachment.cgi?id=150579&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:322
> +    // When a frame has not finished decoding, always mark it as having alpha.

The comment was copied from ImageSource::frameHasAlphaAtIndex(size_t index)

[Dongseong Hwang]

(In reply to comment #0)
> Add an assertion and a comment in order to clarify the states of BitmapImage.

I removed the assertion in patch v.2 because I misunderstood at the time to patch v.1.

[Dongseong Hwang]

(In reply to comment #4)
> (From update of attachment 150579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150579&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:322
> > +    // When a frame has not finished decoding, always mark it as having alpha.
> 
> The comment was copied from ImageSource::frameHasAlphaAtIndex(size_t index)

The comment of ImageSource::frameHasAlphaAtIndex is long, so I pasted just summary of the comment to BitmapImage::frameHasAlphaAtIndex.

bool ImageSource::frameHasAlphaAtIndex(ImageFrame* buffer)
{
    // When a frame has not finished decoding, always mark it as having alpha.
    // Ports that check the result of this function to determine their
    // compositing op need this in order to not draw the undecoded portion as
    // black.
    // TODO: Perhaps we should ensure that each individual decoder returns true
    // in this case.
    return !frameIsCompleteAtIndex(buffer) || buffer->hasAlpha();
}

[Eric Seidel (no email)]

Comment on attachment 150579 [details]
patch v.2

View in context: https://bugs.webkit.org/attachment.cgi?id=150579&action=review

>>> Source/WebCore/platform/graphics/BitmapImage.cpp:322
>>> +    // When a frame has not finished decoding, always mark it as having alpha.
>> 
>> The comment was copied from ImageSource::frameHasAlphaAtIndex(size_t index)
> 
> The comment of ImageSource::frameHasAlphaAtIndex is long, so I pasted just summary of the comment to BitmapImage::frameHasAlphaAtIndex.
> 
> bool ImageSource::frameHasAlphaAtIndex(ImageFrame* buffer)
> {
>     // When a frame has not finished decoding, always mark it as having alpha.
>     // Ports that check the result of this function to determine their
>     // compositing op need this in order to not draw the undecoded portion as
>     // black.
>     // TODO: Perhaps we should ensure that each individual decoder returns true
>     // in this case.
>     return !frameIsCompleteAtIndex(buffer) || buffer->hasAlpha();
> }

You should add a line that said:
// See ImageSource::framehasAlphaAtIndex for explanation of why incomplete images claim to have alpha.

[Dongseong Hwang]

Created attachment 150704 [details]
patch v.3

Absolutely!

[WebKit Review Bot]

Comment on attachment 150704 [details]
patch v.3

Clearing flags on attachment: 150704

Committed r121827: <http://trac.webkit.org/changeset/121827>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.