Avoid updateFromElement() usage in SVG
authorzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 26 May 2012 09:08:14 +0000 (09:08 +0000)
committerzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 26 May 2012 09:08:14 +0000 (09:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=87573

Stop relying on updateFromElement() - instead rely on addChild/removeChild, which
allows us to optimize the resources re-fetching. When a child is added to the tree
we don't need to remove existing resources from the SVGResourcesCache - the renderer
can't be in the cache yet. Similary, remove the entry from the cache earlier: as soon
as the renderer is removed from the tree, instead of waiting for willBeDestroyed().

No new tests, refactoring only.

* rendering/svg/RenderSVGBlock.cpp:
* rendering/svg/RenderSVGBlock.h:
(RenderSVGBlock):
* rendering/svg/RenderSVGContainer.cpp:
(WebCore::RenderSVGContainer::addChild):
(WebCore):
(WebCore::RenderSVGContainer::removeChild):
* rendering/svg/RenderSVGContainer.h:
(RenderSVGContainer):
* rendering/svg/RenderSVGInline.cpp:
(WebCore::RenderSVGInline::addChild):
(WebCore::RenderSVGInline::removeChild):
* rendering/svg/RenderSVGInline.h:
(RenderSVGInline):
* rendering/svg/RenderSVGModelObject.cpp:
* rendering/svg/RenderSVGModelObject.h:
(RenderSVGModelObject):
* rendering/svg/RenderSVGResourceContainer.cpp:
(WebCore::RenderSVGResourceContainer::registerResource):
* rendering/svg/RenderSVGRoot.cpp:
(WebCore::RenderSVGRoot::addChild):
(WebCore):
(WebCore::RenderSVGRoot::removeChild):
* rendering/svg/RenderSVGRoot.h:
(RenderSVGRoot):
* rendering/svg/RenderSVGText.cpp:
(WebCore::RenderSVGText::addChild):
(WebCore::RenderSVGText::removeChild):
* rendering/svg/SVGResourcesCache.cpp:
(WebCore::SVGResourcesCache::clientStyleChanged):
(WebCore::rendererCanHaveResources):
(WebCore):
(WebCore::SVGResourcesCache::clientWasAddedToTree):
(WebCore::SVGResourcesCache::clientWillBeRemovedFromTree):
* rendering/svg/SVGResourcesCache.h:
(SVGResourcesCache):
* svg/SVGStyledElement.cpp:
* svg/SVGStyledElement.h:
(SVGStyledElement):

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

17 files changed:
Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/RenderSVGBlock.cpp
Source/WebCore/rendering/svg/RenderSVGBlock.h
Source/WebCore/rendering/svg/RenderSVGContainer.cpp
Source/WebCore/rendering/svg/RenderSVGContainer.h
Source/WebCore/rendering/svg/RenderSVGInline.cpp
Source/WebCore/rendering/svg/RenderSVGInline.h
Source/WebCore/rendering/svg/RenderSVGModelObject.cpp
Source/WebCore/rendering/svg/RenderSVGModelObject.h
Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp
Source/WebCore/rendering/svg/RenderSVGRoot.cpp
Source/WebCore/rendering/svg/RenderSVGRoot.h
Source/WebCore/rendering/svg/RenderSVGText.cpp
Source/WebCore/rendering/svg/SVGResourcesCache.cpp
Source/WebCore/rendering/svg/SVGResourcesCache.h
Source/WebCore/svg/SVGStyledElement.cpp
Source/WebCore/svg/SVGStyledElement.h

index 5385cb6..b0e8280 100644 (file)
@@ -1,3 +1,56 @@
+2012-05-26  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        Avoid updateFromElement() usage in SVG
+        https://bugs.webkit.org/show_bug.cgi?id=87573
+
+        Stop relying on updateFromElement() - instead rely on addChild/removeChild, which
+        allows us to optimize the resources re-fetching. When a child is added to the tree
+        we don't need to remove existing resources from the SVGResourcesCache - the renderer
+        can't be in the cache yet. Similary, remove the entry from the cache earlier: as soon
+        as the renderer is removed from the tree, instead of waiting for willBeDestroyed().
+
+        No new tests, refactoring only.
+
+        * rendering/svg/RenderSVGBlock.cpp:
+        * rendering/svg/RenderSVGBlock.h:
+        (RenderSVGBlock):
+        * rendering/svg/RenderSVGContainer.cpp:
+        (WebCore::RenderSVGContainer::addChild):
+        (WebCore):
+        (WebCore::RenderSVGContainer::removeChild):
+        * rendering/svg/RenderSVGContainer.h:
+        (RenderSVGContainer):
+        * rendering/svg/RenderSVGInline.cpp:
+        (WebCore::RenderSVGInline::addChild):
+        (WebCore::RenderSVGInline::removeChild):
+        * rendering/svg/RenderSVGInline.h:
+        (RenderSVGInline):
+        * rendering/svg/RenderSVGModelObject.cpp:
+        * rendering/svg/RenderSVGModelObject.h:
+        (RenderSVGModelObject):
+        * rendering/svg/RenderSVGResourceContainer.cpp:
+        (WebCore::RenderSVGResourceContainer::registerResource):
+        * rendering/svg/RenderSVGRoot.cpp:
+        (WebCore::RenderSVGRoot::addChild):
+        (WebCore):
+        (WebCore::RenderSVGRoot::removeChild):
+        * rendering/svg/RenderSVGRoot.h:
+        (RenderSVGRoot):
+        * rendering/svg/RenderSVGText.cpp:
+        (WebCore::RenderSVGText::addChild):
+        (WebCore::RenderSVGText::removeChild):
+        * rendering/svg/SVGResourcesCache.cpp:
+        (WebCore::SVGResourcesCache::clientStyleChanged):
+        (WebCore::rendererCanHaveResources):
+        (WebCore):
+        (WebCore::SVGResourcesCache::clientWasAddedToTree):
+        (WebCore::SVGResourcesCache::clientWillBeRemovedFromTree):
+        * rendering/svg/SVGResourcesCache.h:
+        (SVGResourcesCache):
+        * svg/SVGStyledElement.cpp:
+        * svg/SVGStyledElement.h:
+        (SVGStyledElement):
+
 2012-05-25  Nat Duca  <nduca@chromium.org>
 
         [chromium] Instrument V8 GC with TraceEvent
index cf44eff..c4f9a28 100644 (file)
@@ -104,12 +104,6 @@ void RenderSVGBlock::styleDidChange(StyleDifference diff, const RenderStyle* old
     SVGResourcesCache::clientStyleChanged(this, diff, style());
 }
 
-void RenderSVGBlock::updateFromElement()
-{
-    RenderBlock::updateFromElement();
-    SVGResourcesCache::clientUpdatedFromElement(this, style());
-}
-
 }
 
 #endif
