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

[bugzilla-daemon at webkit.org]

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

--- Comment #11 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 461746
  --> https://bugs.webkit.org/attachment.cgi?id=461746
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:781
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (auto* image = cachedImage->image()) {
> +            if (auto* bitmapImage = dynamicDowncast<BitmapImage>(*image))
> +                return bitmapImage;
> +        }
> +    }
> +    return nullptr;

Can this be:

if (auto* cachedImage = this->cachedImage())
    return dynamicDowncast<BitmapImage>(cachedImage->image());
return nullptr;

> Source/WebCore/html/HTMLImageElement.cpp:1004
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (auto* bitmapImage = cachedBitmapImage())
> +            return cachedImage->image()->isAnimated() || !bitmapImage->isAnimationFinished();
> +    }
> +    return false;

Can cachedImage->image()->isAnimated() return true without a non-null bitmap image? If so, maybe that shouldn't be gated by the presence of a cachedBitmapImage(). Maybe something like this:

bool HTMLImageElement::isAnimatedImage() const
{
    if (auto* cachedImage = this->cachedImage()) {
        if (cachedImage->image()->isAnimated())
            return true;
    }

    if (auto* bitmapImage = cachedBitmapImage())
        return !bitmapImage->isAnimationFinished();

    return false;
}

> Source/WebCore/page/Page.cpp:4056
> +                        if (auto* backgroundImage = cachedBackgroundImage->image()) {
> +                            if (backgroundImage) {

Is this `if (backgroundImage)` unnecessary when you have `if (auto* backgroundImage = cachedBackgroundImage->image())` directly above?

> Source/WebCore/page/Page.cpp:4057
> +                                BitmapImage& bitmapImage = downcast<BitmapImage>(*backgroundImage);

Does auto& rather than BitmapImage& work here?

> Source/WebCore/page/Page.cpp:4078
> +                foundAnimatedImage = true;

Can we break out of this loop when we set `foundAnimatedImage = true;`?

> Source/WebCore/page/Page.cpp:4087
> +                        if (auto* backgroundImage = cachedBackgroundImage->image()) {
> +                            if (backgroundImage && backgroundImage->isAnimated())

Is the `backgroundImage` null check necessary here when we have` if (auto* backgroundImage = cachedBackgroundImage->image())` directly above?

> Source/WebCore/svg/graphics/SVGImage.cpp:382
> +    if (!rootElement || !rootElement->animationsPaused() || m_page->settings().animatedImagesDisabled() || !m_page)

Here we added:

|| m_page->settings().animatedImagesDisabled() || !m_page

Seems like we null check after dereferencing m_page via -> — should these be flipped?

> Source/WebCore/testing/Internals.cpp:1069
> +    auto* page = document->page();
> +    if (!page)
> +        return;
> +
> +    return page->toggleAllAnimations();

This is a void method, so we probably only need `page->toggleAllAnimations();` rather than `return page->toggleAllAnimations();`, right?

Also, I wonder if this could be:

if (auto* page = document->page())
   page->toggleAllAnimations();

-- 
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/20220822/2b3a23b3/attachment-0001.htm>