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.
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.
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
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:01 PDT, Eric Seidel (no email)
2009-05-03 21:02 PDT, Eric Seidel (no email)
2009-05-03 21:02 PDT, Eric Seidel (no email)