[webkit-reviews] review denied: [Bug 32197] feDiffuseLighting filter is not implemented : [Attachment 54937] first draft again
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 3 10:43:56 PDT 2010
Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 32197: feDiffuseLighting filter is not implemented
https://bugs.webkit.org/show_bug.cgi?id=32197
Attachment 54937: first draft again
https://bugs.webkit.org/attachment.cgi?id=54937&action=review
------- Additional Comments from Dirk Schulze <krit at webkit.org>
At first, the patch misses ChangeLogs. You did not add any new LayoutTests, nor
did you update any current LayoutTests.
Also, what does "Use -grapcssystem raster on Qt, or an OS which fully support
QPixmaps
" mean? Only Qt will display this effect? The current SVG filters
implementation is meant to be platform aware.
WebCore/WebCore.pro:1813
+ svg/graphics/filters/SVGFEMerge.h \
Other build sytems like GNUMakefile, Gyp, Mac, Win, Android... need an update
too.
WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:32
+ FEDiffuseLighting::FEDiffuseLighting(FilterEffect* in, const Color&
lightingColor, const float& surfaceScale,
There is nothing left of FEDiffuseLighting, maybe you should think about
getting rid of FEDiffuseLighting and FESpecularLighting and directly create
FELighting with an enum on the beloonging SVGFE*Element's.
WebCore/svg/graphics/filters/SVGFELighting.cpp:106
+ float surfaceScale = m_surfaceScale / 255.0;
255.0f
WebCore/svg/graphics/filters/SVGLightSource.cpp:67
+ if (ls > 0) {
if (ls)
WebCore/svg/graphics/filters/SVGLightSource.cpp:83
+ float azimuth = m_azimuth * M_PI / 180;
Don't we have constants for M_PI / 180? Otherwise you should create one:
kDegreeToRad or so.
Is it a complete implementation of all
lighting effects? Both of the two effects call FELighting. Are more tests
affected by this
patch? How do the SVG tests of the official SVG test suite look like?
r- so far
More information about the webkit-reviews
mailing list