Bug 47254 – Only execute first SVG text layout phase if needed

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Only if x/y/dx/dy/rotate values change or the style of a RenderSVGInlineText rendere, or the text content itself, the first layout phase needs to be executed from RenderSVGText::layout().
It's currently done on every layout, wasting time.

[Nikolas Zimmermann]

Created attachment 69919 [details]
Patch

[Dirk Schulze]

Comment on attachment 69919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69919&action=review

Please look if the Renderer in ResourceGradient can't be const. Otherwise looks good. r=me

> WebCore/ChangeLog:24
> +        (WebCore::RenderSVGInline::objectBoundingBox): adapt to renames, use RenderSVGText::locateRenderSVGTextAncestor().

s/adapt/Adapt/

> WebCore/rendering/RenderSVGResourceGradient.cpp:81
> -                                                           const RenderObject* object)
> +                                                           RenderObject* object)
>  {
> -    const RenderObject* textRootBlock = SVGRenderSupport::findTextRootObject(object);
> +    RenderObject* textRootBlock = RenderSVGText::locateRenderSVGTextAncestor(object);

Why not const RenderObject*?

> WebCore/rendering/RenderSVGResourceGradient.cpp:115
> -                                             const RenderObject* object,
> +                                             RenderObject* object,
>                                               GradientData* gradientData)
>  {
> -    const RenderObject* textRootBlock = SVGRenderSupport::findTextRootObject(object);
> +    RenderObject* textRootBlock = RenderSVGText::locateRenderSVGTextAncestor(object);

Ditto.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 69919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69919&action=review
> 
> Please look if the Renderer in ResourceGradient can't be const. Otherwise looks good. r=me
I made it non-const on purpose. The parameter that's passed to this function is a "RenderObject*", prior to this patch, we converted it to a "const RenderObject*", I basically fixed this round trip.

> 
> > WebCore/ChangeLog:24
> > +        (WebCore::RenderSVGInline::objectBoundingBox): adapt to renames, use RenderSVGText::locateRenderSVGTextAncestor().
> 
> s/adapt/Adapt/
Fixed.

> 
> > WebCore/rendering/RenderSVGResourceGradient.cpp:81
> > -                                                           const RenderObject* object)
> > +                                                           RenderObject* object)
> >  {
> > -    const RenderObject* textRootBlock = SVGRenderSupport::findTextRootObject(object);
> > +    RenderObject* textRootBlock = RenderSVGText::locateRenderSVGTextAncestor(object);
> 
> Why not const RenderObject*?
> 
> > WebCore/rendering/RenderSVGResourceGradient.cpp:115
> > -                                             const RenderObject* object,
> > +                                             RenderObject* object,
> >                                               GradientData* gradientData)
> >  {
> > -    const RenderObject* textRootBlock = SVGRenderSupport::findTextRootObject(object);
> > +    RenderObject* textRootBlock = RenderSVGText::locateRenderSVGTextAncestor(object);
> 
> Ditto.

As I said before, the param passed to both of this functions is actually a RenderObject*.

[Nikolas Zimmermann]

Thanks for the review. Landed in r69186.