[Webkit-unassigned] [Bug 32197] feDiffuseLighting filter is not implemented

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32197


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55696|review?                     |review-
               Flag|                            |




--- Comment #28 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-11 06:49:38 PST ---
(From update of attachment 55696)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.