index 9ef55a9..7f5b785 100644 (file)
@@ -45,7 +45,6 @@ private:
 
     virtual void styleWillChange(StyleDifference, const RenderStyle* newStyle);
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
-    virtual void updateFromElement();
 };
 
 }
index a1fd832..7b28661 100644 (file)
@@ -87,6 +87,19 @@ void RenderSVGContainer::layout()
     setNeedsLayout(false);
 }
 
+void RenderSVGContainer::addChild(RenderObject* child, RenderObject* beforeChild)
+{
+    RenderSVGModelObject::addChild(child, beforeChild);
+    SVGResourcesCache::clientWasAddedToTree(child, child->style());
+}
+
+void RenderSVGContainer::removeChild(RenderObject* child)
+{
+    SVGResourcesCache::clientWillBeRemovedFromTree(child);
+    RenderSVGModelObject::removeChild(child);
+}
+
+
 bool RenderSVGContainer::selfWillPaint()
 {
     SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(this);
index 5be0366..335732b 100644 (file)
@@ -53,6 +53,8 @@ protected:
 
     virtual void layout();
 
+    virtual void addChild(RenderObject* child, RenderObject* beforeChild = 0) OVERRIDE;
+    virtual void removeChild(RenderObject*) OVERRIDE;
     virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&);
 
     virtual FloatRect objectBoundingBox() const { return m_objectBoundingBox; }
index bf099ad..057bfda 100644 (file)
@@ -119,21 +119,19 @@ void RenderSVGInline::styleDidChange(StyleDifference diff, const RenderStyle* ol
     SVGResourcesCache::clientStyleChanged(this, diff, style());
 }
 
