Changeset 179340 in webkit

Timestamp:

Jan 29, 2015, 3:32:53 AM (12 years ago)

Author:

commit-queue@webkit.org

Message:

CachedImage: ensure clients overrides imageChanged instead of notifyFinished
​https://bugs.webkit.org/show_bug.cgi?id=140722

Patch by Julien Isorce <​j.isorce@samsung.com> on 2015-01-29
Reviewed by Tim Horton.

imageChanged is called whenever a frame of an image changes
because we got more data from the network.

notifyFinished was called when the image was entirely loaded.

The problem was that some clients were implementing only
imageChanged (ex: RenderBox), some only notifyFinished and
some both (ex: RenderImage) which made the situation difficult
to understand and to maintain.

For example when the image finished loading, both imageChanged
and notifyFinished were called with the difference that for the
first one isLoaded() returned false.
It could result in functions being called twice in a row,
ex: contentChanged(ImageChanged).

So this patch tries to simplify the situation by marking
CachedImageClient::notifyFinished final in order to prevent
clients from implementing it.
Indeed this patch ensure that CachedImage clients implement
and only implement imageChanged function.

Also Clients can now differentiate intermediate and end
calls by checking isLoaded() in imageChanged.

• html/HTMLImageLoader.cpp:

(WebCore::HTMLImageLoader::imageChanged): Added instead
of notifyFinished.
(WebCore::HTMLImageLoader::notifyFinished): Deleted.

• html/HTMLImageLoader.h:

• loader/ImageLoader.cpp:

(WebCore::ImageLoader::imageChanged): Added instead
of notifyFinished.
(WebCore::ImageLoader::notifyFinished): Deleted.

• loader/ImageLoader.h:

• loader/cache/CachedImage.cpp:

(WebCore::CachedImage::finishLoading): Explicilty mark image as
loaded and before notifying observers. So that it avoids to call
notifyFinished (from CachedResource::finishLoading).

• loader/cache/CachedImageClient.h:

Make CachedImageClient::notifyFinished final to make sure
sub classes implement imageChanged instead.

• rendering/RenderImage.cpp:

(WebCore::RenderImage::notifyFinished): Deleted.
ImageChanged already exists and is more clever than notifyFinished.
Indeed invalidateBackgroundObscurationStatus() will be called by
RenderReplaced::layout() upon call to setNeedsLayout() in
RenderImage::imageDimensionsChanged.
Also contentChanged(ImageChanged) is now called only when necessary.

• rendering/RenderImage.h:

• svg/SVGFEImageElement.cpp:

(WebCore::SVGFEImageElement::imageChanged): Added instead
of notifyFinished.
(WebCore::SVGFEImageElement::notifyFinished): Deleted.

• svg/SVGFEImageElement.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/HTMLImageLoader.cpp (modified) (1 diff)
• html/HTMLImageLoader.h (modified) (1 diff)
• loader/ImageLoader.cpp (modified) (3 diffs)
• loader/ImageLoader.h (modified) (1 diff)
• loader/cache/CachedImage.cpp (modified) (1 diff)
• loader/cache/CachedImageClient.h (modified) (1 diff)
• rendering/RenderImage.cpp (modified) (1 diff)
• rendering/RenderImage.h (modified) (1 diff)
• svg/SVGFEImageElement.cpp (modified) (1 diff)
• svg/SVGFEImageElement.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-01-29  Julien Isorce  <j.isorce@samsung.com>
+
+        CachedImage: ensure clients overrides imageChanged instead of notifyFinished
+        https://bugs.webkit.org/show_bug.cgi?id=140722
+
+        Reviewed by Tim Horton.
+
+        imageChanged is called whenever a frame of an image changes
+        because we got more data from the network.
+
+        notifyFinished was called when the image was entirely loaded.
+
+        The problem was that some clients were implementing only
+        imageChanged (ex: RenderBox), some only notifyFinished and
+        some both (ex: RenderImage) which made the situation difficult
+        to understand and to maintain.
+
+        For example when the image finished loading, both imageChanged
+        and notifyFinished were called with the difference that for the
+        first one isLoaded() returned false.
+        It could result in functions being called twice in a row,
+        ex: contentChanged(ImageChanged).
+
+        So this patch tries to simplify the situation by marking
+        CachedImageClient::notifyFinished final in order to prevent
+        clients from implementing it.
+        Indeed this patch ensure that CachedImage clients implement
+        and only implement imageChanged function.
+
+        Also Clients can now differentiate intermediate and end
+        calls by checking isLoaded() in imageChanged.
+
+        * html/HTMLImageLoader.cpp:
+        (WebCore::HTMLImageLoader::imageChanged): Added instead
+        of notifyFinished.
+        (WebCore::HTMLImageLoader::notifyFinished): Deleted.
+        * html/HTMLImageLoader.h:
+
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageLoader::imageChanged): Added instead
+        of notifyFinished.
+        (WebCore::ImageLoader::notifyFinished): Deleted.
+        * loader/ImageLoader.h:
+
+        * loader/cache/CachedImage.cpp:
+        (WebCore::CachedImage::finishLoading): Explicilty mark image as
+        loaded and before notifying observers. So that it avoids to call
+        notifyFinished (from CachedResource::finishLoading).
+
+        * loader/cache/CachedImageClient.h:
+        Make CachedImageClient::notifyFinished final to make sure
+        sub classes implement imageChanged instead.
+
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::notifyFinished): Deleted.
+        ImageChanged already exists and is more clever than notifyFinished.
+        Indeed invalidateBackgroundObscurationStatus() will be called by
+        RenderReplaced::layout() upon call to setNeedsLayout() in
+        RenderImage::imageDimensionsChanged.
+        Also contentChanged(ImageChanged) is now called only when necessary.
+        * rendering/RenderImage.h:
+
+        * svg/SVGFEImageElement.cpp:
+        (WebCore::SVGFEImageElement::imageChanged): Added instead
+        of notifyFinished.
+        (WebCore::SVGFEImageElement::notifyFinished): Deleted.
+        * svg/SVGFEImageElement.h:
+
 2015-01-28  Said Abou-Hallawa  <sabouhallawa@apple.com>
 

