Bug 248545 - Rename HAVE(ACCESSIBILITY_ANIMATED_IMAGE_CONTROL) to ENABLE(ACCESSIBILITY_ANIMATION_CONTROL) and use it in more places
Summary: Rename HAVE(ACCESSIBILITY_ANIMATED_IMAGE_CONTROL) to ENABLE(ACCESSIBILITY_ANI...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-11-30 11:54 PST by Tyler Wilcock
Modified: 2022-12-04 13:34 PST (History)
11 users (show)

See Also:

Attachments
Patch (15.89 KB, patch)
2022-11-30 12:01 PST, Tyler Wilcock 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.60 KB, patch)
2022-11-30 13:04 PST, Tyler Wilcock 		no flags Details | Formatted Diff | Diff
Patch (18.11 KB, patch)
2022-11-30 15:55 PST, Tyler Wilcock 		no flags Details | Formatted Diff | Diff
Patch (18.80 KB, patch)
2022-11-30 16:11 PST, Tyler Wilcock 		no flags Details | Formatted Diff | Diff
Patch (18.75 KB, patch)
2022-12-03 14:24 PST, Tyler Wilcock 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2022-11-30 11:54:24 PST 
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.
Comment 1 Tyler Wilcock 2022-11-30 12:01:57 PST 
Created attachment 463810 [details]
Patch
Comment 2 Tyler Wilcock 2022-11-30 13:04:46 PST 
Created attachment 463814 [details]
Patch
Comment 3 Tyler Wilcock 2022-11-30 15:55:01 PST 
Created attachment 463816 [details]
Patch
Comment 4 Tyler Wilcock 2022-11-30 16:11:37 PST 
Created attachment 463817 [details]
Patch
Comment 5 chris fleizach 2022-12-01 10:30:44 PST 
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?
Comment 6 Tyler Wilcock 2022-12-01 10:55:57 PST 
(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.
Comment 7 chris fleizach 2022-12-03 12:06:19 PST 
Comment on attachment 463817 [details]
Patch

Thanks, lgtm
Comment 8 Tyler Wilcock 2022-12-03 14:24:54 PST 
Created attachment 463878 [details]
Patch
Comment 9 EWS 2022-12-04 13:33:52 PST 
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].
Comment 10 Radar WebKit Bug Importer 2022-12-04 13:34:18 PST 
<rdar://problem/102960581>