-void RenderSVGInline::updateFromElement()
-{
-    RenderInline::updateFromElement();
-    SVGResourcesCache::clientUpdatedFromElement(this, style());
-}
-
 void RenderSVGInline::addChild(RenderObject* child, RenderObject* beforeChild)
 {
     RenderInline::addChild(child, beforeChild);
+    SVGResourcesCache::clientWasAddedToTree(child, child->style());
+
     if (RenderSVGText* textRenderer = RenderSVGText::locateRenderSVGTextAncestor(this))
         textRenderer->subtreeChildWasAdded(child);
 }
 
 void RenderSVGInline::removeChild(RenderObject* child)
 {
+    SVGResourcesCache::clientWillBeRemovedFromTree(child);
+
     RenderSVGText* textRenderer = RenderSVGText::locateRenderSVGTextAncestor(this);
     if (!textRenderer) {
         RenderInline::removeChild(child);
index 1dc5a2e..533a99a 100644 (file)
@@ -57,9 +57,8 @@ private:
     virtual void willBeDestroyed();
     virtual void styleWillChange(StyleDifference, const RenderStyle* newStyle);
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
-    virtual void updateFromElement();
 
-    virtual void addChild(RenderObject* child, RenderObject* beforeChild = 0);
+    virtual void addChild(RenderObject* child, RenderObject* beforeChild = 0) OVERRIDE;
     virtual void removeChild(RenderObject*) OVERRIDE;
 };
 
index 2912591..c1c27b0 100644 (file)
@@ -111,12 +111,6 @@ void RenderSVGModelObject::styleDidChange(StyleDifference diff, const RenderStyl
     SVGResourcesCache::clientStyleChanged(this, diff, style());
 }
 
-void RenderSVGModelObject::updateFromElement()
-{
-    RenderObject::updateFromElement();
-    SVGResourcesCache::clientUpdatedFromElement(this, style());
-}
-
 bool RenderSVGModelObject::nodeAtPoint(const HitTestRequest&, HitTestResult&, const LayoutPoint&, const LayoutPoint&, HitTestAction)
 {
     ASSERT_NOT_REACHED();
index 5717e98..d82600f 100644 (file)
@@ -62,7 +62,6 @@ public:
     virtual const RenderObject* pushMappingToContainer(const RenderBoxModelObject* ancestorToStopAt, RenderGeometryMap&) const;
     virtual void styleWillChange(StyleDifference, const RenderStyle* newStyle);
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
-    virtual void updateFromElement();
 
     static bool checkIntersection(RenderObject*, const FloatRect&);
     static bool checkEnclosure(RenderObject*, const FloatRect&);
index 3dba672..80760fd 100644 (file)
@@ -165,7 +165,7 @@ void RenderSVGResourceContainer::registerResource()
         RenderObject* renderer = (*it)->renderer();
         if (!renderer)
             continue;
-        SVGResourcesCache::clientUpdatedFromElement(renderer, renderer->style());
+        SVGResourcesCache::clientStyleChanged(renderer, StyleDifferenceLayout, renderer->style());
         renderer->setNeedsLayout(true);
     }
 }
index 1bdfe28..6632453 100644 (file)
@@ -326,10 +326,16 @@ void RenderSVGRoot::styleDidChange(StyleDifference diff, const RenderStyle* oldS
     SVGResourcesCache::clientStyleChanged(this, diff, style());
 }
 
-void RenderSVGRoot::updateFromElement()
+void RenderSVGRoot::addChild(RenderObject* child, RenderObject* beforeChild)
 {
-    RenderReplaced::updateFromElement();
-    SVGResourcesCache::clientUpdatedFromElement(this, style());
+    RenderReplaced::addChild(child, beforeChild);
+    SVGResourcesCache::clientWasAddedToTree(child, child->style());
+}
+
+void RenderSVGRoot::removeChild(RenderObject* child)
+{
+    SVGResourcesCache::clientWillBeRemovedFromTree(child);
+    RenderReplaced::removeChild(child);
 }
 
 // RenderBox methods will expect coordinates w/o any transforms in coordinates
index 05b1d22..6fb5694 100644 (file)
@@ -78,7 +78,8 @@ private:
     virtual void willBeDestroyed();
     virtual void styleWillChange(StyleDifference, const RenderStyle* newStyle);
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
-    virtual void updateFromElement();
+    virtual void addChild(RenderObject* child, RenderObject* beforeChild = 0) OVERRIDE;
+    virtual void removeChild(RenderObject*) OVERRIDE;
 
     virtual const AffineTransform& localToParentTransform() const;
 
index 6583b2f..a2cae7d 100644 (file)
@@ -529,11 +529,15 @@ FloatRect RenderSVGText::repaintRectInLocalCoordinates() const
 void RenderSVGText::addChild(RenderObject* child, RenderObject* beforeChild)
 {
     RenderSVGBlock::addChild(child, beforeChild);
+
+    SVGResourcesCache::clientWasAddedToTree(child, child->style());
     subtreeChildWasAdded(child);
 }
 
 void RenderSVGText::removeChild(RenderObject* child)
 {
+    SVGResourcesCache::clientWillBeRemovedFromTree(child);
+
     Vector<SVGTextLayoutAttributes*, 2> affectedAttributes;
     FontCachePurgePreventer fontCachePurgePreventer;
     subtreeChildWillBeRemoved(child, affectedAttributes);
index 1715f6c..87531ed 100644 (file)
@@ -128,24 +128,44 @@ void SVGResourcesCache::clientLayoutChanged(RenderObject* object)
 void SVGResourcesCache::clientStyleChanged(RenderObject* renderer, StyleDifference diff, const RenderStyle* newStyle)
 {
     ASSERT(renderer);
-    if (diff == StyleDifferenceEqual)
+    if (diff == StyleDifferenceEqual || !renderer->parent())
         return;
 
     // In this case the proper SVGFE*Element will decide whether the modified CSS properties require a relayout or repaint.
     if (renderer->isSVGResourceFilterPrimitive() && diff == StyleDifferenceRepaint)
         return;
 
-    clientUpdatedFromElement(renderer, newStyle);
+    // Dynamic changes of CSS properties like 'clip-path' may require us to recompute the associated resources for a renderer.
+    // FIXME: Avoid passing in a useless StyleDifference, but instead compare oldStyle/newStyle to see which resources changed
+    // to be able to selectively rebuild individual resources, instead of all of them.
+    SVGResourcesCache* cache = resourcesCacheFromRenderObject(renderer);
+    cache->removeResourcesFromRenderObject(renderer);
+    cache->addResourcesFromRenderObject(renderer, newStyle);
+
+    RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer, false);
 }
 
