Bug 90505

Summary: Set the access qualifier of two methods to query frame specific info of BitmapImage to protected.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: ImagesAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, eric, noel.gordon, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch v.2
none
patch v.3 none

Description Dongseong Hwang 2012-07-03 16:20:52 PDT
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);
Comment 1 Dongseong Hwang 2012-07-03 16:23:11 PDT
Created attachment 150688 [details]
patch
Comment 2 Dongseong Hwang 2012-07-03 16:34:55 PDT
Created attachment 150689 [details]
patch v.2

Correct grammar of title.
Comment 3 Alexey Proskuryakov 2012-07-04 13:00:11 PDT
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?
Comment 4 Dongseong Hwang 2012-07-06 20:57:06 PDT
(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).
Comment 5 Dongseong Hwang 2012-07-06 20:58:51 PDT
(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?
Comment 6 Alexey Proskuryakov 2012-07-06 23:50:05 PDT
> 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?
Comment 7 Dongseong Hwang 2012-07-08 22:45:31 PDT
Created attachment 151186 [details]
patch v.3
Comment 8 Dongseong Hwang 2012-07-08 22:56:04 PDT
(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.
Comment 9 Eric Seidel (no email) 2012-08-12 03:47:54 PDT
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?
Comment 10 Eric Seidel (no email) 2012-08-12 03:48:13 PDT
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?
Comment 11 Eric Seidel (no email) 2012-08-12 03:48:40 PDT
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.
Comment 12 WebKit Review Bot 2012-08-12 05:13:11 PDT
Comment on attachment 151186 [details]
patch v.3

Clearing flags on attachment: 151186

Committed r125374: <http://trac.webkit.org/changeset/125374>
Comment 13 WebKit Review Bot 2012-08-12 05:13:15 PDT
All reviewed patches have been landed.  Closing bug.