Bug 270136 – AX: Avoid work in BitmapImage::canAnimate if animation controls are disabled

• Component: Accessibility
• Status: RESOLVED FIXED

[Joshua Hoffman]

If a user does not have animation controls enabled, there is no need to do this excess work.

<rdar://109646792>

[Radar WebKit Bug Importer]

<rdar://problem/123660105>

[Joshua Hoffman]

rdar://109646792

[Joshua Hoffman]

Created attachment 470059 [details]
Patch

[Simon Fraser (smfr)]

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.

[Tyler Wilcock]

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()?

[Tyler Wilcock]

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?

[Joshua Hoffman]

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.

[Joshua Hoffman]

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.

[Joshua Hoffman]

Created attachment 470083 [details]
Patch

[Joshua Hoffman]

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.

[Tyler Wilcock]

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.

[Joshua Hoffman]

Created attachment 470098 [details]
Patch

[Joshua Hoffman]

(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!

[Tyler Wilcock]

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"

?

[Joshua Hoffman]

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!

[Joshua Hoffman]

Created attachment 470099 [details]
Patch

[Joshua Hoffman]

Created attachment 470100 [details]
Patch

[Joshua Hoffman]

Created attachment 470106 [details]
Patch

[EWS]

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].