If a user does not have animation controls enabled, there is no need to do this excess work. <rdar://109646792>
<rdar://problem/123660105>
rdar://109646792
Created attachment 470059 [details] Patch
Comment on attachment 470059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470059&action=review > Source/WebCore/page/Page.cpp:2254 > + Image::setSystemAllowsAnimationControls(isAllowed); This is setting process-wide state, but you can have more than one Page in a WebProcess.
Comment on attachment 470059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470059&action=review > Source/WebCore/platform/graphics/BitmapImage.cpp:343 > + return repetitionCount() && !m_animationFinished && (!Image::systemAllowsAnimationControls() || imageObserver()->allowsAnimation(*this)); Do we still want to null-check imageObserver()?
Comment on attachment 470059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470059&action=review > Source/WebCore/loader/cache/CachedImage.cpp:744 > + if (!Image::systemAllowsAnimationControls()) Might be nice to have a comment explaining that CachedImageClient::allowsAnimation() will only ever return false when `systemAllowsAnimationControls`, so this is a fast-path exit to avoid an unnecessary CachedResourceClientWalker walk?
Comment on attachment 470059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470059&action=review >> Source/WebCore/page/Page.cpp:2254 >> + Image::setSystemAllowsAnimationControls(isAllowed); > > This is setting process-wide state, but you can have more than one Page in a WebProcess. That should be fine since we don't enable animation controls per-page. But, I can move this call to the process-level to make that clear.
Comment on attachment 470059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470059&action=review >> Source/WebCore/loader/cache/CachedImage.cpp:744 >> + if (!Image::systemAllowsAnimationControls()) > > Might be nice to have a comment explaining that CachedImageClient::allowsAnimation() will only ever return false when `systemAllowsAnimationControls`, so this is a fast-path exit to avoid an unnecessary CachedResourceClientWalker walk? Good idea, will do! >> Source/WebCore/platform/graphics/BitmapImage.cpp:343 >> + return repetitionCount() && !m_animationFinished && (!Image::systemAllowsAnimationControls() || imageObserver()->allowsAnimation(*this)); > > Do we still want to null-check imageObserver()? Yeah, I inadvertently removed that when refactoring. Will add it back in.
Created attachment 470083 [details] Patch
Updated the patch to set the Image::systemAllowsAnimationControls global from WebProcess, rather than the page, because this setting is per-process. This allows us to remove what we were doing prior with Page::systemAllowsAnimationControl, and have callers to that just call the static method on Image instead.
Comment on attachment 470083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470083&action=review Those api-ios test failures look relevant to this patch > COMMIT_MESSAGE:8 > +Before this patch, the expensive work that happens in BitmapImage::canAnimate would still occur > +in two unnecessary cases: (1) if the image has a single-frame and (2) if image animation controls "Before this patch, the expensive work that happens in BitmapImage::canAnimate would unnecessarily occur in two cases: ..." ? Might be a bit more succinct. > Source/WebCore/loader/cache/CachedImage.cpp:481 > + // *::allowsAnimation can only return false when > + // systemAllowsAnimationControls == true, so this prevents unnecessary work > + // by exiting early. Not a huge deal, but IMO this comment should take up 2 lines, with the first line being longer or equal length to the subsequent one.
Created attachment 470098 [details] Patch
(In reply to Tyler Wilcock from comment #11) > Comment on attachment 470083 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=470083&action=review > > Those api-ios test failures look relevant to this patch Ah yep—since we don't actually perform AX preference changes in layout tests, we were never setting Image::systemAllowsAnimationControls. Fixed this so that we also set it in the Internals method we invoke in the tests. > > COMMIT_MESSAGE:8 > > +Before this patch, the expensive work that happens in BitmapImage::canAnimate would still occur > > +in two unnecessary cases: (1) if the image has a single-frame and (2) if image animation controls > > "Before this patch, the expensive work that happens in > BitmapImage::canAnimate would unnecessarily occur in two cases: ..." ? Might > be a bit more succinct. > > > Source/WebCore/loader/cache/CachedImage.cpp:481 > > + // *::allowsAnimation can only return false when > > + // systemAllowsAnimationControls == true, so this prevents unnecessary work > > + // by exiting early. > > Not a huge deal, but IMO this comment should take up 2 lines, with the first > line being longer or equal length to the subsequent one. Addressed both of these, thank you!
Comment on attachment 470098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470098&action=review > Source/WebCore/platform/graphics/Image.h:190 > + WEBCORE_EXPORT static bool gSystemAllowsAnimationControls; Does this need to be WEBCORE_EXPORT? > Source/WebCore/testing/Internals.cpp:1127 > + // We need to set this here to mimic the behavior for the AX preference changing "the behavior of the AX preference changing" rather than "the behavior for the AX preference changing" ?
Comment on attachment 470098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470098&action=review >> Source/WebCore/platform/graphics/Image.h:190 >> + WEBCORE_EXPORT static bool gSystemAllowsAnimationControls; > > Does this need to be WEBCORE_EXPORT? Nope, apologies! will remove.... >> Source/WebCore/testing/Internals.cpp:1127 >> + // We need to set this here to mimic the behavior for the AX preference changing > > "the behavior of the AX preference changing" rather than "the behavior for the AX preference changing" > > ? Changed!
Created attachment 470099 [details] Patch
Created attachment 470100 [details] Patch
Created attachment 470106 [details] Patch
Committed 275563@main (1d7be2a5f983): <https://commits.webkit.org/275563@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 470106 [details].