Changeset 194706 in webkit

Timestamp:

Jan 7, 2016 11:40:46 AM (8 years ago)

Author:

Chris Dumez

Message:

Directly-composited animated GIFs never resume once scrolled offscreen
​https://bugs.webkit.org/show_bug.cgi?id=152817
<rdar://problem/19982020>

Reviewed by Daniel Bates.

Source/WebCore:

Directly-composited animated GIFs would never resume once scrolled
offscreen. This is because calling repaint() in this case would not
cause BitmapImage::draw() to be called and the animation would thus
not be resumed. To address the problem,
repaintForPausedImageAnimationsIfNeeded() now calls
RenderBoxModelObject::contentChanged(ImageChanged) in addition to
repaint() to make sure the animation actually gets resumed, even in
the directly-composited animated GIF case.

Test: fast/images/composited-animated-gif-outside-viewport.html

• platform/graphics/BitmapImage.h:

Make currentFrame() public so it can be exposed via Internals for the
purpose of testing.

• rendering/RenderElement.cpp:

(WebCore::RenderElement::repaintForPausedImageAnimationsIfNeeded):
Call RenderBoxModelObject::contentChanged(ImageChanged) in addition to
calling repaint() to make sure the animation actually gets resumed in
the directly-composited animated GIFs case.

• testing/Internals.cpp:

(WebCore::Internals::imageFrameIndex):

• testing/Internals.h:
• testing/Internals.idl:

Expose new "unsigned long imageFrameIndex(Element)" operation on
Internals so layout tests can better check if an image is actually
animating. Previously, we would rely on the output of
internals.hasPausedImageAnimations(Element) but this is not sufficient
to cover this bug as our rendering code believed it has resumed the
animations but the GIF was not actually animating due to it being
directly-composited.

LayoutTests:

Add a layout test to check that directly-composited animated GIFs are
properly suspended / resumed based on visibility inside the viewport.

• fast/images/composited-animated-gif-outside-viewport-expected.txt: Added.
• fast/images/composited-animated-gif-outside-viewport.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/images/composited-animated-gif-outside-viewport-expected.txt (added)
• LayoutTests/fast/images/composited-animated-gif-outside-viewport.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/BitmapImage.h (modified) (2 diffs)
• Source/WebCore/rendering/RenderElement.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.cpp (modified) (2 diffs)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-01-07  Chris Dumez  <cdumez@apple.com>
+
+        Directly-composited animated GIFs never resume once scrolled offscreen
+        https://bugs.webkit.org/show_bug.cgi?id=152817
+        <rdar://problem/19982020>
+
+        Reviewed by Daniel Bates.
+
+        Add a layout test to check that directly-composited animated GIFs are
+        properly suspended / resumed based on visibility inside the viewport.
+
+        * fast/images/composited-animated-gif-outside-viewport-expected.txt: Added.
+        * fast/images/composited-animated-gif-outside-viewport.html: Added.
+
 2016-01-06  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-01-07  Chris Dumez  <cdumez@apple.com>
+
+        Directly-composited animated GIFs never resume once scrolled offscreen
+        https://bugs.webkit.org/show_bug.cgi?id=152817
+        <rdar://problem/19982020>
+
+        Reviewed by Daniel Bates.
+
+        Directly-composited animated GIFs would never resume once scrolled
+        offscreen. This is because calling repaint() in this case would not
+        cause BitmapImage::draw() to be called and the animation would thus
+        not be resumed. To address the problem,
+        repaintForPausedImageAnimationsIfNeeded() now calls
+        RenderBoxModelObject::contentChanged(ImageChanged) in addition to
+        repaint() to make sure the animation actually gets resumed, even in
+        the directly-composited animated GIF case.
+
+        Test: fast/images/composited-animated-gif-outside-viewport.html
+
+        * platform/graphics/BitmapImage.h:
+        Make currentFrame() public so it can be exposed via Internals for the
+        purpose of testing.
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::repaintForPausedImageAnimationsIfNeeded):
+        Call RenderBoxModelObject::contentChanged(ImageChanged) in addition to
+        calling repaint() to make sure the animation actually gets resumed in
+        the directly-composited animated GIFs case.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::imageFrameIndex):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+        Expose new "unsigned long imageFrameIndex(Element)" operation on
+        Internals so layout tests can better check if an image is actually
+        animating. Previously, we would rely on the output of
+        internals.hasPausedImageAnimations(Element) but this is not sufficient
+        to cover this bug as our rendering code believed it has resumed the
+        animations but the GIF was not actually animating due to it being
+        directly-composited.
+
 2016-01-07  Michael Catanzaro  <mcatanzaro@igalia.com>
 

trunk/Source/WebCore/platform/graphics/BitmapImage.h

@@ -184 +184 @@
     bool allowSubsampling() const { return m_allowSubsampling; }
     void setAllowSubsampling(bool allowSubsampling) { m_allowSubsampling = allowSubsampling; }
+
+    size_t currentFrame() const { return m_currentFrame; }
     
 private:
@@ -211 +213 @@
 #endif
 
-    size_t currentFrame() const { return m_currentFrame; }
     size_t frameCount();
 

trunk/Source/WebCore/rendering/RenderElement.cpp

@@ -1518 +1518 @@
     if (!shouldRepaintForImageAnimation(*this, visibleRect))
         return false;
+
     repaint();
+
+    // For directly-composited animated GIFs it does not suffice to call repaint() to resume animation. We need to mark the image as changed.
+    if (is<RenderBoxModelObject>(*this))
+        downcast<RenderBoxModelObject>(*this).contentChanged(ImageChanged);
+
     return true;
 }

trunk/Source/WebCore/testing/Internals.cpp

@@ -33 +33 @@
 #include "ApplicationCacheStorage.h"
 #include "BackForwardController.h"
+#include "BitmapImage.h"
 #include "CachedImage.h"
 #include "CachedResourceLoader.h"
@@ -589 +590 @@
 }
 
+size_t Internals::imageFrameIndex(Element* element, ExceptionCode& ec)
+{
+    if (!is<HTMLImageElement>(element)) {
+        ec = TypeError;
+        return 0;
+    }
+
+    auto* cachedImage = downcast<HTMLImageElement>(*element).cachedImage();
+    if (!cachedImage)
+        return 0;
+
+    auto* image = cachedImage->image();
+    return is<BitmapImage>(image) ? downcast<BitmapImage>(*image).currentFrame() : 0;
+}
+
 void Internals::clearPageCache()
 {

trunk/Source/WebCore/testing/Internals.h

@@ -105 +105 @@
     unsigned memoryCacheSize() const;
 
+    size_t imageFrameIndex(Element*, ExceptionCode&);
+
     void clearPageCache();
     unsigned pageCacheSize() const;

trunk/Source/WebCore/testing/Internals.idl

@@ -206 +206 @@
     [RaisesException] boolean isPageBoxVisible(long pageNumber);
 
+    [RaisesException] unsigned long imageFrameIndex(Element element);
+
     readonly attribute InternalSettings settings;
     readonly attribute unsigned long workerThreadCount;