Attachment 60547 Details for Bug 15391 – Updated patch

[patch] Updated patch

NextPatch.diff (text/plain), 17.12 KB, created by Nikolas Zimmermann on 2010-07-05 09:07:16 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Nikolas Zimmermann

Created: 2010-07-05 09:07:16 PDT

Size: 17.12 KB

patch

obsolete

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 62490)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,41 @@
>+2010-07-05  Nikolas Zimmermann  <nzimmermann@rim.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        SVGRenderContainer forces too many kids to relayout
>+        https://bugs.webkit.org/show_bug.cgi?id=15391
>+
>+        Fixing a long-standing performance issue. We should only ever need to relayout container children when the parent bounds change.
>+        The bounds of a container can only change, if the outermost RenderSVGRoot container uses relative length values and its size changes.
>+        This can either happen when the window resizes for standalone SVG documents, or if an enclosing RenderBox changes width/height values.
>+
>+        Only relayout container children if the container has selfNeedsLayout() true, or if the parent bounds change.
>+        Lively Kernel doesn't do any relayouts anymore, except if you change the Safari window size, this is a great progression.
>+
>+        * rendering/RenderPath.cpp:
>+        (WebCore::RenderPath::layout): No need to special case Path updates, if the element uses relative lengths. SVGRenderSupport now handles this case.
>+        * rendering/RenderSVGContainer.cpp:
>+        (WebCore::RenderSVGContainer::layout): Fix some style issues.
>+        * rendering/RenderSVGContainer.h:
>+        (WebCore::RenderSVGContainer::setDrawsContents): Inlined for speed.
>+        (WebCore::RenderSVGContainer::drawsContents): Ditto.
>+        * rendering/RenderSVGRoot.cpp:
>+        (WebCore::RenderSVGRoot::RenderSVGRoot): Initialize m_isLayoutSizeChanged to false.
>+        (WebCore::RenderSVGRoot::layout): Set m_isLayoutSizeChanged=true when the RenderSVGRoot size changes during layout.
>+        (WebCore::RenderSVGRoot::calcViewport): Remove hasRelativeLengths() special case.
>+        * rendering/RenderSVGRoot.h:
>+        (WebCore::RenderSVGRoot::isLayoutSizeChanged): New function, which returns true during layout() if the outermost <svg> size changes.
>+        * rendering/RenderSVGViewportContainer.cpp:
>+        (WebCore::RenderSVGViewportContainer::calcViewport): Cleanup code, and remove obsolete hasRelativeLengths() special case.
>+        * rendering/SVGRenderSupport.cpp:
>+        (WebCore::svgRootTreeObject): Added helper function, that returns the RenderSVGRoot for a given RenderObject.
>+        (WebCore::SVGRenderSupport::layoutChildren): Remove FIXME, only relayout container children, if the parent bounds change and the child uses relative lengths.
>+        * svg/SVGStyledElement.cpp:
>+        (WebCore::SVGStyledElement::updateRelativeLengthsInformation): Implemented this function. Keeps track of relative lengths elements, so that the
>+                                                                       hasRelativeLengths() information is always up2date.
>+        * svg/SVGStyledElement.h:
>+        (WebCore::SVGStyledElement::hasRelativeLengths): Don't call the virtual selfHasRelativeLengths() information, just return wheter m_elementsWithRelativeLengths is not empty.
>+
> 2010-07-05  Pavel Feldman  <pfeldman@chromium.org>
> 
>         Reviewed by Yury Semikhatsky.
>Index: WebCore/rendering/RenderPath.cpp
>===================================================================
>--- WebCore/rendering/RenderPath.cpp	(revision 62488)
>+++ WebCore/rendering/RenderPath.cpp	(working copy)
>@@ -100,13 +100,7 @@ void RenderPath::layout()
>     LayoutRepainter repainter(*this, checkForRepaintDuringLayout() && selfNeedsLayout());
>     SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
> 
>-    // We need to update the Path object whenever the underlying SVGStyledTransformableElement uses relative values
>-    // as the viewport size may have changed. It would be nice to optimize this to detect these changes, and only
>-    // update when needed, even when using relative values.
>     bool needsPathUpdate = m_needsPathUpdate;
>-    if (!needsPathUpdate && element->hasRelativeLengths())
>-        needsPathUpdate = true;
>-
>     if (needsPathUpdate) {
>         m_path = element->toPathData();
>         m_needsPathUpdate = false;
>Index: WebCore/rendering/RenderSVGContainer.cpp
>===================================================================
>--- WebCore/rendering/RenderSVGContainer.cpp	(revision 62484)
>+++ WebCore/rendering/RenderSVGContainer.cpp	(working copy)
>@@ -40,29 +40,24 @@ RenderSVGContainer::RenderSVGContainer(S
> {
> }
> 
>-bool RenderSVGContainer::drawsContents() const
>-{
>-    return m_drawsContents;
>-}
>-
>-void RenderSVGContainer::setDrawsContents(bool drawsContents)
>-{
>-    m_drawsContents = drawsContents;
>-}
>-
> void RenderSVGContainer::layout()
> {
>     ASSERT(needsLayout());
>-    ASSERT(!view()->layoutStateEnabled()); // RenderSVGRoot disables layoutState for the SVG rendering tree.
> 
>-    calcViewport(); // Allow RenderSVGViewportContainer to update its viewport
>+    // RenderSVGRoot disables layoutState for the SVG rendering tree.
>+    ASSERT(!view()->layoutStateEnabled());
>+
>+    // Allow RenderSVGViewportContainer to update its viewport.
>+    calcViewport();
> 
>     LayoutRepainter repainter(*this, checkForRepaintDuringLayout() || selfWillPaint());
>-    calculateLocalTransform(); // Allow RenderSVGTransformableContainer to update its transform
>+
>+    // Allow RenderSVGTransformableContainer to update its transform.
>+    calculateLocalTransform();
> 
>     SVGRenderSupport::layoutChildren(this, selfNeedsLayout());
>-    repainter.repaintAfterLayout();
> 
>+    repainter.repaintAfterLayout();
>     setNeedsLayout(false);
> }
> 
>Index: WebCore/rendering/RenderSVGContainer.h
>===================================================================
>--- WebCore/rendering/RenderSVGContainer.h	(revision 62484)
>+++ WebCore/rendering/RenderSVGContainer.h	(working copy)
>@@ -39,8 +39,8 @@ public:
>     RenderObjectChildList* children() { return &m_children; }
> 
>     // <marker> uses these methods to only allow drawing children during a special marker draw time
>-    void setDrawsContents(bool);
>-    bool drawsContents() const;
>+    void setDrawsContents(bool drawsContents) { m_drawsContents = drawsContents; }
>+    bool drawsContents() const { return m_drawsContents; }
> 
>     virtual void paint(PaintInfo&, int parentX, int parentY);
> 
>@@ -97,5 +97,3 @@ void toRenderSVGContainer(const RenderSV
> 
> #endif // ENABLE(SVG)
> #endif // RenderSVGContainer_h
>-
>-// vim:ts=4:noet
>Index: WebCore/rendering/RenderSVGRoot.cpp
>===================================================================
>--- WebCore/rendering/RenderSVGRoot.cpp	(revision 62488)
>+++ WebCore/rendering/RenderSVGRoot.cpp	(working copy)
>@@ -45,6 +45,7 @@ namespace WebCore {
> 
> RenderSVGRoot::RenderSVGRoot(SVGStyledElement* node)
>     : RenderBox(node)
>+    , m_isLayoutSizeChanged(false)
> {
>     setReplaced(true);
> }
>@@ -113,13 +114,14 @@ void RenderSVGRoot::layout()
>     IntSize oldSize(width(), height());
>     calcWidth();
>     calcHeight();
>-
>     calcViewport();
> 
>-    // RenderSVGRoot needs to take special care to propagate window size changes to the children,
>-    // if the outermost <svg> is using relative x/y/width/height values. Hence the additonal parameters.
>     SVGSVGElement* svg = static_cast<SVGSVGElement*>(node());
>-    SVGRenderSupport::layoutChildren(this, needsLayout || (svg->hasRelativeLengths() && oldSize != size()));
>+    m_isLayoutSizeChanged = svg->hasRelativeLengths() && oldSize != size();
>+
>+    SVGRenderSupport::layoutChildren(this, needsLayout);
>+    m_isLayoutSizeChanged = false;
>+
>     repainter.repaintAfterLayout();
> 
>     view()->enableLayoutState();
>@@ -197,9 +199,6 @@ void RenderSVGRoot::calcViewport()
> {
>     SVGSVGElement* svg = static_cast<SVGSVGElement*>(node());
> 
>-    if (!selfNeedsLayout() && !svg->hasRelativeLengths())
>-        return;
>-
>     if (!svg->hasSetContainerSize()) {
>         // In the normal case of <svg> being stand-alone or in a CSSBoxModel object we use
>         // RenderBox::width()/height() (which pulls data from RenderStyle)
>Index: WebCore/rendering/RenderSVGRoot.h
>===================================================================
>--- WebCore/rendering/RenderSVGRoot.h	(revision 62484)
>+++ WebCore/rendering/RenderSVGRoot.h	(working copy)
>@@ -40,6 +40,8 @@ public:
>     const RenderObjectChildList* children() const { return &m_children; }
>     RenderObjectChildList* children() { return &m_children; }
> 
>+    bool isLayoutSizeChanged() const { return m_isLayoutSizeChanged; }
>+
> private:
>     virtual RenderObjectChildList* virtualChildren() { return children(); }
>     virtual const RenderObjectChildList* virtualChildren() const { return children(); }
>@@ -84,6 +86,7 @@ private:
>     RenderObjectChildList m_children;
>     FloatSize m_viewportSize;
>     mutable AffineTransform m_localToParentTransform;
>+    bool m_isLayoutSizeChanged : 1;
> };
> 
> inline RenderSVGRoot* toRenderSVGRoot(RenderObject* object)
>Index: WebCore/rendering/RenderSVGViewportContainer.cpp
>===================================================================
>--- WebCore/rendering/RenderSVGViewportContainer.cpp	(revision 62488)
>+++ WebCore/rendering/RenderSVGViewportContainer.cpp	(working copy)
>@@ -45,18 +45,13 @@ void RenderSVGViewportContainer::applyVi
> 
> void RenderSVGViewportContainer::calcViewport()
> {
>-    SVGElement* svgelem = static_cast<SVGElement*>(node());
>-    if (svgelem->hasTagName(SVGNames::svgTag)) {
>-        SVGSVGElement* svg = static_cast<SVGSVGElement*>(node());
>-
>-        if (!selfNeedsLayout() && !svg->hasRelativeLengths())
>-            return;
>-
>-        float x = svg->x().value(svg);
>-        float y = svg->y().value(svg);
>-        float w = svg->width().value(svg);
>-        float h = svg->height().value(svg);
>-        m_viewport = FloatRect(x, y, w, h);
>+     SVGElement* element = static_cast<SVGElement*>(node());
>+     if (element->hasTagName(SVGNames::svgTag)) {
>+         SVGSVGElement* svg = static_cast<SVGSVGElement*>(element);
>+         m_viewport = FloatRect(svg->x().value(svg)
>+                                , svg->y().value(svg)
>+                                , svg->width().value(svg)
>+                                , svg->height().value(svg));
>     }
> }
> 
>Index: WebCore/rendering/SVGRenderSupport.cpp
>===================================================================
>--- WebCore/rendering/SVGRenderSupport.cpp	(revision 62488)
>+++ WebCore/rendering/SVGRenderSupport.cpp	(working copy)
>@@ -32,12 +32,14 @@
> #include "ImageBuffer.h"
> #include "NodeRenderStyle.h"
> #include "RenderLayer.h"
>+#include "RenderPath.h"
> #include "RenderSVGContainer.h"
> #include "RenderSVGResource.h"
> #include "RenderSVGResourceClipper.h"
> #include "RenderSVGResourceFilter.h"
> #include "RenderSVGResourceMarker.h"
> #include "RenderSVGResourceMasker.h"
>+#include "RenderSVGRoot.h"
> #include "SVGStyledElement.h"
> #include "TransformState.h"
> #include <wtf/UnusedParam.h>
>@@ -230,25 +232,42 @@ FloatRect SVGRenderSupport::computeConta
>     return boundingBox;
> }
> 
>+static inline RenderSVGRoot* svgRootTreeObject(RenderObject* start)
>+{
>+    while (start && !start->isSVGRoot())
>+        start = start->parent();
>+
>+    ASSERT(start);
>+    ASSERT(start->isSVGRoot());
>+    return toRenderSVGRoot(start);
>+}
>+
> void SVGRenderSupport::layoutChildren(RenderObject* start, bool selfNeedsLayout)
> {
>+    bool layoutSizeChanged = svgRootTreeObject(start)->isLayoutSizeChanged();
>+
>     for (RenderObject* child = start->firstChild(); child; child = child->nextSibling()) {
>-        // Only force our kids to layout if we're being asked to relayout as a result of a parent changing
>-        // FIXME: We should be able to skip relayout of non-relative kids when only bounds size has changed
>-        // that's a possible future optimization using LayoutState
>-        // http://bugs.webkit.org/show_bug.cgi?id=15391
>         bool needsLayout = selfNeedsLayout;
>-        if (!needsLayout) {
>+
>+        if (layoutSizeChanged) {
>+            // When selfNeedsLayout is false and the layout size changed, we have to check whether this child uses relative lengths
>             if (SVGElement* element = child->node()->isSVGElement() ? static_cast<SVGElement*>(child->node()) : 0) {
>-                if (element->isStyled())
>-                    needsLayout = static_cast<SVGStyledElement*>(element)->hasRelativeLengths();
>+                if (element->isStyled() && static_cast<SVGStyledElement*>(element)->hasRelativeLengths()) {
>+                    // When the layout size changed and when using relative values tell the RenderPath to update its Path object
>+                    if (child->isRenderPath())
>+                        toRenderPath(child)->setNeedsPathUpdate();
>+
>+                    needsLayout = true;
>+                }
>             }
>         }
> 
>-        if (needsLayout)
>+        if (needsLayout) {
>             child->setNeedsLayout(true, false);
>+            child->layout();
>+        } else
>+            child->layoutIfNeeded();
> 
>-        child->layoutIfNeeded();
>         ASSERT(!child->needsLayout());
>     }
> }
>Index: WebCore/svg/SVGStyledElement.cpp
>===================================================================
>--- WebCore/svg/SVGStyledElement.cpp	(revision 62488)
>+++ WebCore/svg/SVGStyledElement.cpp	(working copy)
>@@ -366,10 +366,43 @@ AffineTransform SVGStyledElement::localC
>     return AffineTransform();
> }
> 
>-void SVGStyledElement::updateRelativeLengthsInformation(bool, SVGStyledElement*)
>+void SVGStyledElement::updateRelativeLengthsInformation(bool hasRelativeLengths, SVGStyledElement* element)
> {
>-    // FIXME: The actual code will land in a follow-up patch.
>-    // See https://bugs.webkit.org/show_bug.cgi?id=41566
>+    // If we're not yet in a document, this function will be called again from insertedIntoDocument(). Do nothing now.
>+    if (!inDocument())
>+        return;
>+
>+    // An element wants to notify us that its own relative lengths state changed.
>+    // Register it in the relative length map, and register us in the parent relative length map.
>+    // Register the parent in the grandparents map, etc. Repeat procedure until the root of the SVG tree.
>+
>+    if (hasRelativeLengths)
>+        m_elementsWithRelativeLengths.add(element);
>+    else {
>+        if (!m_elementsWithRelativeLengths.contains(element)) {
>+            // We were never registered. Do nothing.
>+            return;
>+        }
>+
>+        m_elementsWithRelativeLengths.remove(element);
>+    }
>+
>+    // Find first styled parent node, and notify it that we've changed our relative length state.
>+    Node* node = parent();
>+    while (node) {
>+        if (!node->isSVGElement())
>+            break;
>+
>+        SVGElement* element = static_cast<SVGElement*>(node);
>+        if (!element->isStyled()) {
>+            node = node->parent();
>+            continue;
>+        }
>+
>+        // Register us in the parent element map.
>+        static_cast<SVGStyledElement*>(element)->updateRelativeLengthsInformation(hasRelativeLengths, this);
>+        break;
>+    }
> }
> 
> }
>Index: WebCore/svg/SVGStyledElement.h
>===================================================================
>--- WebCore/svg/SVGStyledElement.h	(revision 62488)
>+++ WebCore/svg/SVGStyledElement.h	(working copy)
>@@ -40,8 +40,7 @@ namespace WebCore {
> 
>         virtual String title() const;
> 
>-        // FIXME: The actual code will land in a follow-up patch.
>-        bool hasRelativeLengths() const { return selfHasRelativeLengths(); }
>+        bool hasRelativeLengths() const { return !m_elementsWithRelativeLengths.isEmpty(); }
> 
>         virtual bool isStyled() const { return true; }
>         virtual bool supportsMarkers() const { return false; }
>@@ -81,6 +80,7 @@ namespace WebCore {
>         virtual bool selfHasRelativeLengths() const { return false; }
> 
>     private:
>+        HashSet<SVGStyledElement*> m_elementsWithRelativeLengths;
>         DECLARE_ANIMATED_PROPERTY(SVGStyledElement, HTMLNames::classAttr, String, ClassName, className)
>     };
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 62490)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,14 @@
>+2010-07-05  Nikolas Zimmermann  <nzimmermann@rim.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        SVGRenderContainer forces too many kids to relayout
>+        https://bugs.webkit.org/show_bug.cgi?id=15391
>+
>+        Update results, svg/hixie/error/013.xml. Marginal size change, but a progression.
>+
>+        * platform/mac/svg/hixie/error/013-expected.txt:
>+
> 2010-07-05  Nikolas Zimmermann  <nzimmermann@rim.com>
> 
>         Reviewed by Dirk Schulze.
>Index: LayoutTests/platform/mac/svg/hixie/error/013-expected.txt
>===================================================================
>--- LayoutTests/platform/mac/svg/hixie/error/013-expected.txt	(revision 62484)
>+++ LayoutTests/platform/mac/svg/hixie/error/013-expected.txt	(working copy)
>@@ -5,7 +5,7 @@ layer at (0,0) size 800x100
>   RenderBlock {html} at (0,0) size 800x100
>     RenderBody {body} at (8,8) size 784x76
>       RenderBlock (anonymous) at (0,0) size 784x24
>-        RenderSVGRoot {svg} at (8,8) size 769x20
>+        RenderSVGRoot {svg} at (8,8) size 784x20
>           RenderSVGText {text} at (10,20) size 125x13 contains 1 chunk(s)
>             RenderSVGInlineText {#text} at (0,0) size 125x13
>               chunk 1 text run 1 at (10.00,30.00) startOffset 0 endOffset 30 width 125.00: "FAIL (This should not render.)"

Flags:

krit: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 15391: 60535 | 60547