Bug 248545 – Rename HAVE(ACCESSIBILITY_ANIMATED_IMAGE_CONTROL) to ENABLE(ACCESSIBILITY_ANIMATION_CONTROL) and use it in more places

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Tyler Wilcock]

We need to rename it to ENABLE(...) because certain Internals methods should be compiled conditionally based on this flag, and HAVE conditionals are not supported by our IDL codegen script. Also, in general we should be using this flag in more places.

[Tyler Wilcock]

Created attachment 463810 [details]
Patch

[Tyler Wilcock]

Created attachment 463814 [details]
Patch

[Tyler Wilcock]

Created attachment 463816 [details]
Patch

[Tyler Wilcock]

Created attachment 463817 [details]
Patch

[chris fleizach]

Comment on attachment 463817 [details]
Patch

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

> Source/WebCore/page/Page.cpp:1996
> +    if (m_imageAnimationEnabled == enabled || !settings().imageAnimationControlEnabled())

could this mean we end up in a state where user toggles off the setting, but the state is OFF, so now they can't turn animations back on?

[Tyler Wilcock]

(In reply to chris fleizach from comment #5)
> Comment on attachment 463817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463817&action=review
> 
> > Source/WebCore/page/Page.cpp:1996
> > +    if (m_imageAnimationEnabled == enabled || !settings().imageAnimationControlEnabled())
> 
> could this mean we end up in a state where user toggles off the setting, but
> the state is OFF, so now they can't turn animations back on?
Yes, but all it would take to fix this is:
  1. Closing the page and re-opening it
  2. Turning ImageAnimationControlEnabled back on

I think this is fine since this is an experimental setting, and they are explicitly turning off the ability to control animations while they're in a state of being paused. Making this change allowed for simplification of all the callsites of this function, since without it whether the caller is allowed to `setImageAnimationEnabled` is context-dependent.

[chris fleizach]

Comment on attachment 463817 [details]
Patch

Thanks, lgtm

[Tyler Wilcock]

Created attachment 463878 [details]
Patch

[EWS]

Committed 257362@main (5bfe4c6223b7): <https://commits.webkit.org/257362@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463878 [details].

[Radar WebKit Bug Importer]

<rdar://problem/102960581>