[Webkit-unassigned] [Bug 244128] Add experimental feature to disable Bitmap image animations

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=244128

--- Comment #51 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 462169
  --> https://bugs.webkit.org/attachment.cgi?id=462169
Patch

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

> Source/WebCore/dom/Document.cpp:9285
> +bool Document::isImageURLAllowedToAnimate(const URL& url) const
> +{
> +    return m_imageURLsAllowedToAnimate.contains(url);
> +}
> +
> +void Document::addAllowedAnimatedImageURL(const URL& url)
> +{
> +    m_imageURLsAllowedToAnimate.add(url);
> +}
> +
> +void Document::removeAllowedAnimatedImageURL(const URL& url)
> +{
> +    m_imageURLsAllowedToAnimate.remove(url);
> +}

This code is not needed. Please remove.

> Source/WebCore/dom/Document.h:1704
> +    bool isImageURLAllowedToAnimate(const URL&) const;
> +    void addAllowedAnimatedImageURL(const URL&);
> +    void removeAllowedAnimatedImageURL(const URL&);

Ditto.

> Source/WebCore/dom/Document.h:2312
> +    ListHashSet<URL> m_imageURLsAllowedToAnimate;

Ditto.

> Source/WebCore/html/HTMLImageElement.cpp:800
> +bool HTMLImageElement::isAnimating() const
> +{
> +    if (auto* image = this->image())
> +        return image->isAnimating();
> +    return false;
> +}

This function is not used. Please remove.

> Source/WebCore/html/HTMLImageElement.cpp:998
> +bool HTMLImageElement::isAnimatedImage() const
> +{
> +    if (auto* image = this->image())
> +        return image->isAnimated();
> +    return false;
> +}

This function is not used. Please remove.

> Source/WebCore/html/HTMLImageElement.h:163
> +    bool isAnimating() const;
> +    bool isAnimatedImage() const;

Please remove.

> Source/WebCore/html/ImageBitmap.cpp:639
> +    bool allowsAnimation() override { return true; }

This can be removed if ImageObserver::allowsAnimation() returns true.

> Source/WebCore/loader/cache/CachedImage.cpp:450
> +bool CachedImage::CachedImageObserver::allowsAnimation()

This function should take an Image as an input. See CachedImage::CachedImageObserver::canDestroyDecodedData() as an example.

> Source/WebCore/loader/cache/CachedImage.cpp:712
> +bool CachedImage::allowsAnimation()

This function should take an Image as an input. See CachedImage::canDestroyDecodedData() as an example.

> Source/WebCore/platform/graphics/BitmapImage.cpp:377
> +    return repetitionCount() && !m_animationFinished && imageObserver() && imageObserver()->allowsAnimation();

*this should be passed to allowsAnimation() here.

> Source/WebCore/platform/graphics/ImageObserver.h:56
> +    virtual bool allowsAnimation() = 0;

I think this should return true instead of having it a pure virtual function. I think all images should allow animation by default.

Also this function should take an Image as an input. See canDestroyDecodedData() above as an example.

> Source/WebCore/rendering/RenderImage.cpp:451
> +}

How about this?

    if (auto* image = cachedImage() ? cachedImage()->image() : nullptr)
        return image->isAnimated();
    return false;

Also I think isAnimatedImage() is not a correct name. I think it should be hasAnimatedImage() or something else. RenderImage is not an "image" per se. It just holds an "image".

> Source/WebCore/rendering/RenderView.cpp:879
> +    for (auto& element : descendantsOfType<RenderImage>(*this)) {

Getting the RenderImage only will not repaint elements with CSS background images.

> Tools/TestWebKitAPI/Tests/WebCore/SVGImageCasts.cpp:81
> +    bool allowsAnimation() final
> +    {
> +        return true;
> +    }

This can be removed if ImageObserver::allowsAnimation() returns true.

> LayoutTests/accessibility/mac/disable-animations-enabled-expected.txt:1
> +This tests that animations are disabled when the disable animated images setting is turned on.

I think these tests should be moved to fast/images directory.

> LayoutTests/platform/mac-wk1/TestExpectations:459
> +# Image animation disabled (experimental feature)
> +accessibility/mac/disable-animations-enabled.html [ Skip ]
> +accessibility/mac/disable-animations-play-individual-animation.html [ Skip ]
> +accessibility/mac/disable-animations-resuming-frame.html [ Skip ]

I think these tests should work for WK1 as well. Please remove these skip lines.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220907/503fef6d/attachment.htm>