[Webkit-unassigned] [Bug 246737] AX: Make animated image experimental feature respect preferences

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from Tyler Wilcock <tyler_w at apple.com> ---
(In reply to chris fleizach from comment #5)
> Comment on attachment 463087 [details]
> patch
> >> Source/WebCore/page/SettingsBase.cpp:-491
> >> -}
> > 
> > I'm not sure if we can remove this until we remove the experimental flag entirely. This code is important because it turns animations back on if the feature flag is disabled so users aren't stuck with a page of disabled animations.
> 
> or they can just reload the page.
> 
> but I wanted opinions on this anyway...
> 
> basically the Client side is sending over the preference for whether this is
> on. that bit + feature flag determines if it's on
> 
> In this case here we only have access to the final state + the new
> experimental state... so basically you could have a different state than
> with the preference
> 
> then I wondered do we even need experimental flag if we're gaiting with a
> preference? thoughts?
So if I understand correctly, you're saying we don't have access to the preference state in this context, and by allowing this function to set image animation to true when the feature flag is changed to disabled, the "image animation enabled" pref could be disabled while WebKit is actually allowing animations?

If so, I think that's the experimental feature flag functioning as expected and therefore fine behavior. The user themselves is the one changing the feature flag state, and is knowingly overriding their preference choice. I imagine they'd be surprised if it _didn't_ cause a change in the page.

I also imagine this feature flag should be deleted entirely (with "on" being the only behavior) before this setting is even exposed to users, making this problem moot. But maybe you had other plans?

What do you think?

> >> Source/WebCore/testing/Internals.cpp:1055
> >> +        page->setImageAnimationEnabled(enabled);
> > 
> > Why remove:
> > 
> > if (page->settings().imageAnimationControlEnabled())
> > 
> > here? Is it no longer necessary?
> 
> this is internals only. It seemed unnecessary to tie Internals to the
> experimental feature flag. do you have a strong opinion?
True. No strong opinion here — keeping or removing is fine.

-- 
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/20221019/ee95060d/attachment-0001.htm>