Clean up SVG rendering tree more I wrote 16 patches on my flight from LAX->SYD cleaning up the SVG rendering tree more. They're all small (some of them tiny) and don't really need their own bugs, so I'm going to attach the ones which don't already have other bugs here.
Created attachment 29972 [details] Fix RenderForeignObject::paint() .../text/foreignObject-repaint-expected.checksum | 1 + .../svg/text/foreignObject-repaint-expected.png | Bin 0 -> 32329 bytes .../svg/text/foreignObject-repaint-expected.txt | 10 +++++++ LayoutTests/svg/text/foreignObject-repaint.xml | 15 +++++++++++ WebCore/ChangeLog | 11 ++++++++ WebCore/rendering/RenderForeignObject.cpp | 26 ++++++++----------- 6 files changed, 48 insertions(+), 15 deletions(-)
Created attachment 29973 [details] Remove redundant disableLayoutState() calls WebCore/ChangeLog | 14 ++++++++++++++ WebCore/rendering/RenderForeignObject.cpp | 9 ++------- WebCore/rendering/RenderSVGContainer.cpp | 7 +------ WebCore/rendering/RenderSVGViewportContainer.cpp | 12 ++++-------- 4 files changed, 21 insertions(+), 21 deletions(-)
Created attachment 29974 [details] Share layout code between RenderSVGViewportContainer and RenderSVGContainer WebCore/ChangeLog | 15 ++++++++++++++ WebCore/rendering/RenderSVGContainer.cpp | 4 ++- WebCore/rendering/RenderSVGContainer.h | 3 +- WebCore/rendering/RenderSVGViewportContainer.cpp | 23 ---------------------- WebCore/rendering/RenderSVGViewportContainer.h | 3 +- 5 files changed, 21 insertions(+), 27 deletions(-)
Created attachment 29975 [details] Remove dead code from RenderPath WebCore/ChangeLog | 10 ++++++++++ WebCore/rendering/RenderPath.cpp | 10 ---------- WebCore/rendering/RenderPath.h | 2 -- 3 files changed, 10 insertions(+), 12 deletions(-)
Created attachment 29976 [details] Remove broken absoluteTransform() code from RenderSVGInlineText .../mac/svg/custom/focus-ring-expected.checksum | 2 +- .../mac/svg/custom/focus-ring-expected.png | Bin 40658 -> 43386 bytes .../svg/custom/svgsvgelement-ctm-expected.txt | 8 ++-- WebCore/ChangeLog | 33 ++++++++++++++++++++ WebCore/rendering/RenderSVGInlineText.cpp | 30 ++++++++---------- WebCore/rendering/RenderSVGInlineText.h | 1 + WebCore/rendering/RenderSVGRoot.cpp | 14 ++++++++ WebCore/rendering/RenderSVGRoot.h | 2 + WebCore/rendering/RenderSVGText.cpp | 14 ++++++++ WebCore/rendering/RenderSVGText.h | 2 + WebCore/svg/SVGSVGElement.cpp | 4 +- 11 files changed, 86 insertions(+), 24 deletions(-)
Created attachment 29977 [details] Move RenderSVGText off of localToAbsolute() WebCore/ChangeLog | 11 +++++++++++ WebCore/rendering/RenderSVGText.cpp | 20 ++++++-------------- 2 files changed, 17 insertions(+), 14 deletions(-)
Created attachment 29978 [details] Move absoluteRects and absoluteQuads into RenderSVGInline and remove absoluteTransform() usage WebCore/ChangeLog | 16 +++++++++++ WebCore/rendering/RenderSVGInline.cpp | 43 ++++++++++++++++++++++++++++- WebCore/rendering/RenderSVGInline.h | 4 +++ WebCore/rendering/RenderSVGTSpan.cpp | 45 ------------------------------- WebCore/rendering/RenderSVGTSpan.h | 2 - WebCore/rendering/RenderSVGTextPath.cpp | 40 --------------------------- WebCore/rendering/RenderSVGTextPath.h | 2 - 7 files changed, 62 insertions(+), 90 deletions(-)
Created attachment 29979 [details] Add SVGRenderBase to share logic between SVG renderers WebCore/ChangeLog | 39 ++++++++++++++++++ WebCore/rendering/RenderSVGBlock.h | 3 +- WebCore/rendering/RenderSVGImage.h | 3 +- WebCore/rendering/RenderSVGModelObject.h | 7 ++- WebCore/rendering/RenderSVGRoot.h | 3 +- WebCore/rendering/SVGRenderSupport.cpp | 20 ++++----- WebCore/rendering/SVGRenderSupport.h | 50 ++++++++++++++-------- WebCore/rendering/SVGRootInlineBox.cpp | 9 ++-- WebCore/svg/SVGMaskElement.cpp | 2 +- WebCore/svg/SVGPatternElement.cpp | 4 +- WebCore/svg/graphics/SVGPaintServerGradient.cpp | 5 +- 11 files changed, 100 insertions(+), 45 deletions(-)
Created attachment 29980 [details] Move more code into SVGRenderBase WebCore/ChangeLog | 35 +++++++++++++++++++++++++++ WebCore/rendering/RenderSVGImage.cpp | 19 +++++--------- WebCore/rendering/RenderSVGImage.h | 4 ++- WebCore/rendering/RenderSVGModelObject.cpp | 27 ++++----------------- WebCore/rendering/RenderSVGText.cpp | 25 ++----------------- WebCore/rendering/SVGRenderSupport.cpp | 36 +++++++++++++++++++++++++++- WebCore/rendering/SVGRenderSupport.h | 5 ++++ 7 files changed, 93 insertions(+), 58 deletions(-)
Created attachment 29981 [details] Remove the vestigial calculateLocalTransform() .../mac/svg/custom/focus-ring-expected.checksum | 2 +- .../mac/svg/custom/focus-ring-expected.png | Bin 43386 -> 50898 bytes WebCore/ChangeLog | 33 ++++++++++++++++++++ WebCore/rendering/RenderForeignObject.cpp | 9 +----- WebCore/rendering/RenderForeignObject.h | 4 +- WebCore/rendering/RenderPath.cpp | 14 ++------ WebCore/rendering/RenderPath.h | 1 - WebCore/rendering/RenderSVGContainer.cpp | 6 --- WebCore/rendering/RenderSVGContainer.h | 3 +- WebCore/rendering/RenderSVGImage.cpp | 20 +++++------- WebCore/rendering/RenderSVGImage.h | 1 - WebCore/rendering/RenderSVGText.cpp | 13 ++------ WebCore/rendering/RenderSVGText.h | 2 - .../rendering/RenderSVGTransformableContainer.cpp | 4 +-- .../rendering/RenderSVGTransformableContainer.h | 4 +- 15 files changed, 56 insertions(+), 60 deletions(-)
Created attachment 29982 [details] Remove m_absoluteBounds hack from RenderSVGText WebCore/ChangeLog | 13 +++++++++++++ WebCore/rendering/RenderSVGText.cpp | 6 +----- WebCore/rendering/RenderSVGText.h | 1 - 3 files changed, 14 insertions(+), 6 deletions(-)
Created attachment 29983 [details] ASSERT(useTransforms) in SVG mapLocalToContainer implementations WebCore/ChangeLog | 12 ++++++++++++ WebCore/rendering/RenderSVGRoot.cpp | 9 +++------ WebCore/rendering/SVGRenderSupport.cpp | 9 ++------- 3 files changed, 17 insertions(+), 13 deletions(-)
Oops. I need to fix my get-send-bugzilla script to be shorter. :( Also, I know this is a lot of patches for one bug. I expect that pretty much all of these will be simple r+'s. If any of them need discussion I'm happy to break them out into other bugs.
Comment on attachment 29979 [details] Add SVGRenderBase to share logic between SVG renderers > // Most renderers in the SVG rendering tree will inherit from this class > // but not all. (e.g. RenderSVGForeignObject, RenderSVGBlock, RenderSVGImage) thus methods > -// required by SVG renders need to be declared on RenderObject, but some shared > -// logic can go in this class. > +// required by SVG renders need to be declared on RenderObject, but shared > +// logic can go in this class or in SVGRenderBase. You should give some direction about why one or the other is appropriate.
Comment on attachment 29980 [details] Move more code into SVGRenderBase > +IntRect SVGRenderBase::clippedOverflowRectForRepaint(RenderObject* object, RenderBoxModelObject* repaintContainer) > +{ Maybe assert that repaintContainer != this to make sure that someone isn't trying to do a container-relative repaint inside of SVG content? > + // Return early for any cases where we don't actually paint > + if (object->style()->visibility() != VISIBLE && !object->enclosingLayer()->hasVisibleContent()) > + return IntRect(); > + > + // Pass our local paint rect to computeRectForRepaint() which will > + // map to parent coords and recurse up the parent chain. > + IntRect repaintRect = enclosingIntRect(object->repaintRectInLocalCoordinates()); > + object->computeRectForRepaint(repaintContainer, repaintRect); > + return repaintRect; > +void SVGRenderBase::computeRectForRepaint(RenderObject* object, RenderBoxModelObject* repaintContainer, IntRect& repaintRect, bool fixed) > +{ Assert that this != repaintContainer ? > + // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent > + repaintRect = object->localToParentTransform().mapRect(repaintRect); > + object->parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed); Is object->parent() always non-null? > +void SVGRenderBase::mapLocalToContainer(const RenderObject* object, RenderBoxModelObject* repaintContainer, bool fixed , bool useTransforms, TransformState& transformState) > +{ > + ASSERT(!fixed); // We should have no fixed content in the SVG rendering tree. > + > + // FIXME: If we don't respect useTransforms we break SVG text rendering. > + // Seems RenderSVGInlineText has some own broken translation hacks which depend useTransforms=false > + // This should instead be ASSERT(useTransforms) once we fix RenderSVGInlineText > + if (useTransforms) > + transformState.applyTransform(object->localToParentTransform()); > + > + object->parent()->mapLocalToContainer(repaintContainer, fixed, useTransforms, transformState); Is object->parent() always non-null?
Comment on attachment 29982 [details] Remove m_absoluteBounds hack from RenderSVGText > + No functional changes (SVGs inside CSS transformed HTML should theoretically repaint better) Can we have a testcase pls?
(In reply to comment #16) > (From update of attachment 29982 [details] [review]) > > + No functional changes (SVGs inside CSS transformed HTML should theoretically repaint better) > > Can we have a testcase pls? It's possible that the test cases from bug 23112 or bug 23113 may already be fixed by these changes.
(In reply to comment #15) > (From update of attachment 29980 [details] [review]) object()->parent() is alwasy non-null, yes. Unless you're the root of the render tree... which is only RenderCanvas iirc. And few SVG renderers are RenderBoxModelObject's so checking this != repaintContainer seems overkill to me.
Yay, these are all landed now.
A test case for some of this is up for review at: https://bugs.webkit.org/show_bug.cgi?id=23112
Comment on attachment 29983 [details] ASSERT(useTransforms) in SVG mapLocalToContainer implementations Had to roll this one out, I must not have run the tests after this change... I thought I did. Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/rendering/RenderSVGRoot.cpp M WebCore/rendering/SVGRenderSupport.cpp Committed r43218
Re-opening since I rolled out the last change.
I'll land this last patch as part of: https://bugs.webkit.org/show_bug.cgi?id=25568 We can go ahead and close this bug again. :)