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

Fri Aug 19 14:40:48 PDT 2022

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

--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 461738
  --> https://bugs.webkit.org/attachment.cgi?id=461738
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:77
> +    m_disableImageAnimation = document.settings().animatedImagesDisabled();

Should be part of the static initializer list. Also, per coding style, since this is a boolean variable, it needs a prefix, for e.g. `m_shouldDisableImageAnimation`.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:509
> +        m_modelPlayer->setAnimationIsPlaying(false, [] (bool success) mutable {

Why is the lambda mutable?

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:535
> +    m_allowIndividualAnimation = true;

Missing prefix for boolean variables (per coding style).

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:541
> +    m_modelPlayer->setAnimationIsPlaying(false, [] (bool success) mutable {

Why mutable?

> Source/WebCore/html/HTMLImageElement.cpp:424
> +        if (auto* image = cachedImage->image()) {

if (auto* image = dynamicDowncast<BitmapImage>(cachedImage->image()))
    image->updateDocumentOnRender(document());

>> Source/WebCore/html/HTMLImageElement.cpp:787
>> +    if (auto* cachedImage = this->cachedImage()) {
> 
> looks like a lot of duplicated code here - can you condense into helper methods

I agree it would be nice to add something like a cachedBitmapImage() getter on HTMLImageElement.

> Source/WebCore/html/HTMLImageElement.cpp:789
> +            if (is<BitmapImage>(*image)) {

Should use dynamicDowncast like above.

> Source/WebCore/html/HTMLImageElement.cpp:802
> +            if (is<BitmapImage>(*image)) {

ditto.

> Source/WebCore/html/HTMLImageElement.cpp:815
> +        if (auto* image = cachedImage->image()) {

ditto.

> Source/WebCore/html/HTMLImageElement.cpp:1020
> +            if (is<BitmapImage>(*image)) {

ditto

> Source/WebCore/html/HTMLImageElement.h:160
> +    bool isAnimating();

Can this be const?

> Source/WebCore/html/HTMLImageElement.h:161
> +    bool isAnimatedImage();

ditto

> Source/WebCore/page/Page.cpp:4036
> +        for (auto& element : descendantsOfType<Element>(const_cast<ContainerNode&>(document.rootNode()))) {

I think that document.rootNode() is always the document itself.

> Source/WebCore/page/Page.cpp:4037
> +            if (is<HTMLImageElement>(element)) {

Could use dynamicDowncast.

> Source/WebCore/page/Page.cpp:4043
> +            if (is<SVGSMILElement>(element)) {

else if ?

Could use dynamicDowncast.

> Source/WebCore/page/Page.cpp:4049
> +            if (is<HTMLModelElement>(element)) {

else if ?

Could use dynamicDowncast.

> Source/WebCore/page/Page.cpp:4059
> +                            if (backgroundImage && is<BitmapImage>(backgroundImage)) {

is<> already does the null check for you. Also, can probably use dynamicDowncast()

> Source/WebCore/page/Page.cpp:4079
> +        for (auto& element : descendantsOfType<Element>(const_cast<ContainerNode&>(document.rootNode()))) {

document.rootNode() is the document AFAIK

> Source/WebCore/page/Page.cpp:4109
> +bool Page::isAllAnimationPaused()

Should probably be inlined in the header given how trivial it is.

Also isAllAnimationPaused -> areAllAnimationsPaused?

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

m_hasUnpausedAnimation? The name is a bit confusing at the moment.

> Source/WebCore/platform/graphics/BitmapImage.h:166
> +    bool isAnimationFinished() { return m_animationFinished; };

Should be const:
`bool isAnimationFinished() const { return m_animationFinished; };`

> Source/WebCore/platform/graphics/BitmapImage.h:256
> +    bool m_disableImageAnimation { false };

Missing prefix.

> Source/WebCore/platform/graphics/BitmapImage.h:257
> +    bool m_allowIndividualAnimation { false };

Missing prefix.

> Source/WebCore/svg/SVGElement.cpp:163
> +    , m_allowIndividualAnimation(false)

Please use inline initialization in the header.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:1245
> +    m_allowIndividualAnimation = !m_allowIndividualAnimation;

= true; would be clearer I think.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:1253
> +    m_allowIndividualAnimation = !m_allowIndividualAnimation;

= false; would be clearer I think.

> LayoutTests/accessibility/mac/disable-animations-enabled.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/accessibility/mac/disable-animations-enabled.html:7
> +<body id="body">

id seems unnecessary.

> LayoutTests/accessibility/mac/disable-animations-enabled.html:9
> +    jsTestIsAsync = true;

Please add a `description("This tests that animations are disabled when the disable animated images setting is turned on.");` call before this.

> LayoutTests/accessibility/mac/disable-animations-enabled.html:13
> +        debug("This tests that animations are disabled when the disable animated images setting is turned on.");		

And drop this.

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:7
> +<body id="body">

id seems unnecessary?

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:14
> +        debug("This tests that when image animation is disabled, they can be toggled on and off individually.");		

Same comment about using description() instead of debug for this.

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:27
> +<script src="../../resources/js-test-post.js"></script>

Not needed.

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:7
> +<body id="body">

unnecessary id?

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:15
> +        debug("This tests that when image animation is disabled, all animations on a page can be toggled on and off.");		

Please use description()

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:29
> +<script src="../../resources/js-test-post.js"></script>

Not needed

-- 
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/20220819/e8377e48/attachment-0001.htm>