[webkit-reviews] review granted: [Bug 25532] Clean up SVG rendering tree more : [Attachment 29980] Move more code into SVGRenderBase

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Eric Seidel
<eric at webkit.org>'s request for review:
Bug 25532: Clean up SVG rendering tree more
https://bugs.webkit.org/show_bug.cgi?id=25532

Attachment 29980: Move more code into SVGRenderBase
https://bugs.webkit.org/attachment.cgi?id=29980&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> +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?