Bug 47254 - Only execute first SVG text layout phase if needed
Summary: Only execute first SVG text layout phase if needed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-06 03:45 PDT by Nikolas Zimmermann
Modified: 2010-10-06 05:30 PDT (History)
2 users (show)

See Also:


Attachments
Patch (19.55 KB, patch)
2010-10-06 03:52 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-10-06 03:45:58 PDT
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.
Comment 1 Nikolas Zimmermann 2010-10-06 03:52:11 PDT
Created attachment 69919 [details]
Patch
Comment 2 Dirk Schulze 2010-10-06 05:23:47 PDT
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.
Comment 3 Nikolas Zimmermann 2010-10-06 05:26:47 PDT
(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*.
Comment 4 Nikolas Zimmermann 2010-10-06 05:30:14 PDT
Thanks for the review. Landed in r69186.