[webkit-reviews] review denied: [Bug 32197] feDiffuseLighting filter is not implemented : [Attachment 54937] first draft again

[bugzilla-daemon at webkit.org]

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