[Webkit-unassigned] [Bug 244128] Add experimental feature to disable Bitmap image animations

[bugzilla-daemon at webkit.org]

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

--- Comment #39 from Joshua Hoffman <jhoffman23 at apple.com> ---
(In reply to Said Abou-Hallawa from comment #37)

Thanks for all of the comments! I addressed the feedback in most-recently uploaded patch.

> > Source/WebCore/page/Page.cpp:4048
> > +void Page::toggleAllAnimationsWhenAnimationSettingEnabled(bool shouldAnimate)
> > +{
> > +    if (!settings().animatedImagesDisabled())
> > +        return;
> > +    
> > +    m_shouldPageAnimationPlayIfSettingEnabled = shouldAnimate;
> > +    forEachDocument([] (Document& document) {
> > +        for (auto& element : descendantsOfType<Element>(document.rootNode())) {
> > +            if (auto* renderer = element.renderer()) {
> > +                if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))
> > +                    renderer->repaint();
> > +                if (renderer->style().hasBackgroundImage()) {
> > +                    if (renderer->style().backgroundImagesCanAnimate())
> > +                        renderer->repaint();
> > +                }
> > +            }
> > +        }
> > +    });
> > +}
> 
> Why do not we use Page::resumeAnimatingImages() instead of adding a new
> function? I think Page::resumeAnimatingImages() is called when animated
> images brought back in the current viewport like you scroll the window for
> example.
>
> ... 
>
> Also this function and toggleAllAnimationsWhenAnimationSettingEnabled() are
> expensive since they enumerate all sub-documents in the page and in each
> document they enumerate all the descendant elements. We usually care about
> the current view port when invalidating renderers.

In a subsequent patch, this method allows us to expand toggling all animations to <svg> & <model> elements as we walk the tree and resume/pause. `resumeAnimatingImages` was not repainting my bitmap images as expected. 

> 
> >> Source/WebCore/platform/graphics/BitmapImage.cpp:378
> >> +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );
> > 
> > Why doesn't the call out to imageObserver do the job of consulting the setting and whether animations have been re-enabled? It would be better if BitmapImage didn't have to store m_disableAnimation and m_allowAnimationWhenSettingEnabled
> 
> I agree. See my comment above. This new code can be replaced by "&&
> imageObserver()->isImageAnimationEnabled()"

I've addressed this and have moved all of the image's animation state to the CachedImageClient.

> 
> >> Source/WebCore/platform/graphics/BitmapImage.cpp:680
> >> +void BitmapImage::resumeAnimation()
> > 
> > It's not good for a generically named function like "resumeAnimation" to store state which mirrors some settings-related state elsewhere (i.e. don't store m_allowAnimationWhenSettingEnabled here).
> 
> I do not think you need this function. If the Settings changed and you
> invalidate the renderers of this BitmapImage, the BitmapImage will be
> redrawn. And in this time shouldAnimate() will be true because
> imageObserver()->isImageAnimationEnabled() will be true.

This patch allows image animation (when ImageAnimationEnabled is not turned on) to be resumed/paused at the page and at the individual image level. The resumeAnimation and pauseAnimation methods allow a single image to be controlled, while the setting is there to simply stop animations by default. But, in my updated patch, I've limited these methods to just HTMLImageElements, and removed these methods/state from BitmapImage. 

> 
> > LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:13
> > +    if (window.accessibilityController) {
> 
> Why do we test this code only through window.accessibilityController? I
> think this feature should be tested for general use.
> 
> Also we need to test that the timing of toggling the flag will work as
> expected. For example if we disable the animation while painting the frame 5
> of the image, and then resume the animation it should be resumed at frame 6.
> 
> Look at LayoutTests/fast/images/animated-heics-draw.html to see how we can
> control the timing of the animated image.
> 

I included a test for this in the new patch, which tests the frames are in order even if paused/resumed.

> > LayoutTests/platform/mac-wk1/TestExpectations:436
> > +# Image animation disabled (experimental feature)
> > +accessibility/mac/disable-animations-toggle-page-animations.html [ Skip ]
> > +accessibility/mac/disable-animations-enabled.html [ Skip ]
> > +accessibility/mac/disable-animations-play-individual-animation.html [ Skip ]
> 
> Are these tests skipped for WK1 because window.accessibilityController is
> available for WK2?

There is not a plan to support this feature on WK1 at the moment, so the test scope is limited.

-- 
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/20220831/bb31ed04/attachment-0001.htm>