[webkit-reviews] review granted: [Bug 170155] Animated SVG images are not paused when outside viewport : [Attachment 305620] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 170155: Animated SVG images are not paused when outside viewport
https://bugs.webkit.org/show_bug.cgi?id=170155

Attachment 305620: Patch

https://bugs.webkit.org/attachment.cgi?id=305620&action=review




--- Comment #7 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 305620
  --> https://bugs.webkit.org/attachment.cgi?id=305620
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305620&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:121
> +    if (m_image)
> +	   m_image->startAnimation();

It might be clearer if this was renamed to something like
startAnimationIfNeeded() as it doesn't actually always start it.

> Source/WebCore/loader/cache/CachedImage.cpp:526
> +    bool shouldPauseAnimation = true;
> +
>      CachedResourceClientWalker<CachedImageClient> clientWalker(m_clients);
> -    while (CachedImageClient* client = clientWalker.next())
> -	   client->newImageAnimationFrameAvailable(*this);
> +    while (CachedImageClient* client = clientWalker.next()) {
> +	   bool canPause = false;
> +	   client->newImageAnimationFrameAvailable(*this, canPause);

One bool would be enough (canPause could be moved outside the loop).

> Source/WebCore/rendering/RenderElement.cpp:1496
> +void RenderElement::newImageAnimationFrameAvailable(CachedImage& image,
bool& canPause)

Another option would be to use an enum return value.

> Source/WebCore/rendering/RenderView.cpp:1402
> +	   return Vector<CachedImage*>();

Are these CachedImages somehow guaranteed to stay alive?