Simplify how SVG containers paint (and likely fix bugs by doing so) See attached patch (with long ChangeLog entry).
Created attachment 29827 [details] Simplify how SVG containers paint 21 files changed, 302 insertions(+), 203 deletions(-)
About "FIXME: Should we still render filters?" in RenderSVGRoot. I think so, yes. I understand it that way, that you want to stop rendering process, if the root-element is empty. E.g. a empty <g>, right? The spec want to continue on empty elements for filters.
Yeah, there are likely lots of filter bugs in our tree right now that we can't test because we have filters off. Filters still kinda need to be re-written some day anyway. :)
Comment on attachment 29827 [details] Simplify how SVG containers paint > bool RenderSVGContainer::selfWillPaint() const > { > #if ENABLE(SVG_FILTERS) > @@ -121,29 +110,29 @@ void RenderSVGContainer::paint(PaintInfo& paintInfo, int, int) > // Spec: groups w/o children still may render filter content. > if (!firstChild() && !selfWillPaint()) > return; > - > - paintInfo.context->save(); > - applyContentTransforms(paintInfo); > > - SVGResourceFilter* filter = 0; > - PaintInfo savedInfo(paintInfo); > + PaintInfo childPaintInfo(paintInfo); > > - FloatRect boundingBox = repaintRectInLocalCoordinates(); > - if (paintInfo.phase == PaintPhaseForeground) > - prepareToRenderSVGContent(this, paintInfo, boundingBox, filter); > + childPaintInfo.context->save(); > + > + // Let the RenderSVGViewportContainer subclass clip if necessary > + applyViewportClip(childPaintInfo); > > - applyAdditionalTransforms(paintInfo); > + applyTransformToPaintInfo(childPaintInfo, localToParentTransform()); > + > + SVGResourceFilter* filter = 0; > + FloatRect boundingBox = repaintRectInLocalCoordinates(); > + if (childPaintInfo.phase == PaintPhaseForeground) > + prepareToRenderSVGContent(this, childPaintInfo, boundingBox, filter); > > - // default implementation. Just pass paint through to the children > - PaintInfo childInfo(paintInfo); > - childInfo.paintingRoot = paintingRootForChildren(paintInfo); > + childPaintInfo.paintingRoot = paintingRootForChildren(childPaintInfo); > for (RenderObject* child = firstChild(); child; child = child->nextSibling()) > - child->paint(childInfo, 0, 0); > + child->paint(childPaintInfo, 0, 0); > > if (paintInfo.phase == PaintPhaseForeground) > - finishRenderSVGContent(this, paintInfo, boundingBox, filter, savedInfo.context); > + finishRenderSVGContent(this, childPaintInfo, boundingBox, filter, paintInfo.context); Can we start to optimize painting by skipping children that are entirely clipped out? > +void RenderSVGImage::computeRectForRepaint(RenderBoxModelObject* repaintContainer, IntRect& repaintRect, bool fixed) > { > - // FIXME: handle non-root repaintContainer > - return m_absoluteBounds; > + // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent > + repaintRect = localToParentTransform().mapRect(repaintRect); > + parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed); > } It's a shame we can't just fall back on RenderBox::computeRectForRepaint(). If we fixed that to be more HTML-agnostic, could we? > void RenderSVGRoot::calcViewport() > @@ -204,35 +182,59 @@ void RenderSVGRoot::calcViewport() > if (!selfNeedsLayout() && !svg->hasRelativeValues()) > return; > > - SVGLength width = svg->width(); > - SVGLength height = svg->height(); > if (!svg->hasSetContainerSize()) { > - m_viewport = FloatRect(0, 0, width.value(svg), height.value(svg)); > - return; > + // In the normal case of <svg> being stand-alone or in a CSSBoxModel object we use > + // RenderBox::width()/height() (which pulls data from RenderStyle) > + m_viewportSize = FloatSize(width(), height()); > + } else { > + // In the SVGImage case grab the SVGLength values off of SVGSVGElement and use > + // the special relativeWidthValue accessors which respect the specified containerSize > + SVGLength width = svg->width(); > + SVGLength height = svg->height(); > + float viewportWidth = (width.unitType() == LengthTypePercentage) ? svg->relativeWidthValue() : width.value(svg); > + float viewportHeight = (height.unitType() == LengthTypePercentage) ? svg->relativeHeightValue() : height.value(svg); > + m_viewportSize = FloatSize(viewportWidth, viewportHeight); Does this logic deserve to be wrapped in its own method? > TransformationMatrix RenderSVGRoot::localToParentTransform() const > { > - TransformationMatrix offsetTranslation; > - offsetTranslation.translate(x(), y()); > - return localToParentTransformWithoutCSSParentOffset() * offsetTranslation; > + IntSize parentToBorderBoxOffset = parentOriginToBorderBox(); > + > + TransformationMatrix borderBoxOriginToParentOrigin; > + borderBoxOriginToParentOrigin.translate(parentToBorderBoxOffset.width(), parentToBorderBoxOffset.height()); > + > + return localToBorderBoxTransform() * borderBoxOriginToParentOrigin; > } Some tests with <svg> inside transformed HTML would be good to see if this all works. > +void RenderSVGText::computeRectForRepaint(RenderBoxModelObject* repaintContainer, IntRect& repaintRect, bool fixed) > +{ > + // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent > + repaintRect = localToParentTransform().mapRect(repaintRect); > + parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed); > } Again, it's a shame we can't just drop into RenderBox::computeRectForRepaint() > + bool calculateLocalTransform(); What does the return value mean? r=me
(In reply to comment #4) > (From update of attachment 29827 [details] [review]) > Can we start to optimize painting by skipping children that are entirely > clipped out? Would we do that by clipping the damage rect and then checking against the clipped damage rect? > It's a shame we can't just fall back on RenderBox::computeRectForRepaint(). If > we fixed that to be more HTML-agnostic, could we? We could, I think that's fodder for a separate bug though. And actually, I'm not sure it will matter as the goal is to move all SVG renderers onto RenderSVGModelObject, and then they'll all share one computeRectForRepaint anyway. > > void RenderSVGRoot::calcViewport() > Does this logic deserve to be wrapped in its own method? Which logic? calcViewport is the wrapper. I do wonder if the SVGRoot for the SVGImage case shouldn't be a different renderer all together... > Some tests with <svg> inside transformed HTML would be good to see if this all > works. I don't think there are any of those yet. I've never dealt with transformed HTML, but yes, I expect much of this works. > > + bool calculateLocalTransform(); > > What does the return value mean? No clue. That's all legacy code. I'll remove the return value.
(In reply to comment #5) > > > + bool calculateLocalTransform(); > > > > What does the return value mean? > > No clue. That's all legacy code. I'll remove the return value. Actually, I still need to remove localTransform() from RenderObject. I'll fix this all in a later patch.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M LayoutTests/platform/mac/svg/custom/circle-move-invalidation-expected.txt M LayoutTests/platform/mac/svg/custom/svg-float-border-padding-expected.txt M LayoutTests/platform/mac/svg/custom/viewport-update2-expected.txt M WebCore/ChangeLog M WebCore/rendering/RenderPath.cpp M WebCore/rendering/RenderSVGContainer.cpp M WebCore/rendering/RenderSVGContainer.h M WebCore/rendering/RenderSVGImage.cpp M WebCore/rendering/RenderSVGImage.h M WebCore/rendering/RenderSVGModelObject.cpp M WebCore/rendering/RenderSVGModelObject.h M WebCore/rendering/RenderSVGRoot.cpp M WebCore/rendering/RenderSVGRoot.h M WebCore/rendering/RenderSVGText.cpp M WebCore/rendering/RenderSVGText.h M WebCore/rendering/RenderSVGViewportContainer.cpp M WebCore/rendering/RenderSVGViewportContainer.h M WebCore/rendering/SVGRenderSupport.cpp M WebCore/rendering/SVGRenderSupport.h M WebCore/rendering/SVGRootInlineBox.cpp Committed r42950
*** Bug 19366 has been marked as a duplicate of this bug. ***
*** Bug 24633 has been marked as a duplicate of this bug. ***