trunk/Source/WebCore/html/HTMLImageLoader.cpp

@@ -73 +73 @@
 }
 
-void HTMLImageLoader::notifyFinished(CachedResource*)
+void HTMLImageLoader::imageChanged(CachedImage* cachedImage, const IntRect*)
 {
-    CachedImage* cachedImage = image();
+    ASSERT(cachedImage == image().get());
+
+    if (!cachedImage->isLoaded())
+        return;
 
     Ref<Element> protect(element());
-    ImageLoader::notifyFinished(cachedImage);
+    ImageLoader::imageChanged(cachedImage);
 
     bool loadError = cachedImage->errorOccurred() || cachedImage->response().httpStatusCode() >= 400;

trunk/Source/WebCore/html/HTMLImageLoader.h

@@ -36 +36 @@
     virtual String sourceURI(const AtomicString&) const override;
 
-    virtual void notifyFinished(CachedResource*) override;
+    virtual void imageChanged(CachedImage*, const IntRect* = nullptr) override;
 };
 

trunk/Source/WebCore/loader/ImageLoader.cpp

@@ -273 +273 @@
 }
 
-void ImageLoader::notifyFinished(CachedResource* resource)
+void ImageLoader::imageChanged(CachedImage* cachedImage, const IntRect*)
 {
     ASSERT(m_failedLoadURL.isEmpty());
-    ASSERT(resource == m_image.get());
+    ASSERT(cachedImage == m_image.get());
+
+    if (!cachedImage->isLoaded())
+        return;
 
     m_imageComplete = true;
@@ -287 +290 @@
     if (element().fastHasAttribute(HTMLNames::crossoriginAttr)
         && !element().document().securityOrigin()->canRequest(image()->response().url())
-        && !resource->passesAccessControlCheck(element().document().securityOrigin())) {
+        && !cachedImage->passesAccessControlCheck(element().document().securityOrigin())) {
 
         setImageWithoutConsideringPendingLoadEvent(0);
@@ -305 +308 @@
     }
 
-    if (resource->wasCanceled()) {
+    if (cachedImage->wasCanceled()) {
         m_hasPendingLoadEvent = false;
         // Only consider updating the protection ref-count of the Element immediately before returning

trunk/Source/WebCore/loader/ImageLoader.h

@@ -74 +74 @@
 protected:
     explicit ImageLoader(Element&);
-    virtual void notifyFinished(CachedResource*) override;
+    virtual void imageChanged(CachedImage*, const IntRect* = nullptr) override;
 
 private:

trunk/Source/WebCore/loader/cache/CachedImage.cpp

@@ -427 +427 @@
     }
 
+    if (m_image)
+        setEncodedSize(m_image->data() ? m_image->data()->size() : 0);
+
+    setLoading(false);
     notifyObservers();
-    if (m_image)
-        setEncodedSize(m_image->data() ? m_image->data()->size() : 0);
-    CachedResource::finishLoading(data);
 }
 

trunk/Source/WebCore/loader/cache/CachedImageClient.h

@@ -39 +39 @@
     // Called whenever a frame of an image changes because we got more data from the network.
     // If not null, the IntRect is the changed rect of the image.
-    virtual void imageChanged(CachedImage*, const IntRect* = 0) { }
+    virtual void imageChanged(CachedImage*, const IntRect* = nullptr) { }
 
     // Called when GIF animation progresses.
     virtual void newImageAnimationFrameAvailable(CachedImage& image) { imageChanged(&image); }
+
+    // Use imageChanged instead.
+    virtual void notifyFinished(CachedResource*) override final { }
 };
 

trunk/Source/WebCore/rendering/RenderImage.cpp

@@ -366 +366 @@
 }
 
-void RenderImage::notifyFinished(CachedResource* newImage)
-{
-    if (documentBeingDestroyed())
-        return;
-
-    invalidateBackgroundObscurationStatus();
-
-    if (newImage == imageResource().cachedImage()) {
-        // tell any potential compositing layers
-        // that the image is done and they can reference it directly.
-        contentChanged(ImageChanged);
-    }
-}
-
 void RenderImage::paintReplaced(PaintInfo& paintInfo, const LayoutPoint& paintOffset)
 {

trunk/Source/WebCore/rendering/RenderImage.h

@@ -100 +100 @@
     virtual LayoutUnit minimumReplacedHeight() const override;
 
-    virtual void notifyFinished(CachedResource*) override final;
     virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, HitTestAction) override final;
 

trunk/Source/WebCore/svg/SVGFEImageElement.cpp

@@ -192 +192 @@
 }
 
-void SVGFEImageElement::notifyFinished(CachedResource*)
-{
+void SVGFEImageElement::imageChanged(CachedImage* cachedImage, const IntRect*)
+{
+    if (!cachedImage || !cachedImage->isLoaded())
+        return;
+
     if (!inDocument())
         return;

trunk/Source/WebCore/svg/SVGFEImageElement.h

@@ -52 +52 @@
     virtual void parseAttribute(const QualifiedName&, const AtomicString&) override;
     virtual void svgAttributeChanged(const QualifiedName&) override;
-    virtual void notifyFinished(CachedResource*) override;
+    virtual void imageChanged(CachedImage*, const IntRect* = nullptr) override;
 
     virtual void addSubresourceAttributeURLs(ListHashSet<URL>&) const override;