Make animated images experimental feature respect preferences
<rdar://problem/101326045>
Created attachment 463087 [details] patch
Comment on attachment 463087 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=463087&action=review > Source/WebCore/PAL/pal/spi/cocoa/AccessibilitySupportSoftLink.cpp:32 > SOFT_LINK_LIBRARY_FOR_SOURCE(PAL, libAccessibility) Wouldn’t we want this in an #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) || HAVE(ACCESSIBILITY_ANIMATED_IMAGES) block? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:785 > - > + Let’s not add leading whitespace like this.
Comment on attachment 463087 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=463087&action=review > Source/WebCore/page/SettingsBase.cpp:-491 > -void SettingsBase::setImageAnimationControlEnabledChanged() > -{ > - if (m_page && !m_page->settings().imageAnimationControlEnabled()) > - m_page->setImageAnimationEnabled(true); > -} 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. > Source/WebCore/testing/Internals.cpp:1055 > - if (auto* page = contextDocument() ? contextDocument()->page() : nullptr) { > - if (page->settings().imageAnimationControlEnabled()) > - page->setImageAnimationEnabled(enabled); > - } > + if (auto* page = contextDocument() ? contextDocument()->page() : nullptr) > + page->setImageAnimationEnabled(enabled); Why remove: if (page->settings().imageAnimationControlEnabled()) here? Is it no longer necessary? > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1129 > + _page->process().updateAnimatedImagesState(); Where is updateAnimatedImagesState defined? I couldn't find it in this patch or in WebKit `main`.
Comment on attachment 463087 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=463087&action=review Apologies - did not mean to get official review on this yet, so I hadn't cleaned it up to that quality yet >> Source/WebCore/PAL/pal/spi/cocoa/AccessibilitySupportSoftLink.cpp:32 >> SOFT_LINK_LIBRARY_FOR_SOURCE(PAL, libAccessibility) > > Wouldn’t we want this in an #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) || HAVE(ACCESSIBILITY_ANIMATED_IMAGES) block? 👍 >> 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? >> 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? >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1129 >> + _page->process().updateAnimatedImagesState(); > > Where is updateAnimatedImagesState defined? I couldn't find it in this patch or in WebKit `main`. good question... >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:785 >> + > > Let’s not add leading whitespace like this. 👍
(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.
Comment on attachment 463087 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=463087&action=review >>>> 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? yea that was what I was thinking. this will go away anyway and wondering whether I should remove it as experimental with this PR
(In reply to chris fleizach from comment #7) > yea that was what I was thinking. this will go away anyway and wondering > whether I should remove it as experimental with this PR Actually yeah, that makes sense. I'm good with deleting it.
Created attachment 463116 [details] Patch
Created attachment 463118 [details] Patch
Created attachment 463136 [details] Patch
Created attachment 463151 [details] Patch
Created attachment 463172 [details] Patch
Created attachment 463186 [details] Patch
Committed 255916@main (bc9fcf475055): <https://commits.webkit.org/255916@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 463186 [details].