Bug 53232 – LightElement changes does not require relayout.

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Zoltan Herczeg]

5x speedup on svg-filter-animation.svg (from 6.6 fps to 30 fps on a Mac mini).

[Zoltan Herczeg]

Created attachment 80310 [details]
patch

I think LightSource.cpp should be split up into 3 files, but I think that should go to a separate patch (requires build file changes, and not the scope of this patch).

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/svg/SVGFELightElement.cpp:103:  Declaration has space between type name and * in SVGFilterPrimitiveStandardAttributes *parentPrimitive  [whitespace/declaration] [3]
Total errors found: 1 in 15 files


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

[Nikolas Zimmermann]

Comment on attachment 80310 [details]
patch

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

> Source/WebCore/platform/graphics/filters/LightSource.h:86
> +    virtual bool setAzimuth(float) { return false; }
> +    virtual bool setElevation(float) { return false; }
> +    virtual bool setX(float) { return false; }
> +    virtual bool setY(float) { return false; }
> +    virtual bool setZ(float) { return false; }
> +    virtual bool setPointsAtX(float) { return false; }
> +    virtual bool setPointsAtY(float) { return false; }
> +    virtual bool setPointsAtZ(float) { return false; }
> +    virtual bool setSpecularExponent(float) { return false; }
> +    virtual bool setLimitingConeAngle(float) { return false; }

Do we really need all those virtual methods? Is there no way to find out wheter a LightSource is eg. a PointLightSource, then cast to it? and call the setter there, instead of having to declare a lot of virtual functions here?
The general concept of the patch seems fine, but I dislike the addition of that many virtual methods.

Please look into another approach! Thanks in advance!

[Zoltan Herczeg]

Created attachment 80328 [details]
updated patch

[Dirk Schulze]

Comment on attachment 80328 [details]
updated patch

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

r=me

> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:123
> +    ASSERT_NOT_REACHED();

Is this assertion correct here? Why can't/shouldn't we pass a wrong attribute here?

[Zoltan Herczeg]

Thanks for the review.

> > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:123
> > +    ASSERT_NOT_REACHED();
> 
> Is this assertion correct here? Why can't/shouldn't we pass a wrong attribute here?

That is correct, since the function is (transitively) called from another function of the same Element, if it recognised the attribute. Thus wrong / unknown attributes should not be passed to this particular function.

[Zoltan Herczeg]

Landed in http://trac.webkit.org/changeset/77240
Closing bug.