-void SVGResourcesCache::clientUpdatedFromElement(RenderObject* renderer, const RenderStyle* newStyle)
+static inline bool rendererCanHaveResources(RenderObject* renderer)
 {
     ASSERT(renderer);
     ASSERT(renderer->parent());
+    return renderer->node() && !renderer->isSVGInlineText();
+}
 
+void SVGResourcesCache::clientWasAddedToTree(RenderObject* renderer, const RenderStyle* newStyle)
+{
+    if (!rendererCanHaveResources(renderer))
+        return;
     SVGResourcesCache* cache = resourcesCacheFromRenderObject(renderer);
-    cache->removeResourcesFromRenderObject(renderer);
     cache->addResourcesFromRenderObject(renderer, newStyle);
+}
+
+void SVGResourcesCache::clientWillBeRemovedFromTree(RenderObject* renderer)
+{
+    if (!rendererCanHaveResources(renderer))
+        return;
+    SVGResourcesCache* cache = resourcesCacheFromRenderObject(renderer);
+    cache->removeResourcesFromRenderObject(renderer);
 
     RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer, false);
 }
index 72e526a..633fcd7 100644 (file)
@@ -37,10 +37,14 @@ public:
     SVGResourcesCache();
     ~SVGResourcesCache();
 
-    void addResourcesFromRenderObject(RenderObject*, const RenderStyle*);
-    void removeResourcesFromRenderObject(RenderObject*);
     static SVGResources* cachedResourcesForRenderObject(const RenderObject*);
 
+    // Called from all SVG renderers addChild() methods.
+    static void clientWasAddedToTree(RenderObject*, const RenderStyle* newStyle);
+
+    // Called from all SVG renderers removeChild() methods.
+    static void clientWillBeRemovedFromTree(RenderObject*);
+
     // Called from all SVG renderers destroy() methods - except for RenderSVGResourceContainer.
     static void clientDestroyed(RenderObject*);
 
@@ -50,13 +54,13 @@ public:
     // Called from all SVG renderers styleDidChange() methods.
     static void clientStyleChanged(RenderObject*, StyleDifference, const RenderStyle* newStyle);
 
-    // Called from all SVG renderers updateFromElement() methods.
-    static void clientUpdatedFromElement(RenderObject*, const RenderStyle* newStyle);
-
     // Called from RenderSVGResourceContainer::willBeDestroyed().
     static void resourceDestroyed(RenderSVGResourceContainer*);
 
 private:
+    void addResourcesFromRenderObject(RenderObject*, const RenderStyle*);
+    void removeResourcesFromRenderObject(RenderObject*);
+
     HashMap<const RenderObject*, SVGResources*> m_cache;
 };
 
index f32e66a..9e0472b 100644 (file)
@@ -350,14 +350,6 @@ void SVGStyledElement::svgAttributeChanged(const QualifiedName& attrName)
     }
 }
 
-void SVGStyledElement::attach()
-{
-    SVGElement::attach();
-
-    if (RenderObject* object = renderer())
-        object->updateFromElement();
-}
-
 Node::InsertionNotificationRequest SVGStyledElement::insertedInto(ContainerNode* rootParent)
 {
     SVGElement::insertedInto(rootParent);
index be9bf24..037d22d 100644 (file)
@@ -71,7 +71,6 @@ protected:
     virtual void collectStyleForAttribute(const Attribute&, StylePropertySet*) OVERRIDE;
     virtual void svgAttributeChanged(const QualifiedName&);
 
-    virtual void attach();
     virtual InsertionNotificationRequest insertedInto(ContainerNode*) OVERRIDE;
     virtual void removedFrom(ContainerNode*) OVERRIDE;
     virtual void childrenChanged(bool changedByParser = false, Node* beforeChange = 0, Node* afterChange = 0, int childCountDelta = 0);