Bug 9767 – WebPreference methods setAllowsAnimatedImages: and setAllowsAnimatedImageLooping: don't actually affect WebView behaviors

• Component: WebKit API
• Status: NEW

[Rachael Worthington (cheers)]

calling setAllowsAnimatedImages: and setAllowsAnimatedImageLooping: on a WebPreferences object has absolutely no effect on animated images. a quick search reveals that WebKit never checks the WebPreferences object for these prefs.

[david rosnow]

Is there any way this bug can be addressed?

It's is apparently a problem for Skype.  Skype uses webkit for its chat interface and according to the developer, animated emoticons are always animated using up CPU even if skype is in the background because of this bug.  Perhaps there is another way to handle that issue but is there any reason why this is not  

Also, in terms of Safari, I find that even if a window has popped behind the main window and is not visible to the user it still seems to be animated and using cpu. Might fixing this allow you to stop animations in windows that are not visible to the user. I suspect that would greatly reduce the cpu burden on users on older machines from these animated ads behind the main window.

[Simon Fraser (smfr)]

<rdar://problem/4679993>

[Simon Fraser (smfr)]

This just needs to be plumbed through from WebPreferences to Settings, and thence to the image code.

[Pablo Flouret]

Created attachment 126875 [details]
Patch for AllowsAnimatedImages pref

This bug should probably be split in two...

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Pablo Flouret]

Comment on attachment 126875 [details]
Patch for AllowsAnimatedImages pref

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:454
> -    if (!skippingFrames && imageObserver()->shouldPauseAnimation(this))
> +    if (imageObserver()->shouldPauseAnimation(this))

I'm not totally sure if this is correct, but if shouldPauseAnimation() is not checked when skipping frames, the effect is advancing one frame at a time in some common situations (like switching tabs and back). If there's a better place to make the check let me know.

[WebKit Review Bot]

Comment on attachment 126875 [details]
Patch for AllowsAnimatedImages pref

Attachment 126875 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514100

[Early Warning System Bot]

Comment on attachment 126875 [details]
Patch for AllowsAnimatedImages pref

Attachment 126875 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11522023

[Gyuyoung Kim]

Comment on attachment 126875 [details]
Patch for AllowsAnimatedImages pref

Attachment 126875 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11518128

[Pablo Flouret]

Created attachment 126910 [details]
Fixed build errors

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Carlos Garcia Campos]

Comment on attachment 126910 [details]
Fixed build errors

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

I appreciate you added the new preference to all ports, there are some issues with WebKit2 GTK+, though.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:975
> +gboolean webkit_settings_get_allows_animated_images(WebKitSettings* settings)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_SETTINGS(settings), FALSE);
> +
> +    return WKPreferencesGetAllowsAnimatedImages(settings->priv->preferences.get());
> +}
> +
> +/**
> + * webkit_settings_set_allows_animated_images:
> + * @settings: a #WebKitSettings
> + * @enabled: Value to be set
> + *
> + * Set the #WebKitSettings:allows-animated-images property.
> + */
> +void webkit_settings_set_allows_animated_images(WebKitSettings* settings, gboolean enabled)
> +{
> +    g_return_if_fail(WEBKIT_IS_SETTINGS(settings));
> +
> +    WebKitSettingsPrivate* priv = settings->priv;
> +    bool currentValue = WKPreferencesGetAllowsAnimatedImages(priv->preferences.get());
> +    if (currentValue == enabled)
> +        return;
> +
> +    WKPreferencesSetAllowsAnimatedImages(priv->preferences.get(), enabled);
> +    g_object_notify(G_OBJECT(settings), "allow-animated-images");
> +}

This is not enough, you should install the property in webkit_settings_class_init() and implement get/set for the property in webKitSettingsGetProperty() and webKitSettingsSetProperty(). Prototypes should be added to the header too (Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h) and new symbols should be added to the documentation (Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt, in the section WebKitSettings add webkit_settings_get_allows_animated_images and webkit_settings_set_allows_animated_images at the end of the section).

[Darin Fisher (:fishd, Google)]

Comment on attachment 126910 [details]
Fixed build errors

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

> Source/WebKit/chromium/public/WebSettings.h:70
> +    virtual void setAllowsAnimatedImages(bool) = 0;

nit: this should probably be "setAllowAnimatedImages" instead.  this matches
other similarly named functions.  i think you should make this change to
WebCore::Settings too.

[Pablo Flouret]

Created attachment 127013 [details]
Fixed chromium and gtk issues

[Pablo Flouret]

