27238 2009-07-13 14:53:34 -0700 Animated GIF dynamically removed and readded to page does not re-animate 2026-03-03 12:03:43 -0800 1 1 1 Unclassified WebKit Images 528+ (Nightly build) PC All NEW InRadar P2 Normal --- 1 pkasting pkasting bfulgham dglazkov japhet kbolisetty koivisto mrahaman mustaf.here pkasting ravi.kasibhatla rudloff spottabathini vswap65 webkit-bug-importer webkit.review.bot oldest_to_newest 131415 0 32681 pkasting 2009-07-13 14:53:34 -0700 Created attachment 32681 Testcase Open the attached testcase. Hitting "Play" repeatedly should repeatedly animate the image. This works fine in Firefox/IE but fails in Safari/Google Chrome. 445104 1 vswap65 2011-08-01 22:20:00 -0700 Hi I am Working on this issue. 449072 2 103461 vswap65 2011-08-10 03:56:29 -0700 Created attachment 103461 Added resetAnimation() fuction call in order to restart animation For each click, it is creating one pair of instances of HTMLImageElement & RenderImage but they all share the same BitmapImage instance for animation. For the first click of play button , image is getting animated. If we click for 2nd time, it again calls for BitmapImage::startAnimation() but because the animation is currently continuing, the function just returns without doing anything. But the expected behavior is that for each click, all the existing image animations has to be restarted as IE/FF does. So, therefore we need to somehow call resetAnimation() for the above behavior in each button click & that's why when the new image being appended, we are calling resetAnimation(). I have thought about all the corner cases to make sure that this change will not have any side effect. Please let me know your opinion. I have not yet added the test case for this. If the code changes are fine, I will add the test case & upload a final patch. 449076 3 103461 webkit.review.bot 2011-08-10 04:10:10 -0700 Comment on attachment 103461 Added resetAnimation() fuction call in order to restart animation Attachment 103461 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9339590 New failing tests: http/tests/inspector/console-resource-errors.html http/tests/inspector/network/disabled-cache-crash.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/network/network-cachedresources-with-same-urls.html http/tests/inspector/console-cd.html http/tests/inspector/change-iframe-src.html http/tests/inspector/console-cd-completions.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network/download.html http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html animations/animation-add-events-in-handler.html http/tests/inspector/network-preflight-options.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-websocket-error.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html http/tests/inspector/network/network-clear-after-disabled.html 449236 4 103461 pkasting 2011-08-10 10:49:51 -0700 Comment on attachment 103461 Added resetAnimation() fuction call in order to restart animation View in context: https://bugs.webkit.org/attachment.cgi?id=103461&action=review Does this change the current animation behavior of dynamically inserting a second copy of an image into a page that already contains one copy? (I don't remember whether that resets animation or not, but whichever way it goes is intentional.) > Source/WebCore/html/HTMLImageElement.cpp:220 > m_imageLoader.updateFromElement(); Does this guarantee that the line below will not deref NULL? 449646 5 vswap65 2011-08-11 00:36:24 -0700 Hi Peter, It doesn't change the current animation behavior. In resetAnimation() function , only the animation values are getting reset. So it doesn't deref NULL. 449647 6 vswap65 2011-08-11 00:47:27 -0700 Hi Peter, If the above comment satisfies you, let me know. I will upload the patch along with the testcases. 449960 7 pkasting 2011-08-11 13:02:21 -0700 (In reply to comment #5) > It doesn't change the current animation behavior. So, the current behavior is that adding a second copy of the image to the page resets the first and causes both to animate from the beginning? > In resetAnimation() function , only the animation values are getting reset. > So it doesn't deref NULL. ? This doesn't answer my question. Right above the call you added we handle the case of m_imageLoader.image() returning NULL. Then your new code promptly derefs that pointer blindly, along with the pointer it returns. I want to know if that's safe. That has nothing to do with what resetAnimation() does. 450311 8 vswap65 2011-08-11 23:31:58 -0700 (In reply to comment #7) > (In reply to comment #5) > > It doesn't change the current animation behavior. > > So, the current behavior is that adding a second copy of the image to the page resets the first and causes both to animate from the beginning? Yes, the current behavior (with my patch) would be the same as you explained above i.e. adding a second copy of the image to the page will reset the first & both will start animation from the beginning. > > > In resetAnimation() function , only the animation values are getting reset. > > So it doesn't deref NULL. > > ? This doesn't answer my question. Right above the call you added we handle the case of m_imageLoader.image() returning NULL. Then your new code promptly derefs that pointer blindly, along with the pointer it returns. I want to know if that's safe. That has nothing to do with what resetAnimation() does. I now understand your question. if m_imageLoader.image() is NULL, m_imageLoader.updateFromElement() populates the m_imageLoader.image(). Therefore the code we have added is safe. Still to be absolutely sure, we can add NULL check before our code also. It will not affect the functionality 450492 9 pkasting 2011-08-12 10:28:51 -0700 (In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #5) > > > In resetAnimation() function , only the animation values are getting reset. > > > So it doesn't deref NULL. > > > > ? This doesn't answer my question. Right above the call you added we handle the case of m_imageLoader.image() returning NULL. Then your new code promptly derefs that pointer blindly, along with the pointer it returns. I want to know if that's safe. That has nothing to do with what resetAnimation() does. > > I now understand your question. if m_imageLoader.image() is NULL, m_imageLoader.updateFromElement() populates the m_imageLoader.image(). Therefore the code we have added is safe. And when m_imageLoader.image() is non-NULL, m_imageLoader.image().image() is guaranteed to be non-NULL? > Still to be absolutely sure, we can add NULL check before our code also. It will not affect the functionality If this really cannot be NULL, we should assert (or do nothing), rather than implying it can be NULL by adding a conditional. 451529 10 vswap65 2011-08-16 02:08:18 -0700 (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #5) > > > > In resetAnimation() function , only the animation values are getting reset. > > > > So it doesn't deref NULL. > > > > > > ? This doesn't answer my question. Right above the call you added we handle the case of m_imageLoader.image() returning NULL. Then your new code promptly derefs that pointer blindly, along with the pointer it returns. I want to know if that's safe. That has nothing to do with what resetAnimation() does. > > > > I now understand your question. if m_imageLoader.image() is NULL, m_imageLoader.updateFromElement() populates the m_imageLoader.image(). Therefore the code we have added is safe. > > And when m_imageLoader.image() is non-NULL, m_imageLoader.image().image() is guaranteed to be non-NULL? Yes, correct. m_imageLoader.image().image() is never NULL. > > > Still to be absolutely sure, we can add NULL check before our code also. It will not affect the functionality > > If this really cannot be NULL, we should assert (or do nothing), rather than implying it can be NULL by adding a conditional. Therefore, we will do nothing before executing resetAnimation() [no NULL checking required] Also, I have one more doubt regarding test cases. I saw few test cases failing for Cr-linux though they did not seem to be related to my changes. Do you have any idea what could be the reason? And Do i need to add any test cases for my change? Because the expected behavior here is to see all the images animating (visual changes) so I was not sure how to write a test cases for my changes. 451734 11 pkasting 2011-08-16 10:51:49 -0700 (In reply to comment #10) > Also, I have one more doubt regarding test cases. I saw few test cases failing for Cr-linux though they did not seem to be related to my changes. Do you have any idea what could be the reason? No. > And Do i need to add any test cases for my change? Because the expected behavior here is to see all the images animating (visual changes) so I was not sure how to write a test cases for my changes. I'm not totally sure either. A simple pixel test is going to show the image sitting on its final frame both with and without your patch. Perhaps your reviewer will have an idea I don't. 453704 12 vswap65 2011-08-19 01:49:42 -0700 Hi Peter, thank you for your comments. Can any one review the patch, and let me know if any further changes needed. 473905 13 108837 ravi.kasibhatla 2011-09-27 06:32:18 -0700 Created attachment 108837 Fix for GIF not animating on dynamic detach & attach pkasting: Attached is a patch which fixes the issue. Patch ensures that any detach of the image element from the DOM also ensures the image associated with the element (CachedResourceClient) is unregistered with the CachedResource which was created while loading the image element. If the CachedResource is not being referenced by any other client it is purged. So, when the image element is reattached, it becomes new client which is registered with the CachedResource and animation state is reset for it. 473918 14 108837 webkit.review.bot 2011-09-27 07:02:35 -0700 Comment on attachment 108837 Fix for GIF not animating on dynamic detach & attach Attachment 108837 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9878337 New failing tests: platform/mac/editing/deleting/deletionUI-differing-background.html 473977 15 pkasting 2011-09-27 09:20:06 -0700 (In reply to comment #13) > Patch ensures that any detach of the image element from the DOM also ensures the image associated with the element (CachedResourceClient) is unregistered with the CachedResource which was created while loading the image element. My instinct is that this is a more correct fix. Unfortunately I don't understand too much about how resource loading and caching actually works so I can't say for sure (and I'm not a reviewer anyway). You might want to ask whomever worked on CachedImage? 474074 16 ravi.kasibhatla 2011-09-27 10:55:24 -0700 Hi Nate, Can you please review the attached patch (https://bugs.webkit.org/attachment.cgi?id=108837) and let me know your comments if any? 474095 17 108837 japhet 2011-09-27 11:14:44 -0700 Comment on attachment 108837 Fix for GIF not animating on dynamic detach & attach View in context: https://bugs.webkit.org/attachment.cgi?id=108837&action=review Antti knows the CachedResource<->CachedResourceClient interface better than I do, so I cc:ed him. One comment inline, though. > Source/WebCore/html/HTMLImageElement.cpp:225 > > + // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references. > + if (m_imageLoader.image()) > + m_imageLoader.image()->removeClient(&m_imageLoader); > + Currently, ImageLoader only calls CachedImage::removeClient() when it clears its pointer to the CachedImage. Since ImageLoader holds a CachedResourceHandle<CachedImage> rather than a raw CachedImage*, this patch as written will cause the CachedImage to be keep alive by an object that can't receive its callbacks. I don't think that's desirable, but maybe I'm missing something. This should probably just be: m_imageLoader.setImage(0); 474527 18 spottabathini 2011-09-27 23:27:18 -0700 > Source/WebCore/html/HTMLImageElement.cpp:225 > > + // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references. > + if (m_imageLoader.image()) > + m_imageLoader.image()->removeClient(&m_imageLoader); > + Hi , this is Swapna. I submitted patch earlier with gmail id "vswap65@gmail.com" While I tested the test case with above changes, I am still able to see the issue. 474690 19 109010 ravi.kasibhatla 2011-09-28 05:54:25 -0700 Created attachment 109010 Fix for GIF not animating on dynamic detach & attach - revision 2 Addressing the comment from Nate. 474694 20 ravi.kasibhatla 2011-09-28 05:58:03 -0700 (In reply to comment #18) > > Source/WebCore/html/HTMLImageElement.cpp:225 > > > > + // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references. > > + if (m_imageLoader.image()) > > + m_imageLoader.image()->removeClient(&m_imageLoader); > > + > > > Hi , this is Swapna. I submitted patch earlier with gmail id "vswap65@gmail.com" > > While I tested the test case with above changes, I am still able to see the issue. Hi Swapna, I have verified the previous patch & current patch on Chromium & GTK ports (on linux) and it is working. Ran layout tests also to ensure no regression with the patch. 474717 21 spottabathini 2011-09-28 06:42:48 -0700 Hi , I tested on windows webkit with winlauncer set up with the above patch , for some reason I don't see it working. Did any one tested on the same environment? If I click on play continuously it is showing animation for some extent . But after that , it is not animating for click events. 475460 22 koivisto 2011-09-29 09:33:27 -0700 Who sets the m_imageLoader back if the same element is re-added to a document? Can this cause us reload the image in that case? 475472 23 109010 koivisto 2011-09-29 09:46:54 -0700 Comment on attachment 109010 Fix for GIF not animating on dynamic detach & attach - revision 2 I suspect this can trigger network reload of the image if the element is re-added to the document (in case it is expired, has no-store, etc). I don't see why bug about restarting animation needs a change with such a broad potential side effects. 475495 24 mrahaman 2011-09-29 10:07:10 -0700 Hi Antti Koivisto, Could you also please review the patch uploaded by Swapna (vswap65) where we are doing resetting the animation? We had verified that the fix is solving the issue for Windows build. Thanks. 475505 25 109010 pkasting 2011-09-29 10:24:54 -0700 Comment on attachment 109010 Fix for GIF not animating on dynamic detach & attach - revision 2 (In reply to comment #23) > (From update of attachment 109010 [details]) > I suspect this can trigger network reload of the image if the element is re-added to the document (in case it is expired, has no-store, etc). I don't see why bug about restarting animation needs a change with such a broad potential side effects. Actually I think this is precisely the correct behavior. When the image is removed from the document, it should really be removed; any caches should not still hang onto it. If when re-added later that means we have to redownload it that's perfectly fine. I think you should rethink your opposition to this patch. 475506 26 103461 pkasting 2011-09-29 10:25:48 -0700 Comment on attachment 103461 Added resetAnimation() fuction call in order to restart animation I am convinced this patch is wrong. 475508 27 mrahaman 2011-09-29 10:31:44 -0700 (In reply to comment #26) > (From update of attachment 103461 [details]) > I am convinced this patch is wrong. Hi Peter, When Swapna uploaded the patch, there were few follow-up discussion regarding your comments and she tried to answer all of your queries. So, may be we missed to realize that the fix was not proper but it would certainly help from our knowledge point of view that in what aspect, you think the patch is wrong? 475509 28 pkasting 2011-09-29 10:33:42 -0700 (In reply to comment #27) > in what aspect, you think the patch is wrong? The fact that something like the other patch is the right way to go. That patch makes it clear that we were not re-animating because we were not fully dropping everyone's references to the image. We should be. 475618 29 koivisto 2011-09-29 12:48:38 -0700 (In reply to comment #25) > Actually I think this is precisely the correct behavior. When the image is removed from the document, it should really be removed; any caches should not still hang onto it. If when re-added later that means we have to redownload it that's perfectly fine. Why? 475704 30 pkasting 2011-09-29 15:01:08 -0700 (In reply to comment #29) > (In reply to comment #25) > > Actually I think this is precisely the correct behavior. When the image is removed from the document, it should really be removed; any caches should not still hang onto it. If when re-added later that means we have to redownload it that's perfectly fine. > > Why? Which part are you asking why about? The reason I think we should drop all references when removing an image from a document is that otherwise they act sort of like we're leaking them -- we aren't displaying the image anymore but we're still keeping data about it alive. Re-adding an image to a document later does not seem qualitatively different to me than some other way of adding it, e.g. if the document simply waited ten seconds after load and then dynamically added an image. In that case the image would clearly be subject to loading off the network etc. The desired behavior here seems like a symptom of the fact that we want browsers to treat these sorts of dynamic re-additions consistently with other additions, and achieving it by ensuring no data structures outlive the original removal seems like the right theoretical behavior that also has the right practical effect here, as opposed to a narrowly-targeted patch to just implement re-animation, which is more of a hack. 475930 31 koivisto 2011-09-29 23:46:28 -0700 (In reply to comment #30) > The reason I think we should drop all references when removing an image from a document is that otherwise they act sort of like we're leaking them -- we aren't displaying the image anymore but we're still keeping data about it alive. If the HTMLImageElement is staying alive after being removed from he tree then most likely a Javascript wrapper is holding a reference to it. In that case (ignoring gc delay) there is a decent chance that it will get reattached. I see no obvious reason to dump the cached data while it is outside of the tree. Word "leak" has specific meaning which does not apply here. > Re-adding an image to a document later does not seem qualitatively different to me than some other way of adding it, e.g. if the document simply waited ten seconds after load and then dynamically added an image. In that case the image would clearly be subject to loading off the network etc. Moving an item seems qualitatively different to creating an item to me. > The desired behavior here seems like a symptom of the fact that we want browsers to treat these sorts of dynamic re-additions consistently with other additions, and achieving it by ensuring no data structures outlive the original removal seems like the right theoretical behavior that also has the right practical effect here, as opposed to a narrowly-targeted patch to just implement re-animation, which is more of a hack. I'm not convinced by this theory. What do other engines do? 475943 32 mrahaman 2011-09-30 00:02:25 -0700 Is it also worth to investigate as why this patch is not solving the issue for windows port? 476000 33 spottabathini 2011-09-30 03:09:10 -0700 @@ -219,6 +219,10 @@ void HTMLImageElement::removedFromDocument() document->removeExtraNamedItem(m_id); } + // Unregister with CachedImage when removing image element from DOM to ensure CachedImage is not holding dead references. + if (m_imageLoader.image()) + m_imageLoader.setImage(0); + I have tried setting up GTK build and verified with above changes, behaviour on GTK Linux is same as Windows environment. i.e,Image stops animating immediately after few clicks. Patch proposed by me is actually solving the issue on both Win & GTK ports, i will try to enhance the same considering Resource Leaks from the above conversation. 476352 34 109358 pkasting 2011-09-30 15:42:41 -0700 Created attachment 109358 Second testcase (In reply to comment #31) > (In reply to comment #30) > > The desired behavior here seems like a symptom of the fact that we want browsers to treat these sorts of dynamic re-additions consistently with other additions, and achieving it by ensuring no data structures outlive the original removal seems like the right theoretical behavior that also has the right practical effect here, as opposed to a narrowly-targeted patch to just implement re-animation, which is more of a hack. > > I'm not convinced by this theory. What do other engines do? I've attached a second testcase that is useful for understanding this. I tested the behavior five ways, and drew conclusions based on those as well as the fact that all engines re-animate the original testcase on hitting play: (1) With the stock testcase. This differs from the original in two ways. First, we have an <img> element pointing to the same src, to test how image objects and elements that point to the same src are linked. Second, we don't recreate the image object -- we simply dynamically add and remove it from the document. (2) Removing the <img> element from the testcase, leaving only the image object. This tests how a single object behaves when dynamically removed and re-added. (3) Restoring the <img> element but changing the image object to be recreated on each call to run() (like the first testcase did). This tests whether dropping the old image object ref is meaningful in the presence of a long-lived <img>. (4) Moving the "var img" declaration entirely into run() and making done() do nothing, so we continually append new image objects. (5) Removing the appendChild() call in run(), so it merely creates new image objects, but doesn't display them. This tests whether object creation or addition to the DOM is what resets animation state. Firefox, IE, and Opera disagree about most things here, except that they all animate all images on load. Firefox: (1) Nothing animates on hitting play. (2) Nothing animates on hitting play. (3) Both images re-animate every time you hit play. (4) Each time you hit play, there is a new image, and all the images re-animate. (5) Each time you hit play, the image re-animates. Conclusion: The animation states of all the <img> elements and image objects with the same src attributes are linked. This linked animation state is reset each time a new image object is created. Addition to the DOM or destruction of old objects are both irrelevant, it's object creation that resets state. IE: (1) The image object animates on hitting play, but the <img> element does not. (2) The image object animates on hitting play. (3) Nothing animates on hitting play. (4) Nothing animates on hitting play. (5) Nothing animates on hitting play. Conclusion: This behavior makes no sense. Result (1) suggests that animation states for objects versus elements are tracked separately, yet result (3) (which differs from the original testcase only by the presence of an <img> element) suggests that they are linked together. I cannot come up with an explanation for how this behavior is possible. Opera: (1) Nothing animates on hitting play. (2) The image object animates on hitting play. (3) Nothing animates on hitting play. (4) Nothing animates on hitting play. (5) Nothing animates on hitting play. Conclusion: The animation states of all the <img> elements and image objects with the same src attributes are linked. This linked animation state is reset when we add an object to the DOM, but only if there are no other linked objects in the DOM already. Firefox and Opera both arguably make some sense. I would probably prefer to adopt the Firefox behavior. If we want to do that, we'll need to make some changes to our animation code. Further testing confirms that if you create a brand new animated image object (to a src that no other object or element has), then wait, and only later add it to the DOM, Firefox begins animating it on creation, so on the later addition, the image appears midway through its animation. In WebKit I believe we don't trigger animation to start until we first draw() the image. This would have to change to instead start animation on object creation. I _think_ the "image invisible, skip frames" logic in BitmapImage.cpp would mean this wouldn't have any CPU load effects, it would merely make the animation start timing be set at load instead of when first drawing, so this change would be feasible. 476354 35 109359 pkasting 2011-09-30 15:43:25 -0700 Created attachment 109359 Real second testcase Oops, attached wrong file. 484801 36 kbolisetty 2011-10-16 12:30:39 -0700 Hi All, the above comparison and conclusions on different browser’s is quite useful. Based on the above note and some more testing, I am sharing my observations. In WebKit also, all the resources with same src attributes are linked. Infact this is what is the CachedResource we get from docloader after cache validation. I tried to create/append a non-animating image with same src attribute. I could clearly see the images getting appended to the document. Infact I could see the animating Image also getting appended properly to the document. But Since it is a cached Image (cached animating image) and it is already done with the animation, renderer find 0 pending frames of the animation image to paint, and hence nothing is painted except for the first time. I tried the following algorithm: 1. Check if the Resource being attached to DOM Tree is a cachedResource. 2. If it is a cached resource, check if it is animation Image. ( Image framecount>1 only for animation Image. So I used this to find if it’s a animating image or not) 3. If it is a cached animating Image, only then reset the animation. If this is making sense, I can provide the patch based on the above Algo. Thanks. 484883 37 pkasting 2011-10-16 19:37:53 -0700 That proposal wouldn't make us match Gecko. If I understand correctly it would affect animation at the time of DOM attachment rather than image object creation; and it also wouldn't change the timings as described at the very end of comment 34. 485022 38 kbolisetty 2011-10-17 02:46:33 -0700 (In reply to comment #37) > That proposal wouldn't make us match Gecko. If I understand correctly it would affect animation at the time of DOM attachment rather than image object creation; and it also wouldn't change the timings as described at the very end of comment 34. The Proposal Actually is in Sync with Gecko(for restting animation), but little deviation that favours toward webkit rendering approach. #1> I am resetting the animation during object creation. I mean when src attribute is initialized. #2> As you know, WebKit engine will actually start the animation while rendering and not at the time of object creation. Even though, FireFox is resetting and starting animation at the time of Image creation, but I am in favour of WebKit behaviour as mentioned in #2 above(ie., starting animation while rendering), because there is no point in starting animation when the created image is never attached to DOM. Here is the svn diff of my proposal: $ svn diff Source/WebCore/loader/ImageLoader.cpp Index: Source/WebCore/loader/ImageLoader.cpp =================================================================== --- Source/WebCore/loader/ImageLoader.cpp (revision 97439) +++ Source/WebCore/loader/ImageLoader.cpp (working copy) @@ -218,6 +218,12 @@ if (RenderImageResource* imageResource = renderImageResource()) imageResource->resetAnimation(); + + // TODO:: Kishore To Check if this should be done before Resource Load T rigger from Memory Cache. + // TODO:: CachedResourceLoader::requestResource() -> USE -> notifyLoaded FromMemoryCache(resource) + if(newImage->status() == CachedResource::Cached && newImage->image()->is AnimationImage()) { + newImage->image()->resetAnimation(); + } } void ImageLoader::updateFromElementIgnoringPreviousError() kbolisetty@IM-LP-093 ~/WebKit $ svn diff Surce/WebCore/platform/graphics/Image.h Index: Source/WebCore/platform/graphics/Image.h =================================================================== --- Source/WebCore/platform/graphics/Image.h (revision 97439) +++ Source/WebCore/platform/graphics/Image.h (working copy) @@ -124,6 +124,7 @@ virtual void startAnimation(bool /*catchUpIfNecessary*/ = true) { } virtual void stopAnimation() {} virtual void resetAnimation() {} + virtual bool isAnimationImage() const {return false;} // Typically the CachedImage that owns us. ImageObserver* imageObserver() const { return m_imageObserver; } kbolisetty@IM-LP-093 ~/WebKit $ svn diff Source/WebCore/platform/graphics/BitmapImage.h Index: Source/WebCore/platform/graphics/BitmapImage.h =================================================================== --- Source/WebCore/platform/graphics/BitmapImage.h (revision 97439) +++ Source/WebCore/platform/graphics/BitmapImage.h (working copy) @@ -126,7 +126,7 @@ // automatically pause once all observers no longer want to render the imag e anywhere. virtual void stopAnimation(); virtual void resetAnimation(); - + virtual bool isAnimationImage() const { return (m_frameCount>1)?true:false; } virtual unsigned decodedSize() const { return m_decodedSize; } #if PLATFORM(MAC) kbolisetty@IM-LP-093 ~/WebKit 485262 39 pkasting 2011-10-17 10:41:56 -0700 (In reply to comment #38) > Even though, FireFox is resetting and starting animation at the time of Image creation, but I am in favour of WebKit behaviour as mentioned in #2 above(ie., starting animation while rendering), because there is no point in starting animation when the created image is never attached to DOM. Yes there is; to keep timings synced between different images. For example, if you want to dynamically swap animated images for each other and have them be at the same point in the animation stream, this is useful. Note that we won't actually animate the image while it's detached, we'd just want to set the start time of the animation correctly. That will make us spin forward the animation by the right amount when we finally do draw the image. > Here is the svn diff of my proposal: Please don't put diffs inline. If you want to show people a diff attach it to the bug as a patch. 485265 40 pkasting 2011-10-17 10:44:24 -0700 (In reply to comment #39) > (In reply to comment #38) > > Even though, FireFox is resetting and starting animation at the time of Image creation, but I am in favour of WebKit behaviour as mentioned in #2 above(ie., starting animation while rendering), because there is no point in starting animation when the created image is never attached to DOM. > > Yes there is; to keep timings synced between different images. And, of course: to match Gecko. We want browser behaviors to converge to a single common behavior so that authors can write something once and have it work everywhere. Gratuitously differing from other rendering engines doesn't advance this goal. The point of documenting other engines' behavior was to try and pick a behavior that matched at least one, not to leave us in a state where we don't match anything. 486881 41 pkasting 2011-10-19 10:45:14 -0700 I don't know whether fixing https://bugzilla.mozilla.org/show_bug.cgi?id=573583 has affected Firefox' behavior. Someone might want to retry with a trunk build to see. 532513 42 109010 eric 2012-01-09 14:02:07 -0800 Comment on attachment 109010 Fix for GIF not animating on dynamic detach & attach - revision 2 Seems like this only fixes the issue if there is only one copy of the gif on the page. If there is more than one copy and we only remove/re-add one of them, then that one will still not reset. Is that expected behavior? 545136 43 rudloff 2012-01-30 08:50:11 -0800 The same problem appears with animated SVG. Should I fill a new bug ? 545564 44 109010 eric 2012-01-30 16:21:13 -0800 Comment on attachment 109010 Fix for GIF not animating on dynamic detach & attach - revision 2 Need an answer to my question about multiple copies of the same gif on the page (and if we care about that case). 2186647 45 webkit-bug-importer 2026-03-03 12:03:43 -0800 <rdar://problem/171648233> 32681 2009-07-13 14:53:34 -0700 2009-07-13 14:53:34 -0700 Testcase test.html text/html 297 pkasting PHNjcmlwdD4KdmFyIGltZzsKZnVuY3Rpb24gcnVuKCkgewogIGltZyA9IG5ldyBJbWFnZSgpOwog IGltZy5zcmMgPSAiaHR0cDovL3d3dy5nb29nbGUuY29tL2ltYWdlcy9zd3hhLmdpZiI7CiAgZG9j dW1lbnQuYm9keS5hcHBlbmRDaGlsZChpbWcpOwogIHdpbmRvdy5zZXRUaW1lb3V0KGRvbmUsIDc1 MCk7Cn0KZnVuY3Rpb24gZG9uZSgpIHsKICBkb2N1bWVudC5ib2R5LnJlbW92ZUNoaWxkKGltZyk7 Cn0KPC9zY3JpcHQ+CjxpbnB1dCB0eXBlPWJ1dHRvbiB2YWx1ZT1QbGF5IG9uY2xpY2s9InJ1bigp Ij4KPGJyPjxicj4K 103461 2011-08-10 03:56:29 -0700 2011-09-29 10:25:48 -0700 Added resetAnimation() fuction call in order to restart animation Propose_Patch_27238 text/plain 1260 vswap65 SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkyNzYxKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDgtMTAgIFN3YXBuYSBQ ICA8dnN3YXA2NUBnbWFpbC5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgQW5pbWF0ZWQgR0lGIGR5bmFtaWNhbGx5IHJlbW92ZWQgYW5kIHJlYWRk ZWQgdG8gcGFnZSBkb2VzIG5vdCByZS1hbmltYXRlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzIzOAorCisgICAgICAgIEFkZGVkIHJlc2V0QW5pbWF0 aW9uKCkgZnVjdGlvbiBjYWxsIGluIG9yZGVyIHRvIHJlc3RhcnQgYW5pbWF0aW9uIAorICAgICAg ICBmb3IgdGhlIHN1YnNlcXVlbnQgY2xpY2sgZXZlbnRzIGFmdGVyIGZpcnN0IGNsaWNrIGV2ZW50 LgorICAgICAgICAKKyAgICAgICAgKiBodG1sL0hUTUxJbWFnZUVsZW1lbnQuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6SFRNTEltYWdlRWxlbWVudDo6aW5zZXJ0ZWRJbnRvRG9jdW1lbnQpOgorCiAy MDExLTA4LTEwICBZdXRhIEtpdGFtdXJhICA8eXV0YWtAY2hyb21pdW0ub3JnPgogCiAgICAgICAg IFdlYlNvY2tldDogQWRkIGJpbmFyeVR5cGUgYXR0cmlidXRlCkluZGV4OiBTb3VyY2UvV2ViQ29y ZS9odG1sL0hUTUxJbWFnZUVsZW1lbnQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3Jl L2h0bWwvSFRNTEltYWdlRWxlbWVudC5jcHAJKHJldmlzaW9uIDkwNTU1KQorKysgU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjE5LDYg KzIxOSw4IEBAIHZvaWQgSFRNTEltYWdlRWxlbWVudDo6aW5zZXJ0ZWRJbnRvRG9jdW0KICAgICBp ZiAoIW1faW1hZ2VMb2FkZXIuaW1hZ2UoKSkKICAgICAgICAgbV9pbWFnZUxvYWRlci51cGRhdGVG cm9tRWxlbWVudCgpOwogCisgICAgbV9pbWFnZUxvYWRlci5pbWFnZSgpLT5pbWFnZSgpLT5yZXNl dEFuaW1hdGlvbigpOworCiAgICAgSFRNTEVsZW1lbnQ6Omluc2VydGVkSW50b0RvY3VtZW50KCk7 CiB9CiAK 108837 2011-09-27 06:32:18 -0700 2011-09-28 05:54:25 -0700 Fix for GIF not animating on dynamic detach & attach 27238_fix_rev1.diff text/plain 1770 ravi.kasibhatla ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwMDIzOGU1Li4wNDBhN2FkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkg QEAKKzIwMTEtMDktMjcgIFJhdmkgUGhhbmVlbmRyYSBLYXNpYmhhdGxhICA8cmF2aS5rYXNpYmhh dGxhQG1vdG9yb2xhLmNvbT4KKworICAgICAgICBBbmltYXRlZCBHSUYgZHluYW1pY2FsbHkgcmVt b3ZlZCBhbmQgcmVhZGRlZCB0byBwYWdlIGRvZXMgbm90IHJlLWFuaW1hdGUKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI3MjM4CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hlbiBhIGltYWdlIGVsZW1lbnQg YmVpbmcgcmVtb3ZlZCBmcm9tIHRoZSBET00sIHRoZW4gaXQgc2hvdWxkIGJlIHVucmVnaXN0ZXJl ZCB3aXRoIHRoZQorICAgICAgICBDYWNoZWRSZXNvdXJjZSBiZWNhdXNlIGl0IGhvbGRzIHJlZmVy ZW5jZXMgb2YgYWxsIGNsaWVudHMgd2hvIGFyZSB1c2luZyB0aGF0IGNhY2hlZCBpbWFnZS4KKyAg ICAgICAgTm90IGRvaW5nIHNvIGNhdXNlcyBDYWNoZWRJbWFnZSB0byBhc3N1bWUgdGhlIGltYWdl IGVsZW1lbnQgaXMgc3RpbGwgdXNpbmcgdGhlIHJlc291cmNlCisgICAgICAgIHdoaWNoIG1pZ2h0 IGNhdXNlIGxlYWtzIG9yIGFzIGluIHRoaXMgY2FzZSBkb2Vzbid0IHJlc2V0IGFuaW1hdGlvbnMg d2hlbiByZWF0dGFjaGluZy4KKworICAgICAgICAqIGh0bWwvSFRNTEltYWdlRWxlbWVudC5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpIVE1MSW1hZ2VFbGVtZW50OjpyZW1vdmVkRnJvbURvY3VtZW50 KTogVW5yZWdpc3RlciBpdHNlbGYgd2l0aCBDYWNoZWRSZXNvdXJjZSB3aGVuIGJlaW5nIHJlbW92 ZWQKKyAgICAgICAgZnJvbSB0aGUgZG9jdW1lbnQuCisKIDIwMTEtMDktMDkgIFNpbW9uIEZyYXNl ciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgUGl4ZWwgcmVzdWx0IHNob3dz IHRoYXQgY29tcG9zaXRpbmcvaWZyYW1lcy9yZXBhaW50LWFmdGVyLWxvc2luZy1zY3JvbGxiYXJz Lmh0bWwgaXMgZmFpbGluZwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1h Z2VFbGVtZW50LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVudC5jcHAK aW5kZXggMzU3ZjExNmEuLmMxZmJjYzIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwv SFRNTEltYWdlRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VF bGVtZW50LmNwcApAQCAtMjE5LDYgKzIxOSwxMCBAQCB2b2lkIEhUTUxJbWFnZUVsZW1lbnQ6OnJl bW92ZWRGcm9tRG9jdW1lbnQoKQogICAgICAgICBkb2N1bWVudC0+cmVtb3ZlRXh0cmFOYW1lZEl0 ZW0obV9pZCk7CiAgICAgfQogCisgICAgLy8gVW5yZWdpc3RlciB3aXRoIENhY2hlZEltYWdlIHdo ZW4gcmVtb3ZpbmcgaW1hZ2UgZWxlbWVudCBmcm9tIERPTSB0byBlbnN1cmUgQ2FjaGVkSW1hZ2Ug aXMgbm90IGhvbGRpbmcgZGVhZCByZWZlcmVuY2VzLgorICAgIGlmIChtX2ltYWdlTG9hZGVyLmlt YWdlKCkpCisgICAgICAgIG1faW1hZ2VMb2FkZXIuaW1hZ2UoKS0+cmVtb3ZlQ2xpZW50KCZtX2lt YWdlTG9hZGVyKTsKKwogICAgIEhUTUxFbGVtZW50OjpyZW1vdmVkRnJvbURvY3VtZW50KCk7CiB9 CiAK 109010 2011-09-28 05:54:25 -0700 2012-01-30 16:21:13 -0800 Fix for GIF not animating on dynamic detach & attach - revision 2 27238_fix_rev2.diff text/plain 1744 ravi.kasibhatla ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwMDIzOGU1Li4wNDBhN2FkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTkg QEAKKzIwMTEtMDktMjcgIFJhdmkgUGhhbmVlbmRyYSBLYXNpYmhhdGxhICA8cmF2aS5rYXNpYmhh dGxhQG1vdG9yb2xhLmNvbT4KKworICAgICAgICBBbmltYXRlZCBHSUYgZHluYW1pY2FsbHkgcmVt b3ZlZCBhbmQgcmVhZGRlZCB0byBwYWdlIGRvZXMgbm90IHJlLWFuaW1hdGUKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI3MjM4CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hlbiBhIGltYWdlIGVsZW1lbnQg YmVpbmcgcmVtb3ZlZCBmcm9tIHRoZSBET00sIHRoZW4gaXQgc2hvdWxkIGJlIHVucmVnaXN0ZXJl ZCB3aXRoIHRoZQorICAgICAgICBDYWNoZWRSZXNvdXJjZSBiZWNhdXNlIGl0IGhvbGRzIHJlZmVy ZW5jZXMgb2YgYWxsIGNsaWVudHMgd2hvIGFyZSB1c2luZyB0aGF0IGNhY2hlZCBpbWFnZS4KKyAg ICAgICAgTm90IGRvaW5nIHNvIGNhdXNlcyBDYWNoZWRJbWFnZSB0byBhc3N1bWUgdGhlIGltYWdl IGVsZW1lbnQgaXMgc3RpbGwgdXNpbmcgdGhlIHJlc291cmNlCisgICAgICAgIHdoaWNoIG1pZ2h0 IGNhdXNlIGxlYWtzIG9yIGFzIGluIHRoaXMgY2FzZSBkb2Vzbid0IHJlc2V0IGFuaW1hdGlvbnMg d2hlbiByZWF0dGFjaGluZy4KKworICAgICAgICAqIGh0bWwvSFRNTEltYWdlRWxlbWVudC5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpIVE1MSW1hZ2VFbGVtZW50OjpyZW1vdmVkRnJvbURvY3VtZW50 KTogVW5yZWdpc3RlciBpdHNlbGYgd2l0aCBDYWNoZWRSZXNvdXJjZSB3aGVuIGJlaW5nIHJlbW92 ZWQKKyAgICAgICAgZnJvbSB0aGUgZG9jdW1lbnQuCisKIDIwMTEtMDktMDkgIFNpbW9uIEZyYXNl ciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgUGl4ZWwgcmVzdWx0IHNob3dz IHRoYXQgY29tcG9zaXRpbmcvaWZyYW1lcy9yZXBhaW50LWFmdGVyLWxvc2luZy1zY3JvbGxiYXJz Lmh0bWwgaXMgZmFpbGluZwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1h Z2VFbGVtZW50LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVudC5jcHAK aW5kZXggMzU3ZjExNmEuLmNiNjcxZWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2h0bWwv SFRNTEltYWdlRWxlbWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MSW1hZ2VF bGVtZW50LmNwcApAQCAtMjE5LDYgKzIxOSwxMCBAQCB2b2lkIEhUTUxJbWFnZUVsZW1lbnQ6OnJl bW92ZWRGcm9tRG9jdW1lbnQoKQogICAgICAgICBkb2N1bWVudC0+cmVtb3ZlRXh0cmFOYW1lZEl0 ZW0obV9pZCk7CiAgICAgfQogCisgICAgLy8gVW5yZWdpc3RlciB3aXRoIENhY2hlZEltYWdlIHdo ZW4gcmVtb3ZpbmcgaW1hZ2UgZWxlbWVudCBmcm9tIERPTSB0byBlbnN1cmUgQ2FjaGVkSW1hZ2Ug aXMgbm90IGhvbGRpbmcgZGVhZCByZWZlcmVuY2VzLgorICAgIGlmIChtX2ltYWdlTG9hZGVyLmlt YWdlKCkpCisgICAgICAgIG1faW1hZ2VMb2FkZXIuc2V0SW1hZ2UoMCk7CisKICAgICBIVE1MRWxl bWVudDo6cmVtb3ZlZEZyb21Eb2N1bWVudCgpOwogfQogCg== 109358 2011-09-30 15:42:41 -0700 2011-09-30 15:43:25 -0700 Second testcase foo.html text/html 318 pkasting PHNjcmlwdD4NCiAgICB2YXIgaW1nOw0KZnVuY3Rpb24gcnVuKCkgew0KICAgIGltZyA9IG5ldyBJ bWFnZSgpOw0KICAgIGltZy5zcmMgPSAiaHR0cDovL3d3dy5nb29nbGUuY29tL2ltYWdlcy9zd3hh LmdpZiI7DQogICAgd2luZG93LnNldFRpbWVvdXQoZG9uZSwgNDAwKTsNCn0NCmZ1bmN0aW9uIGRv bmUoKSB7DQogICAgZG9jdW1lbnQuYm9keS5hcHBlbmRDaGlsZChpbWcpOw0KfQ0KPC9zY3JpcHQ+ DQo8Ym9keSBvbmxvYWQ9InJ1bigpIj4NCjxpbnB1dCB0eXBlPWJ1dHRvbiB2YWx1ZT1QbGF5IG9u Y2xpY2s9InJ1bigpIj4NCjxicj48YnI+DQo8L2JvZHk+ 109359 2011-09-30 15:43:25 -0700 2011-09-30 15:43:25 -0700 Real second testcase foo.html text/html 382 pkasting PHNjcmlwdD4NCnZhciBpbWcgPSBuZXcgSW1hZ2UoKTsNCmltZy5zcmMgPSAiaHR0cDovL3d3dy5n b29nbGUuY29tL2ltYWdlcy9zd3hhLmdpZiI7DQpmdW5jdGlvbiBydW4oKSB7DQogIGRvY3VtZW50 LmJvZHkuYXBwZW5kQ2hpbGQoaW1nKTsNCiAgd2luZG93LnNldFRpbWVvdXQoZG9uZSwgNzUwKTsN Cn0NCmZ1bmN0aW9uIGRvbmUoKSB7DQogIGRvY3VtZW50LmJvZHkucmVtb3ZlQ2hpbGQoaW1nKTsN Cn0NCjwvc2NyaXB0Pg0KPGJvZHkgb25sb2FkPSJydW4oKSI+DQo8aW1nIHNyYz0iaHR0cDovL3d3 dy5nb29nbGUuY29tL2ltYWdlcy9zd3hhLmdpZiI+DQo8aW5wdXQgdHlwZT1idXR0b24gdmFsdWU9 UGxheSBvbmNsaWNrPSJydW4oKSI+DQo8YnI+PGJyPg0KPC9ib2R5Pg==