Bug 9767 - WebPreference methods setAllowsAnimatedImages: and setAllowsAnimatedImageLooping: don't actually affect WebView behaviors
Summary: WebPreference methods setAllowsAnimatedImages: and setAllowsAnimatedImageLoop...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2006-07-06 15:35 PDT by Rachael Worthington (cheers)
Modified: 2012-09-04 10:02 PDT (History)
17 users (show)

See Also:


Attachments
Patch for AllowsAnimatedImages pref (49.32 KB, patch)
2012-02-13 17:49 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Fixed build errors (50.58 KB, patch)
2012-02-13 22:42 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Fixed chromium and gtk issues (54.76 KB, patch)
2012-02-14 12:04 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Missing fixes from last patch (54.76 KB, patch)
2012-02-14 13:18 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Build fix (54.76 KB, patch)
2012-02-14 14:09 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Yet another build fix (54.76 KB, patch)
2012-02-14 15:44 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Rename to AllowAnimatedImages (53.93 KB, patch)
2012-02-16 16:50 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Win build fix (54.81 KB, patch)
2012-02-22 10:57 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch (59.48 KB, patch)
2012-03-07 15:07 PST, Pablo Flouret
simon.fraser: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rachael Worthington (cheers) 2006-07-06 15:35:08 PDT
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.
Comment 1 david rosnow 2008-04-25 10:23:08 PDT
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.
Comment 2 Simon Fraser (smfr) 2009-01-07 21:17:33 PST
<rdar://problem/4679993>
Comment 3 Simon Fraser (smfr) 2009-01-07 21:20:48 PST
This just needs to be plumbed through from WebPreferences to Settings, and thence to the image code.
Comment 4 Pablo Flouret 2012-02-13 17:49:12 PST
Created attachment 126875 [details]
Patch for AllowsAnimatedImages pref

This bug should probably be split in two...
Comment 5 WebKit Review Bot 2012-02-13 17:53:27 PST
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 6 Pablo Flouret 2012-02-13 17:57:50 PST
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 7 WebKit Review Bot 2012-02-13 18:53:34 PST
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 8 Early Warning System Bot 2012-02-13 18:54:10 PST
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 9 Gyuyoung Kim 2012-02-13 22:24:38 PST
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
Comment 10 Pablo Flouret 2012-02-13 22:42:23 PST
Created attachment 126910 [details]
Fixed build errors
Comment 11 WebKit Review Bot 2012-02-13 22:45:46 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 12 Carlos Garcia Campos 2012-02-14 00:45:51 PST
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 13 Darin Fisher (:fishd, Google) 2012-02-14 11:20:58 PST
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.
Comment 14 Pablo Flouret 2012-02-14 12:04:48 PST
Created attachment 127013 [details]
Fixed chromium and gtk issues
Comment 15 Pablo Flouret 2012-02-14 12:11:03 PST
(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 16 WebKit Review Bot 2012-02-14 12:45:25 PST
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
Comment 17 Pablo Flouret 2012-02-14 13:18:18 PST
Created attachment 127026 [details]
Missing fixes from last patch
Comment 18 WebKit Review Bot 2012-02-14 13:57:09 PST
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
Comment 19 Pablo Flouret 2012-02-14 14:09:02 PST
Created attachment 127042 [details]
Build fix
Comment 20 WebKit Review Bot 2012-02-14 14:58:04 PST
Comment on attachment 127042 [details]
Build fix

Attachment 127042 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11514452
Comment 21 Pablo Flouret 2012-02-14 15:44:29 PST
Created attachment 127068 [details]
Yet another build fix
Comment 22 Carlos Garcia Campos 2012-02-14 23:51:29 PST
Comment on attachment 127068 [details]
Yet another build fix

GTK changes look good to me, thanks!
Comment 23 Darin Fisher (:fishd, Google) 2012-02-16 11:36:42 PST
(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 :-)
Comment 24 Pablo Flouret 2012-02-16 16:50:46 PST
Created attachment 127468 [details]
Rename to AllowAnimatedImages
Comment 25 Darin Fisher (:fishd, Google) 2012-02-17 11:35:21 PST
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?
Comment 26 Pablo Flouret 2012-02-17 11:47:41 PST
(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.
Comment 27 Darin Fisher (:fishd, Google) 2012-02-17 15:18:56 PST
(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.
Comment 28 Darin Fisher (:fishd, Google) 2012-02-17 15:20:23 PST
(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.
Comment 29 Pablo Flouret 2012-02-22 10:57:57 PST
Created attachment 128248 [details]
Win build fix
Comment 30 Pablo Flouret 2012-02-22 10:58:47 PST
Any idea of who i can poke to get the interesting parts of this patch reviewed?
Comment 31 Tim Horton 2012-03-06 17:38:21 PST
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.
Comment 32 Pablo Flouret 2012-03-06 17:45:40 PST
(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 33 Early Warning System Bot 2012-03-07 11:51:48 PST
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
Comment 34 Pablo Flouret 2012-03-07 15:07:14 PST
Created attachment 130705 [details]
Patch

Changed willRenderImage() return clause per Tim's comment, and hopefully fixed qt-wk2 and win issues.
Comment 35 Simon Fraser (smfr) 2012-04-19 14:48:01 PDT
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?
Comment 36 Pablo Flouret 2012-04-24 14:48:00 PDT
(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?