[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 #45 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 462061
  --> https://bugs.webkit.org/attachment.cgi?id=462061
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:995
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (cachedImage->image()->isAnimated())
> +            return true;
> +    }

if (auto* image = this->image())
    return image->isAnimated();
return false;

> Source/WebCore/loader/cache/CachedImage.cpp:453
> +        if (cachedImage->allowsAnimation(sourceUrl()))

Why do we pass sourceUrl() to the CachedImage? We get the sourceUrl() from the CachedImage. I think this should be 

if (cachedImage->allowsAnimation())

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

You do not have to pass a sourceURL. You can use url() which is defined in the base class CachedResource.

> Source/WebCore/loader/cache/CachedImage.cpp:716
> +        if (client->allowsAnimation(sourceURL))

I would suggest passing *this instead of the sourceURL. This will be consistent with other calls to CachedImageClient in this file.

> Source/WebCore/loader/cache/CachedImageClient.h:56
> +    virtual bool allowsAnimation(const URL&) { return false; }

Please make this function take a CachedImage& instead. So it looks similar to the other functions above.

> Source/WebCore/page/Page.cpp:4035
> +    if (settings().imageAnimationEnabled())
> +        return;
> +    
> +    m_allowsImageAnimations = shouldAnimate;

Why do we have to options to enable the animations? You even check the settings() member but you set the Page member. I think we can rely on the Settings only:

    if (settings().imageAnimationEnabled())
        return;

    settings().setImageAnimationEnabled(true);

Another approach is to add a webcoreOnChange to your setting. See CoreMathMLEnabled in WebPreferencesExperimental.yaml as an example.

> Source/WebCore/page/Page.cpp:4049
> +    forEachDocument([] (Document& document) {
> +        for (auto& element : descendantsOfType<Element>(document.rootNode())) {
> +            if (auto* renderer = element.renderer()) {
> +                if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))
> +                    renderer->repaint();
> +                if (renderer->style().hasBackgroundImage()
> +                    || renderer->style().hasBorderImage()
> +                    || renderer->style().hasMask()
> +                    || renderer->style().listStyleImage()) {
> +                    renderer->repaint();
> +                }
> +            }
> +        }
> +    });

This code is expensive. And you do not need to invalidate the renderers outside the viewport. Please consider refactoring or rewriting this code similar to Page::resumeAnimatingImages(). In other words, you need to work only within the visible rectangle.

> Source/WebCore/rendering/RenderElement.cpp:2499
> +    return settings().imageAnimationEnabled() || document().page()->allowsImageAnimation() || document().isImageURLAllowedToAnimate(sourceURL);

I think we should use document().page()->allowsImageAnimation() only to read the global setting.

> Source/WebCore/testing/Internals.cpp:1059
> +void Internals::togglePageAnimation(bool shouldAnimate)

I think this function should be removed. internals.settings.setImageAnimationEnabled() can be used instead.

-- 
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/20220901/02fce5c8/attachment-0001.htm>