Bug 41566 - Logic to track whether elements are using relative lengths is incomplete
Summary: Logic to track whether elements are using relative lengths is incomplete
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: 15391
  Show dependency treegraph
 
Reported: 2010-07-03 01:17 PDT by Nikolas Zimmermann
Modified: 2010-07-05 07:35 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (53.51 KB, patch)
2010-07-03 01:37 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (53.51 KB, patch)
2010-07-03 01:41 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v2 (176.15 KB, patch)
2010-07-05 07:23 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-07-03 01:17:04 PDT
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.
Comment 1 Nikolas Zimmermann 2010-07-03 01:37:57 PDT
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.
Comment 2 WebKit Review Bot 2010-07-03 01:39:27 PDT
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.
Comment 3 Nikolas Zimmermann 2010-07-03 01:41:28 PDT
Created attachment 60444 [details]
Updated patch

Oops, remove leftover tab.
Comment 4 Dirk Schulze 2010-07-03 08:13:41 PDT
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?
Comment 5 Nikolas Zimmermann 2010-07-05 01:57:21 PDT
(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.
Comment 6 Dirk Schulze 2010-07-05 02:02:42 PDT
(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 :-(
Comment 7 Nikolas Zimmermann 2010-07-05 07:23:27 PDT
Created attachment 60530 [details]
Updated patch v2
Comment 8 Dirk Schulze 2010-07-05 07:30:03 PDT
Comment on attachment 60530 [details]
Updated patch v2

r=me
Comment 9 Nikolas Zimmermann 2010-07-05 07:35:49 PDT
Landed in r62488.