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.
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.
<rdar://problem/4679993>
This just needs to be plumbed through from WebPreferences to Settings, and thence to the image code.
Created attachment 126875 [details] Patch for AllowsAnimatedImages pref This bug should probably be split in two...
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
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.
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
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
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
Created attachment 126910 [details] Fixed build errors
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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).
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.
Created attachment 127013 [details] Fixed chromium and gtk issues
(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.
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
Created attachment 127026 [details] Missing fixes from last patch
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
Created attachment 127042 [details] Build fix
Comment on attachment 127042 [details] Build fix Attachment 127042 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11514452
Created attachment 127068 [details] Yet another build fix
Comment on attachment 127068 [details] Yet another build fix GTK changes look good to me, thanks!
(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 :-)
Created attachment 127468 [details] Rename to AllowAnimatedImages
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?
(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.
(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.
(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.
Created attachment 128248 [details] Win build fix
Any idea of who i can poke to get the interesting parts of this patch reviewed?
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.
(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 :-)
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
Created attachment 130705 [details] Patch Changed willRenderImage() return clause per Tim's comment, and hopefully fixed qt-wk2 and win issues.
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?
(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?