Bug 53232 - LightElement changes does not require relayout.
Summary: LightElement changes does not require relayout.
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 55075
  Show dependency treegraph
Reported: 2011-01-27 02:15 PST by Zoltan Herczeg
Modified: 2011-02-23 12:57 PST (History)
3 users (show)

See Also:

patch (27.10 KB, patch)
2011-01-27 02:31 PST, Zoltan Herczeg
zimmermann: review-
Details | Formatted Diff | Diff
updated patch (28.75 KB, patch)
2011-01-27 06:31 PST, Zoltan Herczeg
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 2011-01-27 02:15:53 PST
5x speedup on svg-filter-animation.svg (from 6.6 fps to 30 fps on a Mac mini).
Comment 1 Zoltan Herczeg 2011-01-27 02:31:56 PST
Created attachment 80310 [details]

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).
Comment 2 WebKit Review Bot 2011-01-27 02:35:11 PST
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.
Comment 3 Nikolas Zimmermann 2011-01-27 02:41:23 PST
Comment on attachment 80310 [details]

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!
Comment 4 Zoltan Herczeg 2011-01-27 06:31:43 PST
Created attachment 80328 [details]
updated patch
Comment 5 Dirk Schulze 2011-01-31 11:01:17 PST
Comment on attachment 80328 [details]
updated patch

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


> Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:123

Is this assertion correct here? Why can't/shouldn't we pass a wrong attribute here?
Comment 6 Zoltan Herczeg 2011-02-01 00:37:53 PST
Thanks for the review.

> > Source/WebCore/svg/SVGFEDiffuseLightingElement.cpp:123
> 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.
Comment 7 Zoltan Herczeg 2011-02-01 23:40:12 PST
Landed in http://trac.webkit.org/changeset/77240
Closing bug.