[webkit-reviews] review granted: [Bug 25431] Simplify how SVG containers paint (and likely fix bugs by doing so) : [Attachment 29827] Simplify how SVG containers paint

[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 25431: Simplify how SVG containers paint (and likely fix bugs by doing so)
https://bugs.webkit.org/show_bug.cgi?id=25431

Attachment 29827: Simplify how SVG containers paint
https://bugs.webkit.org/attachment.cgi?id=29827&action=review

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