Bug 171410 – Unify how BitmapImage handles the availability of a decoded for large and animated images

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

-- Do the following renaming:

ImageObserver::animationAdvanced() => ImageObserver::imageFrameAvailable()
CachedImage::CachedImageObserver::animationAdvanced() => CachedImage::CachedImageObserver::imageFrameAvailable()
CachedImage::animationAdvanced() => CachedImage::imageFrameAvailable()
CachedImageClient::newImageAnimationFrameAvailable() => CachedImageClient::imageFrameAvailable()
RenderElement::newImageAnimationFrameAvailable() => RenderElement::imageFrameAvailable()

-- Make BitmapImage calls ImageObserver::imageFrameAvailable() whenever a  frame is available regardless it is for an animated or for a large image.

-- The purpose of this change is to make it easy, from now on, to handle finishing the async decoding more easily.

[Said Abou-Hallawa]

Created attachment 308487 [details]
Patch

[Said Abou-Hallawa]

This is work towards https://bugs.webkit.org/show_bug.cgi?id=170640 and https://bugs.webkit.org/show_bug.cgi?id=171016.

[Simon Fraser (smfr)]

Comment on attachment 308487 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Make BitmapImage calls ImageObserver::imageFrameAvailable() whenever a 

calls -> call

> Source/WebCore/loader/cache/CachedImageClient.h:43
> +    virtual bool imageFrameAvailable(CachedImage& image, bool, const IntRect* changeRect) { imageChanged(&image, changeRect); return false; }

Please add a comment saying what the return value means, or return a self-explanatory enum.

> Source/WebCore/platform/graphics/BitmapImage.cpp:414
> +        imageObserver()->imageFrameAvailable(this, true);

boolean trap. Use an enum?

> Source/WebCore/platform/graphics/BitmapImage.cpp:460
> +        imageObserver()->imageFrameAvailable(this, false);

boolean trap.

> Source/WebCore/rendering/RenderElement.cpp:1508
> +    // Large images should repaint even if they are outside the viewport rectangle
> +    // because they should be inside the TileCoverageRect.
> +    if (shouldRepaint || (!isAnimating && image.image() && image.image()->isBitmapImage()))
> +        imageChanged(&image, changeRect);

This seems like a behavior change. Can it be tested?

> Source/WebCore/svg/graphics/SVGImageClients.h:55
> +        // If m_image->m_page is null, we're being destructed.

destructed -> destroyed

[Said Abou-Hallawa]

Created attachment 308577 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308584 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 308487 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Make BitmapImage calls ImageObserver::imageFrameAvailable() whenever a 
> 
> calls -> call

Fixed.

>> Source/WebCore/loader/cache/CachedImageClient.h:43
>> +    virtual bool imageFrameAvailable(CachedImage& image, bool, const IntRect* changeRect) { imageChanged(&image, changeRect); return false; }
> 
> Please add a comment saying what the return value means, or return a self-explanatory enum.

I added the enum class VisibleInViewportState { Unknown, Yes, No } in CachedImageClient.h. I made this function returns VisibleInViewportState. I replaced RenderElement::VisibleInViewportState with this enum class.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:414
>> +        imageObserver()->imageFrameAvailable(this, true);
> 
> boolean trap. Use an enum?

I added enum class ImageAnimatingState { Yes, No } in ImageTypes and I made this function takes ImageAnimatingState instead of the bool isAnimating.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:460
>> +        imageObserver()->imageFrameAvailable(this, false);
> 
> boolean trap.

Ditto.

>> Source/WebCore/rendering/RenderElement.cpp:1508
>> +        imageChanged(&image, changeRect);
> 
> This seems like a behavior change. Can it be tested?

You are right. This will be a behavior change for SVGImageClients in the case of non animating image. I fixed the condition to be not specific about BitmapImage.

>> Source/WebCore/svg/graphics/SVGImageClients.h:55
>> +        // If m_image->m_page is null, we're being destructed.
> 
> destructed -> destroyed

Fixed.

[WebKit Commit Bot]

Comment on attachment 308584 [details]
Patch

Clearing flags on attachment: 308584

Committed r215952: <http://trac.webkit.org/changeset/215952>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.