(In reply to comment #13)
> (From update of attachment 126910 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126910&action=review
> 
> > Source/WebKit/chromium/public/WebSettings.h:70
> > +    virtual void setAllowsAnimatedImages(bool) = 0;
> 
> nit: this should probably be "setAllowAnimatedImages" instead.  this matches
> other similarly named functions.  i think you should make this change to
> WebCore::Settings too.

Sorry, i just realized you said i should change it in WebCore::Settings as well, but there are other prefs that sort of use this style as well (e.g. loadsImagesAutomatically), and there was some existing boilerplate code in some places using allowsAnimatedImages, so that's why i went with it.

I can change the name everywhere anyway if you prefer (and perhaps split this patch into a webcore patch and a bunch of platform ones), let me know.

[WebKit Review Bot]

Comment on attachment 127013 [details]
Fixed chromium and gtk issues

Attachment 127013 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11515363

[Pablo Flouret]

Created attachment 127026 [details]
Missing fixes from last patch

[WebKit Review Bot]

Comment on attachment 127026 [details]
Missing fixes from last patch

Attachment 127026 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11515392

[Pablo Flouret]

Created attachment 127042 [details]
Build fix

[WebKit Review Bot]

Comment on attachment 127042 [details]
Build fix

Attachment 127042 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514452

[Pablo Flouret]

Created attachment 127068 [details]
Yet another build fix

[Carlos Garcia Campos]

Comment on attachment 127068 [details]
Yet another build fix

GTK changes look good to me, thanks!

[Darin Fisher (:fishd, Google)]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 126910 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126910&action=review
> > 
> > > Source/WebKit/chromium/public/WebSettings.h:70
> > > +    virtual void setAllowsAnimatedImages(bool) = 0;
> > 
> > nit: this should probably be "setAllowAnimatedImages" instead.  this matches
> > other similarly named functions.  i think you should make this change to
> > WebCore::Settings too.
> 
> Sorry, i just realized you said i should change it in WebCore::Settings as well, but there are other prefs that sort of use this style as well (e.g. loadsImagesAutomatically), and there was some existing boilerplate code in some places using allowsAnimatedImages, so that's why i went with it.
> 
> I can change the name everywhere anyway if you prefer (and perhaps split this patch into a webcore patch and a bunch of platform ones), let me know.

On WebCore::Settings, I see 3 functions that look like "setAllowFoo" and 0 functions that look like "setAllowsFoo".  I think you should go with "setAllowFoo" for that reason.  Keep the code consistent :-)

[Pablo Flouret]

Created attachment 127468 [details]
Rename to AllowAnimatedImages

[Darin Fisher (:fishd, Google)]

Comment on attachment 127468 [details]
Rename to AllowAnimatedImages

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

I looked over the bulk of this patch, and it all looks pretty good to me.  I'm not an expert on all of the ports, but the code for them looks reasonable.

> LayoutTests/fast/images/disallow-animated-images.html:34
> +        }, 150); // Green frame of GIF has 100ms duration

what if this timer happens to be delayed due to some slowness on the bot?  if it ends up running after 200 msec, then won't this test potentially fail?

when you rely on timers like this in tests, it is very easy to introduce a flaky test.  is there any other way to approach this that won't be so sensitive to such issues?

[Pablo Flouret]

(In reply to comment #25)
> (From update of attachment 127468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127468&action=review
> 
> I looked over the bulk of this patch, and it all looks pretty good to me.  I'm not an expert on all of the ports, but the code for them looks reasonable.
> 
> > LayoutTests/fast/images/disallow-animated-images.html:34
> > +        }, 150); // Green frame of GIF has 100ms duration
> 
> what if this timer happens to be delayed due to some slowness on the bot?  if it ends up running after 200 msec, then won't this test potentially fail?
> 
> when you rely on timers like this in tests, it is very easy to introduce a flaky test.  is there any other way to approach this that won't be so sensitive to such issues?

There's one frame of green and five frames of red, 100ms each. Also, the idea is that the animation does not run, so if the code that disables animations works correctly, (in theory) it doesn't really matter how long you wait until you look for the result, it will always be green.
If the code doesn't work then it would have to be delayed the exact time to fall back into the green zone, so it would be potentially flaky in that case, but then again, it would also be failing.

I ran it with --repeat-each=1000 a bunch of times and didn't find any flakiness, but if you can think of a better way to test this i'm open to suggestions.

[Darin Fisher (:fishd, Google)]

