When destroying a resource, register "only" the clients who are losing their resource...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Oct 2017 20:34:27 +0000 (20:34 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Oct 2017 20:34:27 +0000 (20:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178567
<rdar://problem/35064781>

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2017-10-20
Reviewed by Simon Fraser.

SVGResources::resourceDestroyed() will return a bool indicating whether
it had a reference to the destroyed resource or not. If it returns true
SVGResourcesCache::resourceDestroyed() will register the client Element
as having pending resources.

* rendering/svg/SVGResources.cpp:
(WebCore::paintingResourceFromSVGPaint):
(WebCore::SVGResources::removeClientFromCache const):
(WebCore::SVGResources::resourceDestroyed):
(WebCore::SVGResources::buildSetOfResources):
(WebCore::SVGResources::resetClipper):
(WebCore::SVGResources::resetFilter):
(WebCore::SVGResources::resetMarkerStart):
(WebCore::SVGResources::resetMarkerMid):
(WebCore::SVGResources::resetMarkerEnd):
(WebCore::SVGResources::resetMasker):
(WebCore::SVGResources::resetFill):
(WebCore::SVGResources::resetStroke):
(WebCore::SVGResources::resetLinkedResource):
* rendering/svg/SVGResources.h:
(WebCore::SVGResources::isEmpty const):
(WebCore::SVGResources::ClipperFilterMaskerData::ClipperFilterMaskerData): Deleted.
(WebCore::SVGResources::MarkerData::MarkerData): Deleted.
(WebCore::SVGResources::FillStrokeData::FillStrokeData): Deleted.
* rendering/svg/SVGResourcesCache.cpp:
(WebCore::SVGResourcesCache::resourceDestroyed):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223789 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/SVGResources.cpp
Source/WebCore/rendering/svg/SVGResources.h
Source/WebCore/rendering/svg/SVGResourcesCache.cpp

index d3e624c..4f8892c 100644 (file)
@@ -1,3 +1,38 @@
+2017-10-20  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        When destroying a resource, register "only" the clients who are losing their resource as having pending resources
+        https://bugs.webkit.org/show_bug.cgi?id=178567
+        <rdar://problem/35064781>
+
+        Reviewed by Simon Fraser.
+
+        SVGResources::resourceDestroyed() will return a bool indicating whether 
+        it had a reference to the destroyed resource or not. If it returns true
+        SVGResourcesCache::resourceDestroyed() will register the client Element
+        as having pending resources.
+
+        * rendering/svg/SVGResources.cpp:
+        (WebCore::paintingResourceFromSVGPaint):
+        (WebCore::SVGResources::removeClientFromCache const):
+        (WebCore::SVGResources::resourceDestroyed):
+        (WebCore::SVGResources::buildSetOfResources):
+        (WebCore::SVGResources::resetClipper):
+        (WebCore::SVGResources::resetFilter):
+        (WebCore::SVGResources::resetMarkerStart):
+        (WebCore::SVGResources::resetMarkerMid):
+        (WebCore::SVGResources::resetMarkerEnd):
+        (WebCore::SVGResources::resetMasker):
+        (WebCore::SVGResources::resetFill):
+        (WebCore::SVGResources::resetStroke):
+        (WebCore::SVGResources::resetLinkedResource):
+        * rendering/svg/SVGResources.h:
+        (WebCore::SVGResources::isEmpty const):
+        (WebCore::SVGResources::ClipperFilterMaskerData::ClipperFilterMaskerData): Deleted.
+        (WebCore::SVGResources::MarkerData::MarkerData): Deleted.
+        (WebCore::SVGResources::FillStrokeData::FillStrokeData): Deleted.
+        * rendering/svg/SVGResourcesCache.cpp:
+        (WebCore::SVGResourcesCache::resourceDestroyed):
+
 2017-10-20  Ryosuke Niwa  <rniwa@webkit.org>
 
         Unify the node removal code in ContainerNode and expand the coverage of NoEventDispatchAssertion
index f0237e3..8ae61c8 100644 (file)
@@ -171,18 +171,18 @@ static inline bool isChainableResource(const SVGElement& element, const SVGEleme
 static inline RenderSVGResourceContainer* paintingResourceFromSVGPaint(Document& document, const SVGPaintType& paintType, const String& paintUri, AtomicString& id, bool& hasPendingResource)
 {
     if (paintType != SVG_PAINTTYPE_URI && paintType != SVG_PAINTTYPE_URI_RGBCOLOR && paintType != SVG_PAINTTYPE_URI_CURRENTCOLOR)
-        return 0;
+        return nullptr;
 
     id = SVGURIReference::fragmentIdentifierFromIRIString(paintUri, document);
     RenderSVGResourceContainer* container = getRenderSVGResourceContainerById(document, id);
     if (!container) {
         hasPendingResource = true;
-        return 0;
+        return nullptr;
     }
 
     RenderSVGResourceType resourceType = container->resourceType();
     if (resourceType != PatternResourceType && resourceType != LinearGradientResourceType && resourceType != RadialGradientResourceType)
-        return 0;
+        return nullptr;
 
     return container;
 }
@@ -330,7 +330,7 @@ bool SVGResources::markerReverseStart() const
 
 void SVGResources::removeClientFromCache(RenderElement& renderer, bool markForInvalidation) const
 {
-    if (!m_clipperFilterMaskerData && !m_markerData && !m_fillStrokeData && !m_linkedResource)
+    if (isEmpty())
         return;
 
     if (m_linkedResource) {
@@ -367,27 +367,29 @@ void SVGResources::removeClientFromCache(RenderElement& renderer, bool markForIn
     }
 }
 
-void SVGResources::resourceDestroyed(RenderSVGResourceContainer& resource)
+bool SVGResources::resourceDestroyed(RenderSVGResourceContainer& resource)
 {
-    if (!m_clipperFilterMaskerData && !m_markerData && !m_fillStrokeData && !m_linkedResource)
-        return;
+    if (isEmpty())
+        return false;
 
     if (m_linkedResource == &resource) {
         ASSERT(!m_clipperFilterMaskerData);
         ASSERT(!m_markerData);
         ASSERT(!m_fillStrokeData);
         m_linkedResource->removeAllClientsFromCache();
-        m_linkedResource = 0;
-        return;
+        m_linkedResource = nullptr;
+        return true;
     }
 
+    bool foundResources = false;
     switch (resource.resourceType()) {
     case MaskerResourceType:
         if (!m_clipperFilterMaskerData)
             break;
         if (m_clipperFilterMaskerData->masker == &resource) {
             m_clipperFilterMaskerData->masker->removeAllClientsFromCache();
-            m_clipperFilterMaskerData->masker = 0;
+            m_clipperFilterMaskerData->masker = nullptr;
+            foundResources = true;
         }
         break;
     case MarkerResourceType:
@@ -395,15 +397,18 @@ void SVGResources::resourceDestroyed(RenderSVGResourceContainer& resource)
             break;
         if (m_markerData->markerStart == &resource) {
             m_markerData->markerStart->removeAllClientsFromCache();
-            m_markerData->markerStart = 0;
+            m_markerData->markerStart = nullptr;
+            foundResources = true;
         }
         if (m_markerData->markerMid == &resource) {
             m_markerData->markerMid->removeAllClientsFromCache();
-            m_markerData->markerMid = 0;
+            m_markerData->markerMid = nullptr;
+            foundResources = true;
         }
         if (m_markerData->markerEnd == &resource) {
             m_markerData->markerEnd->removeAllClientsFromCache();
-            m_markerData->markerEnd = 0;
+            m_markerData->markerEnd = nullptr;
+            foundResources = true;
         }
         break;
     case PatternResourceType:
@@ -413,11 +418,13 @@ void SVGResources::resourceDestroyed(RenderSVGResourceContainer& resource)
             break;
         if (m_fillStrokeData->fill == &resource) {
             m_fillStrokeData->fill->removeAllClientsFromCache();
-            m_fillStrokeData->fill = 0;
+            m_fillStrokeData->fill = nullptr;
+            foundResources = true;
         }
         if (m_fillStrokeData->stroke == &resource) {
             m_fillStrokeData->stroke->removeAllClientsFromCache();
-            m_fillStrokeData->stroke = 0;
+            m_fillStrokeData->stroke = nullptr;
+            foundResources = true;
         }
         break;
     case FilterResourceType:
@@ -425,7 +432,8 @@ void SVGResources::resourceDestroyed(RenderSVGResourceContainer& resource)
             break;
         if (m_clipperFilterMaskerData->filter == &resource) {
             m_clipperFilterMaskerData->filter->removeAllClientsFromCache();
-            m_clipperFilterMaskerData->filter = 0;
+            m_clipperFilterMaskerData->filter = nullptr;
+            foundResources = true;
         }
         break;
     case ClipperResourceType:
@@ -433,17 +441,19 @@ void SVGResources::resourceDestroyed(RenderSVGResourceContainer& resource)
             break; 
         if (m_clipperFilterMaskerData->clipper == &resource) {
             m_clipperFilterMaskerData->clipper->removeAllClientsFromCache();
-            m_clipperFilterMaskerData->clipper = 0;
+            m_clipperFilterMaskerData->clipper = nullptr;
+            foundResources = true;
         }
         break;
     case SolidColorResourceType:
         ASSERT_NOT_REACHED();
     }
+    return foundResources;
 }
 
 void SVGResources::buildSetOfResources(HashSet<RenderSVGResourceContainer*>& set)
 {
-    if (!m_clipperFilterMaskerData && !m_markerData && !m_fillStrokeData && !m_linkedResource)
+    if (isEmpty())
         return;
 
     if (m_linkedResource) {
@@ -498,7 +508,7 @@ void SVGResources::resetClipper()
 {
     ASSERT(m_clipperFilterMaskerData);
     ASSERT(m_clipperFilterMaskerData->clipper);
-    m_clipperFilterMaskerData->clipper = 0;
+    m_clipperFilterMaskerData->clipper = nullptr;
 }
 
 bool SVGResources::setFilter(RenderSVGResourceFilter* filter)
@@ -519,7 +529,7 @@ void SVGResources::resetFilter()
 {
     ASSERT(m_clipperFilterMaskerData);
     ASSERT(m_clipperFilterMaskerData->filter);
-    m_clipperFilterMaskerData->filter = 0;
+    m_clipperFilterMaskerData->filter = nullptr;
 }
 
 bool SVGResources::setMarkerStart(RenderSVGResourceMarker* markerStart)
@@ -540,7 +550,7 @@ void SVGResources::resetMarkerStart()
 {
     ASSERT(m_markerData);
     ASSERT(m_markerData->markerStart);
-    m_markerData->markerStart = 0;
+    m_markerData->markerStart = nullptr;
 }
 
 bool SVGResources::setMarkerMid(RenderSVGResourceMarker* markerMid)
@@ -561,7 +571,7 @@ void SVGResources::resetMarkerMid()
 {
     ASSERT(m_markerData);
     ASSERT(m_markerData->markerMid);
-    m_markerData->markerMid = 0;
+    m_markerData->markerMid = nullptr;
 }
 
 bool SVGResources::setMarkerEnd(RenderSVGResourceMarker* markerEnd)
@@ -582,7 +592,7 @@ void SVGResources::resetMarkerEnd()
 {
     ASSERT(m_markerData);
     ASSERT(m_markerData->markerEnd);
-    m_markerData->markerEnd = 0;
+    m_markerData->markerEnd = nullptr;
 }
 
 bool SVGResources::setMasker(RenderSVGResourceMasker* masker)
@@ -603,7 +613,7 @@ void SVGResources::resetMasker()
 {
     ASSERT(m_clipperFilterMaskerData);
     ASSERT(m_clipperFilterMaskerData->masker);
-    m_clipperFilterMaskerData->masker = 0;
+    m_clipperFilterMaskerData->masker = nullptr;
 }
 
 bool SVGResources::setFill(RenderSVGResourceContainer* fill)
@@ -626,7 +636,7 @@ void SVGResources::resetFill()
 {
     ASSERT(m_fillStrokeData);
     ASSERT(m_fillStrokeData->fill);
-    m_fillStrokeData->fill = 0;
+    m_fillStrokeData->fill = nullptr;
 }
 
 bool SVGResources::setStroke(RenderSVGResourceContainer* stroke)
@@ -649,7 +659,7 @@ void SVGResources::resetStroke()
 {
     ASSERT(m_fillStrokeData);
     ASSERT(m_fillStrokeData->stroke);
-    m_fillStrokeData->stroke = 0;
+    m_fillStrokeData->stroke = nullptr;
 }
 
 bool SVGResources::setLinkedResource(RenderSVGResourceContainer* linkedResource)
@@ -664,7 +674,7 @@ bool SVGResources::setLinkedResource(RenderSVGResourceContainer* linkedResource)
 void SVGResources::resetLinkedResource()
 {
     ASSERT(m_linkedResource);
-    m_linkedResource = 0;
+    m_linkedResource = nullptr;
 }
 
 #if ENABLE(TREE_DEBUGGING)
index 365db1b..c8b4734 100644 (file)
@@ -67,7 +67,8 @@ public:
 
     // Methods operating on all cached resources
     void removeClientFromCache(RenderElement&, bool markForInvalidation = true) const;
-    void resourceDestroyed(RenderSVGResourceContainer&);
+    // Returns true if the resource-to-be-destroyed is one of our resources.
+    bool resourceDestroyed(RenderSVGResourceContainer&);
 
 #if ENABLE(TREE_DEBUGGING)
     void dump(const RenderObject*);
@@ -97,6 +98,8 @@ private:
     bool setFill(RenderSVGResourceContainer*);
     bool setStroke(RenderSVGResourceContainer*);
     bool setLinkedResource(RenderSVGResourceContainer*);
+    
+    bool isEmpty() const { return !m_clipperFilterMaskerData && !m_markerData && !m_fillStrokeData && !m_linkedResource; }
 
     // From SVG 1.1 2nd Edition
     // clipper: 'container elements' and 'graphics elements'
@@ -106,16 +109,10 @@ private:
     struct ClipperFilterMaskerData {
         WTF_MAKE_FAST_ALLOCATED;
     public:
-        ClipperFilterMaskerData()
-            : clipper(0)
-            , filter(0)
-            , masker(0)
-        {
-        }
-
-        RenderSVGResourceClipper* clipper;
-        RenderSVGResourceFilter* filter;
-        RenderSVGResourceMasker* masker;
+        ClipperFilterMaskerData() = default;
+        RenderSVGResourceClipper* clipper { nullptr };
+        RenderSVGResourceFilter* filter { nullptr };
+        RenderSVGResourceMasker* masker { nullptr };
     };
 
     // From SVG 1.1 2nd Edition
@@ -123,16 +120,10 @@ private:
     struct MarkerData {
         WTF_MAKE_FAST_ALLOCATED;
     public:
-        MarkerData()
-            : markerStart(0)
-            , markerMid(0)
-            , markerEnd(0)
-        {
-        }
-
-        RenderSVGResourceMarker* markerStart;
-        RenderSVGResourceMarker* markerMid;
-        RenderSVGResourceMarker* markerEnd;
+        MarkerData() = default;
+        RenderSVGResourceMarker* markerStart { nullptr };
+        RenderSVGResourceMarker* markerMid { nullptr };
+        RenderSVGResourceMarker* markerEnd { nullptr };
     };
 
     // From SVG 1.1 2nd Edition
@@ -142,14 +133,9 @@ private:
     struct FillStrokeData {
         WTF_MAKE_FAST_ALLOCATED;
     public:
-        FillStrokeData()
-            : fill(0)
-            , stroke(0)
-        {
-        }
-
-        RenderSVGResourceContainer* fill;
-        RenderSVGResourceContainer* stroke;
+        FillStrokeData() = default;
+        RenderSVGResourceContainer* fill { nullptr };
+        RenderSVGResourceContainer* stroke { nullptr };
     };
 
     std::unique_ptr<ClipperFilterMaskerData> m_clipperFilterMaskerData;
index f15d974..961adaf 100644 (file)
@@ -161,12 +161,12 @@ void SVGResourcesCache::resourceDestroyed(RenderSVGResourceContainer& resource)
     cache.removeResourcesFromRenderer(resource);
 
     for (auto& it : cache.m_cache) {
-        it.value->resourceDestroyed(resource);
-
-        // Mark users of destroyed resources as pending resolution based on the id of the old resource.
-        Element& resourceElement = resource.element();
-        Element* clientElement = it.key->element();
-        clientElement->document().accessSVGExtensions().addPendingResource(resourceElement.getIdAttribute(), clientElement);
+        if (it.value->resourceDestroyed(resource)) {
+            // Mark users of destroyed resources as pending resolution based on the id of the old resource.
+            Element& resourceElement = resource.element();
+            Element* clientElement = it.key->element();
+            clientElement->document().accessSVGExtensions().addPendingResource(resourceElement.getIdAttribute(), clientElement);
+        }
     }
 }