[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 #31 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 462012
  --> https://bugs.webkit.org/attachment.cgi?id=462012
Patch

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

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

This is a bit of a brain teaser. Why isn't it just called allowsAnimation() ?

> Source/WebCore/loader/cache/CachedImage.cpp:714
> +    return m_skippingRevalidationDocument && m_skippingRevalidationDocument->page() && m_skippingRevalidationDocument->page()->shouldPageAnimationPlayIfSettingEnabled();

m_skippingRevalidationDocument sounds like something special and I'm not sure it's OK to use to get to Page. I think you need to hop out again through CachedImageClient.

> Source/WebCore/loader/cache/CachedImage.h:161
> +        bool hasPageResumedAnimationWhenSettingEnabled() final;

allowsAnimation() ?

> Source/WebCore/page/Page.h:1349
> +    bool m_shouldPageAnimationPlayIfSettingEnabled { false };

This "if setting enabled" is cumbersome (partly because you can't tell which setting it's referring to). An enum would be better:

enum class ImageAnimationState : uint8_t {
  Allowed,
  Disabled,
  Reenabled
};

but I'm not sure why you need the "animations disabled by re-enabled by the user" state. Why not just have a `bool m_allowsImagesAnimations` that defaults to true, is set to false if the Setting is set, then goes back to true if the user re-enables animations?

BTW, what's the lifetime of the "re-enabled by user" state? Does it live across page loads?

> Source/WebCore/platform/graphics/BitmapImage.cpp:378
> +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );

Why doesn't the call out to imageObserver do the job of consulting the setting and whether animations have been re-enabled? It would be better if BitmapImage didn't have to store m_disableAnimation and m_allowAnimationWhenSettingEnabled

> Source/WebCore/platform/graphics/BitmapImage.cpp:680
> +void BitmapImage::resumeAnimation()

It's not good for a generically named function like "resumeAnimation" to store state which mirrors some settings-related state elsewhere (i.e. don't store m_allowAnimationWhenSettingEnabled here).

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

allowsAnimation() ?

> Source/WebCore/rendering/style/RenderStyle.cpp:471
> +bool RenderStyle::backgroundImagesCanAnimate() const

You could have animated images in masks, in border-image, in list-style-image and probably other places too.

-- 
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/20220830/3c009d36/attachment.htm>