(In reply to comment #26)
> There's one frame of green and five frames of red, 100ms each. Also, the idea is that the animation does not run, so if the code that disables animations works correctly, (in theory) it doesn't really matter how long you wait until you look for the result, it will always be green.

Good point!  Pretty obvious in hindsight.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> (From update of attachment 126875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126875&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:454
> > -    if (!skippingFrames && imageObserver()->shouldPauseAnimation(this))
> > +    if (imageObserver()->shouldPauseAnimation(this))
> 
> I'm not totally sure if this is correct, but if shouldPauseAnimation() is not checked when skipping frames, the effect is advancing one frame at a time in some common situations (like switching tabs and back). If there's a better place to make the check let me know.

I'm not an expert on this either.  I am similarly curious if this is the correct code change.

[Pablo Flouret]

Created attachment 128248 [details]
Win build fix

[Pablo Flouret]

Any idea of who i can poke to get the interesting parts of this patch reviewed?

[Tim Horton]

Comment on attachment 128248 [details]
Win build fix

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

> Source/WebCore/rendering/RenderObject.cpp:2593
> +    Settings* settings = document()->settings();
> +
>      // If we're not in a window (i.e., we're dormant from being put in the b/f cache or in a background tab)
>      // then we don't want to render either.
> -    return !document()->inPageCache() && !document()->view()->isOffscreen();
> +    return !document()->inPageCache() && !document()->view()->isOffscreen() && settings && settings->allowAnimatedImages();

I haven't looked at the rest of the patch, but this caught my eye. Presumably you're expecting there's some potential situation where settings could be null, since you added a null check.

Does it make sense, though, to change the return value of willRenderImage() in that case? (i.e. if (!document()->inPageCache() && !document()->view()->isOffscreen()) is true, but settings is null, you will now return false instead of true.

[Pablo Flouret]

(In reply to comment #31)
> (From update of attachment 128248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128248&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:2593
> > +    Settings* settings = document()->settings();
> > +
> >      // If we're not in a window (i.e., we're dormant from being put in the b/f cache or in a background tab)
> >      // then we don't want to render either.
> > -    return !document()->inPageCache() && !document()->view()->isOffscreen();
> > +    return !document()->inPageCache() && !document()->view()->isOffscreen() && settings && settings->allowAnimatedImages();
> 
> I haven't looked at the rest of the patch, but this caught my eye. Presumably you're expecting there's some potential situation where settings could be null, since you added a null check.
> 
> Does it make sense, though, to change the return value of willRenderImage() in that case? (i.e. if (!document()->inPageCache() && !document()->view()->isOffscreen()) is true, but settings is null, you will now return false instead of true.

Yeah, you're right, since allowAnimatedImages defaults to true it should be checking (!settings || settings->allowAnimatedImages()) instead. Good catch :-)

[Early Warning System Bot]

Comment on attachment 128248 [details]
Win build fix

Attachment 128248 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11856194

[Pablo Flouret]

Created attachment 130705 [details]
Patch

Changed willRenderImage() return clause per Tim's comment, and hopefully fixed qt-wk2 and win issues.

[Simon Fraser (smfr)]

Comment on attachment 130705 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/images/disallow-animated-images.html

This needs to have some explanation of what the change is doing.

> Source/WebCore/platform/graphics/BitmapImage.cpp:454
> -    if (!skippingFrames && imageObserver()->shouldPauseAnimation(this))
> +    if (imageObserver()->shouldPauseAnimation(this))

Why was !skippingFrames removed?

> Source/WebCore/rendering/RenderObject.cpp:2593
> +    return !document()->inPageCache() && !document()->view()->isOffscreen() && (!settings || settings->allowAnimatedImages());

Does this also affect static images, not just animating ones?

[Pablo Flouret]

(In reply to comment #35)
> (From update of attachment 130705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130705&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:454
> > -    if (!skippingFrames && imageObserver()->shouldPauseAnimation(this))
> > +    if (imageObserver()->shouldPauseAnimation(this))
> 
> Why was !skippingFrames removed?

If shouldPauseAnimation() is not checked when skipping frames, the effect is that in some situations (like changing tabs and back) the animation is advanced to catch up.
I suppose the question here is whether the animation should catch up to the frame that corresponds to the moment in time when image-animating was actually paused (even if the tab was hidden at the time, for instance), or if it doesn't really matter.
Personally, i don't think it matters too much, this sounds like something to save cpu cycles or to prevent annoyance.

> > Source/WebCore/rendering/RenderObject.cpp:2593
> > +    return !document()->inPageCache() && !document()->view()->isOffscreen() && (!settings || settings->allowAnimatedImages());
> 
> Does this also affect static images, not just animating ones?

It shouldn't affect static images. As far as i can see willRenderImage() is only called from code dealing with animated images, but it's probably wise to check that the image is an animated one. What's a good way to check this, BitmapImage::frameCount() > 1?