Bug 248020

Created attachment 463574 [details] Patch

Created attachment 463576 [details] Patch

Created attachment 463577 [details] Patch

The title of this bug is not specific to animated images, but the patch is.

<rdar://problem/102638732>

Created attachment 463766 [details] Patch

(In reply to Antoine Quint from comment #4) > The title of this bug is not specific to animated images, but the patch is. We talked about this in Slack, but posting here too for posterity. The intention of these APIs is to pause and play all types of animations, not just animated images. So it will include SVG animations, Web Animations, model element animations, etc (the latter two are yet to be implemented). Here's a test exercising the core functionality for SVG and image animations: https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/images/pagewide-play-pause-offscreen-animations.html

Comment on attachment 463766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=463766&action=review > COMMIT_MESSAGE:1 > +Add private iOS WKWebView APIs: _pauseAllAnimations, _playAllAnimations, _isAnyAnimationAllowedToPlay Why private?

(In reply to Darin Adler from comment #8) > Comment on attachment 463766 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=463766&action=review > > > COMMIT_MESSAGE:1 > > +Add private iOS WKWebView APIs: _pauseAllAnimations, _playAllAnimations, _isAnyAnimationAllowedToPlay > > > Why private? As of now, there will only be one internal consumer. I think we could consider promoting it to a public API later on, but I figured making it private to start is a good first step.

(In reply to Tyler Wilcock from comment #9) > As of now, there will only be one internal consumer. I think we could > consider promoting it to a public API later on, but I figured making it > private to start is a good first step. I don’t agree with this general principle. We should discuss our approach so we don’t build up an even longer list of private API.

Comment on attachment 463766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=463766&action=review > COMMIT_MESSAGE:28 > +tests in AnimationControl.mm to pass consistently. Can you explain why this change is needed to make the tests pass consistently? > Source/WebCore/platform/graphics/BitmapImage.cpp:446 > + m_animationIsRunning = !!m_frameTimer; This is a weird place to set this flag. > Source/WebCore/platform/graphics/BitmapImage.h:241 > + bool m_animationIsRunning { false }; What is the meaning of this flag? And what is the difference between its value and the value of canAnimate()? I think you are changing its meaning. It is currently true when we are displaying a frame, waiting for its interval to elapse to draw the next frame. With your change it I think it will mean whether the image is animating or still since it is not cleared in BitmapImage::clearTimer(). Also it is weird to have two flags m_animationIsRunning and m_animationFinished although each of them means the opposite to the other. How can we guarantee they are not both set to true? Actually having both of them initialized with false is confusing. If m_animationIsRunning is false then m_animationFinished should be true.

Comment on attachment 463766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=463766&action=review >> Source/WebCore/platform/graphics/BitmapImage.h:241 >> + bool m_animationIsRunning { false }; > > What is the meaning of this flag? And what is the difference between its value and the value of canAnimate()? > > I think you are changing its meaning. It is currently true when we are displaying a frame, waiting for its interval to elapse to draw the next frame. With your change it I think it will mean whether the image is animating or still since it is not cleared in BitmapImage::clearTimer(). > > Also it is weird to have two flags m_animationIsRunning and m_animationFinished although each of them means the opposite to the other. How can we guarantee they are not both set to true? Actually having both of them initialized with false is confusing. If m_animationIsRunning is false then m_animationFinished should be true. I meant to say you are changing the meaning of BitmapImage::isAnimating() since it is now returning m_animationIsRunning.

It may be wise to keep the new API private until CSS has a resolution on Issue #7440? https://github.com/w3c/csswg-drafts/issues/7440

Created attachment 464018 [details] Patch

(In reply to Said Abou-Hallawa from comment #11) > Comment on attachment 463766 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=463766&action=review > > > COMMIT_MESSAGE:28 > > +tests in AnimationControl.mm to pass consistently. > > Can you explain why this change is needed to make the tests pass > consistently? > > > Source/WebCore/platform/graphics/BitmapImage.cpp:446 > > + m_animationIsRunning = !!m_frameTimer; > > This is a weird place to set this flag. > > > Source/WebCore/platform/graphics/BitmapImage.h:241 > > + bool m_animationIsRunning { false }; > > What is the meaning of this flag? And what is the difference between its > value and the value of canAnimate()? > > I think you are changing its meaning. It is currently true when we are > displaying a frame, waiting for its interval to elapse to draw the next > frame. With your change it I think it will mean whether the image is > animating or still since it is not cleared in BitmapImage::clearTimer(). > > Also it is weird to have two flags m_animationIsRunning and > m_animationFinished although each of them means the opposite to the other. > How can we guarantee they are not both set to true? Actually having both of > them initialized with false is confusing. If m_animationIsRunning is false > then m_animationFinished should be true. Thanks for the insight! I was able to write the test such that the changes to BitmapImage::isAnimating are unnecessary.

Created attachment 464021 [details] Patch

(In reply to Tyler Wilcock from comment #16) > Created attachment 464021 [details] > Patch Looks like feedback has been addressed and public API has been pended for good reasons. I'm inclined to approve this.

Created attachment 464393 [details] Patch

Created attachment 464409 [details] Patch

Committed 258671@main (c6458455e6da): <https://commits.webkit.org/258671@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464409 [details].