Bug 41566 – Logic to track whether elements are using relative lengths is incomplete

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

To be able to avoid relayouts for absolute positioned RenderObjects if just window size changes we need more logic in the SVG DOM to know whether an element has relative lengths or not.

[Nikolas Zimmermann]

Created attachment 60442 [details]
Initial patch

This prepares the actual patch to fix bug 15391. Gives a huge speed benefit, whenever elements with absolute sizes are used, that do not need to relayout anymore.
The actual logic for that comes in the patch for bug 15391, this is just a preparation.

[WebKit Review Bot]

Attachment 60442 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGStyledElement.cpp:371:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 60444 [details]
Updated patch

Oops, remove leftover tab.

[Dirk Schulze]

Comment on attachment 60444 [details]
Updated patch

>  {
>      SVGStyledTransformableElement::svgAttributeChanged(attrName);
>  
> +    bool isLengthAttribute = attrName == SVGNames::cxAttr
> +                          || attrName == SVGNames::cyAttr
> +                          || attrName == SVGNames::rAttr;
> +
> +    if (isLengthAttribute)
> +        updateRelativeLengthsInformation();
> +
>      RenderPath* renderer = static_cast<RenderPath*>(this->renderer());
>      if (!renderer)

updateRelativeLengthsInformation is not in this patch. Do we need to call it, even if we don't have a renderer? I guess you want to update the information about relative values, even if a renderer was not created yet, to avoid a second layout, correct?

> +void SVGStyledElement::updateRelativeLengthsInformation(bool, SVGStyledElement*)
> +{
> +    // FIXME: The actual code will land in a follow-up patch.
> +}
> +
>  }

Please add a link to this bug, even if you provide a followup soon.

It looks like you changed the behavior of the filterElement. Do we have dynamic update tests for filter? If not, can you add some?

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 60444 [details])
> >  {
> >      SVGStyledTransformableElement::svgAttributeChanged(attrName);
> >  
> > +    bool isLengthAttribute = attrName == SVGNames::cxAttr
> > +                          || attrName == SVGNames::cyAttr
> > +                          || attrName == SVGNames::rAttr;
> > +
> > +    if (isLengthAttribute)
> > +        updateRelativeLengthsInformation();
> > +
> >      RenderPath* renderer = static_cast<RenderPath*>(this->renderer());
> >      if (!renderer)
> 
> updateRelativeLengthsInformation is not in this patch. Do we need to call it, even if we don't have a renderer? I guess you want to update the information about relative values, even if a renderer was not created yet, to avoid a second layout, correct?
Well, it's necessary to build this information while parsing, before renderers are available. As soon as renderers are attached, and layouted we need the relative length information.

> > +void SVGStyledElement::updateRelativeLengthsInformation(bool, SVGStyledElement*)
> > +{
> > +    // FIXME: The actual code will land in a follow-up patch.
> > +}
> > +
> >  }
> 
> Please add a link to this bug, even if you provide a followup soon.
Will do.

> 
> It looks like you changed the behavior of the filterElement. Do we have dynamic update tests for filter? If not, can you add some?

I hope you could mail me your set? You said you've already  created some.

[Dirk Schulze]

(In reply to comment #5)
> I hope you could mail me your set? You said you've already  created some.

I'll search them. I changed the hard disk, so I can't promise that I still have them :-(

[Nikolas Zimmermann]

Created attachment 60530 [details]
Updated patch v2

[Dirk Schulze]

Comment on attachment 60530 [details]
Updated patch v2

r=me

[Nikolas Zimmermann]

Landed in r62488.