Bug 90505 – Set the access qualifier of two methods to query frame specific info of BitmapImage to protected.

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Dongseong Hwang]

Set the access qualifier of two methods about querying frame specific info of BitmapImage protected.

Following 4 methods are protected.
  size_t frameCount();
  NativeImagePtr frameAtIndex(size_t);
  bool frameIsCompleteAtIndex(size_t);
  float frameDurationAtIndex(size_t);

So, 2 methds also should be protected.
  bool frameHasAlphaAtIndex(size_t);
  ImageOrientation frameOrientationAtIndex(size_t);

[Dongseong Hwang]

Created attachment 150688 [details]
patch

[Dongseong Hwang]

Created attachment 150689 [details]
patch v.2

Correct grammar of title.

[Alexey Proskuryakov]

Could you please elaborate why this needs to be done? Clearly, this is not a build fix.

-    bool hasAlpha = image->isBitmapImage() ? static_cast<BitmapImage*>(image)->frameHasAlphaAtIndex(0) : true;
+   bool hasAlpha = image->isBitmapImage() ? image->currentFrameHasAlpha() : true;

This looks suspicious - are frame 0 and current one the same?

[Dongseong Hwang]

(In reply to comment #3)
> Could you please elaborate why this needs to be done? Clearly, this is not a build fix.
> 
> -    bool hasAlpha = image->isBitmapImage() ? static_cast<BitmapImage*>(image)->frameHasAlphaAtIndex(0) : true;
> +   bool hasAlpha = image->isBitmapImage() ? image->currentFrameHasAlpha() : true;
> 
> This looks suspicious - are frame 0 and current one the same?

PNG, JPEG, BMP, and WEBP's current is always 0.

GIF and ICO can have an other integer as current index.

It means that previous implementation has bug about alpha because the impl expects the alpha of image->nativeImageForCurrentFrame() is image->frameHasAlphaAtIndex(0).

[Dongseong Hwang]

(In reply to comment #4)
> (In reply to comment #3)
> > Could you please elaborate why this needs to be done? Clearly, this is not a build fix.
> > 
> > -    bool hasAlpha = image->isBitmapImage() ? static_cast<BitmapImage*>(image)->frameHasAlphaAtIndex(0) : true;
> > +   bool hasAlpha = image->isBitmapImage() ? image->currentFrameHasAlpha() : true;
> > 
> > This looks suspicious - are frame 0 and current one the same?
> 
> PNG, JPEG, BMP, and WEBP's current is always 0.
> 
> GIF and ICO can have an other integer as current index.
> 
> It means that previous implementation has bug about alpha because the impl expects the alpha of image->nativeImageForCurrentFrame() is image->frameHasAlphaAtIndex(0).

Is it better that I comment it on Changelog or CPP file?

[Alexey Proskuryakov]

> Is it better that I comment it on Changelog or CPP file?

ChangeLog.

As you're fixing a bug, the obvious question is - can it be tested?

[Dongseong Hwang]

Created attachment 151186 [details]
patch v.3

[Dongseong Hwang]

(In reply to comment #6)
> > Is it better that I comment it on Changelog or CPP file?
> 
> ChangeLog.
I added the explanation in ChangeLog.
> 
> As you're fixing a bug, the obvious question is - can it be tested?
Unfortunately, I could not verify my statement by test.
I mentioned CG port possibly had a bug after reading code, not testing.
I tried to make test it but it is hard to get the animated GIF that has the frame with alpha channel and another frame without alpha channel.
And, most animated GIF is with alpha channel or without channel, not hybrid, so previous implementation rarely show the defect in real sites or tests.

IMHO, changing frameHasAlphaAtIndex(0) to currentFrameHasAlpha() is proper and easy to understand, but if it is not enough due to the lack of test, I'll try to make the test again.

[Eric Seidel (no email)]

Comment on attachment 151186 [details]
patch v.3

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

> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:103
> -    bool hasAlpha = image->isBitmapImage() ? static_cast<BitmapImage*>(image)->frameHasAlphaAtIndex(0) : true;
> +    bool hasAlpha = image->isBitmapImage() ? image->currentFrameHasAlpha() : true;

Why is this the right thing for gif and other multi-frame images?

[Eric Seidel (no email)]

Comment on attachment 151186 [details]
patch v.3

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

>> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:103
>> +    bool hasAlpha = image->isBitmapImage() ? image->currentFrameHasAlpha() : true;
> 
> Why is this the right thing for gif and other multi-frame images?

Do we end up using the current frame anyway?

[Eric Seidel (no email)]

Comment on attachment 151186 [details]
patch v.3

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

>>> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:103
>>> +    bool hasAlpha = image->isBitmapImage() ? image->currentFrameHasAlpha() : true;
>> 
>> Why is this the right thing for gif and other multi-frame images?
> 
> Do we end up using the current frame anyway?

Ah, I've now read your ChangeLog, sorry.

[WebKit Review Bot]

Comment on attachment 151186 [details]
patch v.3

Clearing flags on attachment: 151186

Committed r125374: <http://trac.webkit.org/changeset/125374>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.