Summary: | LightElement changes does not require relayout. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zoltan Herczeg <zherczeg> | ||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | krit, webkit.review.bot, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 55075 | ||||||||
Attachments: |
|
Description
Zoltan Herczeg
2011-01-27 02:15:53 PST
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).
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 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! Created attachment 80328 [details]
updated patch
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? 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.
Landed in http://trac.webkit.org/changeset/77240 Closing bug. |