[webkit-reviews] review denied: [Bug 32197] feDiffuseLighting filter is not implemented : [Attachment 55696] redesigned patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.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 55696: redesigned patch
https://bugs.webkit.org/attachment.cgi?id=55696&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
WebCore/platform/graphics/FloatPoint3D.h:30
 +  class FloatPoint;
If you're including FloatPoint.h, the class forward can go away. I assume you
need the include because you've inlined the FloatPoint copy-construtor.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:33
 +	const float diffuseConstant, const float kernelUnitLengthX, const float
kernelUnitLengthY, PassRefPtr<LightSource> lightSource)
Please replace "const float" by "float". You've only changed it from
const-references to const objects, there's no gain in using const here.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:39
 +	const float surfaceScale, const float diffuseConstant, const float
kernelUnitLengthX,
Ditto.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.h:34
 +	    static PassRefPtr<FEDiffuseLighting> create(FilterEffect*, const
Color&, const float, const float,
Ditto. NOTE: Please leave "const Color&" - non-POD types should still be passed
as const-refererences, only POD types like float/int/etc.. not.

WebCore/svg/graphics/filters/SVGFELighting.cpp:39
 +  FELighting::FELighting(LightingType lightingType, FilterEffect* in, const
Color& lightingColor, const float surfaceScale,
Ditto.

WebCore/svg/graphics/filters/SVGFELighting.cpp:56
 +  const static int pixelSize = 4;
I'd prefer another naming scheme to easily spot statics while reading the code,
something like "cPixelSize" or "gPixelSize". The coding style guideline doesn't
say anything explicit here, several places in the code use it like you've done,
others use a "c" prefix. 

WebCore/svg/graphics/filters/SVGFELighting.cpp:75
 +  static inline int upLeftPixelValue(LightingData& data)
 +  {
 +	return static_cast<int>(data.pixels->get(data.offset -
data.widthMultipliedByPixelSize - pixelSize + alphaChannelOffset));
Excellent!

WebCore/svg/graphics/filters/SVGFELighting.cpp:135
 +	    if (data.specularExponent == 1.0)
Should be "1.0f".

WebCore/svg/graphics/filters/SVGFELighting.cpp:164
 +	data.lightingType = m_lightingType;
Hmm, isn't it possible to just store a pointer to the current FELighting object
in LightingData? Otherwhise you're duplicating the storage of
lightingType/lightSsource/diffuseConstant/etc..


WebCore/svg/graphics/filters/SVGFELighting.cpp:250
 +		data.pixels->set(i, static_cast<unsigned char>(0xff));
Please move the magic 0xff value to a constant out-of-the-loop "cMyColorValue"
or something. To avoid doing this cast on every iteration of the loop.

WebCore/svg/graphics/filters/SVGFELighting.h:41
 +	class CanvasPixelArray;
Indention is wrong in this file, code inside namespace should not be indented.

WebCore/svg/graphics/filters/SVGFESpecularLighting.cpp:32
 +  FESpecularLighting::FESpecularLighting(FilterEffect* in, const Color&
lightingColor, const float surfaceScale,
The "const float" issue again, please use "float".

WebCore/svg/graphics/filters/SVGLightSource.h:72
 +	    virtual void updatePaintingData(PaintingData&, int x, int y, float
z) = 0;
Hm, could you explain again why x/y are int and float is z? I may have
overlooked the obvious reason.

Only very minor issues remaining, almost there!