Directly-composited animated GIFs never resume once scrolled offscreen

[Chris Dumez]

Animated GIFs inside compositing layers never resume once scrolled offscreen.

Repro case:
1. Go to https://jsfiddle.net/a0xudfLw/5/
2. See how the image is animated in the lower left frame
3. Scroll that image out of the viewport
4. Scroll the image back inside the viewport
-> The image animation is not properly resumed :(

[Chris Dumez]

rdar://problem/19982020

[Chris Dumez]

Created attachment 268461 [details]
Patch

[Daniel Bates]

Comment on attachment 268461 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        RenderBoxModelObject::contentChanched(ImageChanged) in addition to

contentChanched => contentChanged

> Source/WebCore/ChangeLog:26
> +        Call RenderBoxModelObject::contentChanched(ImageChanged) in addition to

Ditto

> Source/WebCore/ChangeLog:38
> +        to cover this bug as our rendering code believed it has unpaused the

Nit: "has unpaused" => "had resumed"

> Source/WebCore/rendering/RenderElement.cpp:1519
> +    // Calling repaint() does not suffice to restart the animation in the case of
> +    // directly-composited animated GIFs so we need to do this.

Maybe a better way to write this is:

Mark the image as changed to resume directly-composited animated GIFs.

OR

For directly-composited animated GIFs it does not suffice to call repaint() to resume animation. We need to mark the image as changed.

> LayoutTests/fast/images/composited-animated-gif-outside-viewport.html:7
> +<img id="testImage" src="resources/animated.gif" style="-webkit-transform: translatez(0);" />

Please remove the trailing slash character as the img tag does not have a closing tag among other reasons.

> LayoutTests/fast/images/composited-animated-gif-outside-viewport.html:36
> +function scrollDown() {

Nit: The position of the opening curly brace for this function differs from the position of the opening curly brace used in other functions in this file (e.g. scrollUp). I suggest we place the opening brace on its own line. Regardless, we should pick one style and stick with it throughout this file.

> LayoutTests/fast/images/composited-animated-gif-outside-viewport.html:42
> +function runTest() {

Ditto.

[Daniel Bates]

Comment on attachment 268461 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Directly-composited animated GIFs would never resume once scrolled
> +        offscreen. This is because calling repaint() in this case would not

Would this also affect SVG images? If so, we should add a test for this.

[Daniel Bates]

(In reply to comment #4)
> Comment on attachment 268461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268461&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Directly-composited animated GIFs would never resume once scrolled
> > +        offscreen. This is because calling repaint() in this case would not
> 
> Would this also affect SVG images? If so, we should add a test for this.

Tim Horton mentioned on IRC today that that this does not affect SVG images as they are not directly composited.

[Chris Dumez]

Created attachment 268468 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 268468 [details]
Patch

Clearing flags on attachment: 268468

Committed r194706: <http://trac.webkit.org/changeset/194706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.