Bug 171410 - Unify how BitmapImage handles the availability of a decoded for large and animated images
Summary: Unify how BitmapImage handles the availability of a decoded for large and ani...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on:
Blocks: 170640 171016
  Show dependency treegraph
 
Reported: 2017-04-27 17:56 PDT by Said Abou-Hallawa
Modified: 2017-04-28 15:38 PDT (History)
3 users (show)

See Also:


Attachments
Patch (17.33 KB, patch)
2017-04-27 18:17 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.80 KB, patch)
2017-04-28 12:45 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.91 KB, patch)
2017-04-28 13:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-04-27 17:56:27 PDT
-- 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.
Comment 1 Said Abou-Hallawa 2017-04-27 18:17:22 PDT
Created attachment 308487 [details]
Patch
Comment 2 Said Abou-Hallawa 2017-04-28 08:58:05 PDT
This is work towards https://bugs.webkit.org/show_bug.cgi?id=170640 and https://bugs.webkit.org/show_bug.cgi?id=171016.
Comment 3 Simon Fraser (smfr) 2017-04-28 10:53:11 PDT
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
Comment 4 Said Abou-Hallawa 2017-04-28 12:45:59 PDT
Created attachment 308577 [details]
Patch
Comment 5 Said Abou-Hallawa 2017-04-28 13:05:28 PDT
Created attachment 308584 [details]
Patch
Comment 6 Said Abou-Hallawa 2017-04-28 15:15:16 PDT
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.
Comment 7 WebKit Commit Bot 2017-04-28 15:37:59 PDT
Comment on attachment 308584 [details]
Patch

Clearing flags on attachment: 308584

Committed r215952: <http://trac.webkit.org/changeset/215952>
Comment 8 WebKit Commit Bot 2017-04-28 15:38:01 PDT
All reviewed patches have been landed.  Closing bug.