Attachment 54937[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGLightSource.cpp:25: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 3 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 54937[details]
first draft again
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

This is just a draft patch, not a finished one, so ChangeLog and build systems can be wait. What I need at this stage is the guidance of a reviewer :) )As Darin said before)
The question here is do you like my appoach, or you would prefer something else. In my patch, I combind the diffuse and specular lighting interfaces to a common base class. This base class contains all common, and own arguments. It makes the implementation much easier, but has a space overhead. I think this overhead is acceptable because of the low number of such objects. Do you agree? I mean you said "I should think about...", but you are the reviewer not me :) Would you prefer a patch doing that? Is that fit for the current implementation style or not?
The other thing is how the lighting source interface is changed. I have added members to the interface which is used when the painting happens. This is not a thread safe approach, but faster than passing the same arguments again and again. This is a balance question, do you prefer speed or thread safe operation?
I put this patch here, because only the diffuse lighting part is finished, except one thing: I don't understand the purpose of kernel units. The official definition is fuzzy, and firefox results for different kernel units are strange. It makes the image "dirty", I can't describe it better. Lots of random pixels appear. I thought it is kind of a scaling argument, but I am not sure anymore.
Also interesting, that the cut-off argument of spot light is not used by the spot light apply formulas. I could create rules for that, but not sure that is the best way, since I should follow the rules in the standard.
> Don't we have constants for M_PI / 180?
Where? Or where to put DegreeToRad?
First I plan to finish the code, the pixel tests can wait.

(In reply to comment #9)
> This is just a draft patch, not a finished one, so ChangeLog and build systems
> can be wait. What I need at this stage is the guidance of a reviewer :) )As
> Darin said before)
>
> The question here is do you like my appoach, or you would prefer something
> else. In my patch, I combind the diffuse and specular lighting interfaces to a
> common base class. This base class contains all common, and own arguments. It
> makes the implementation much easier, but has a space overhead. I think this
> overhead is acceptable because of the low number of such objects. Do you agree?
> I mean you said "I should think about...", but you are the reviewer not me :)
> Would you prefer a patch doing that? Is that fit for the current implementation
> style or not?
>
> The other thing is how the lighting source interface is changed. I have added
> members to the interface which is used when the painting happens. This is not a
> thread safe approach, but faster than passing the same arguments again and
> again. This is a balance question, do you prefer speed or thread safe
> operation?
>
> I put this patch here, because only the diffuse lighting part is finished,
> except one thing: I don't understand the purpose of kernel units. The official
> definition is fuzzy, and firefox results for different kernel units are
> strange. It makes the image "dirty", I can't describe it better. Lots of random
> pixels appear. I thought it is kind of a scaling argument, but I am not sure
> anymore.
>
> Also interesting, that the cut-off argument of spot light is not used by the
> spot light apply formulas. I could create rules for that, but not sure that is
> the best way, since I should follow the rules in the standard.
>
>
> > Don't we have constants for M_PI / 180?
> Where? Or where to put DegreeToRad?
>
> First I plan to finish the code, the pixel tests can wait.
The problem on combining the both effects is on changing the effects (by script or animation). We'll recreate the complete filter on changing one effect, but this should change in the future and only effects that need updates should be rerendered. So we might leave the FEDiffuseLighting.cpp.
For the graphic side of implementation, you should ask a graphic guy. Oliver Hunt and others have experience here.

About the "-graphcssystem raster" option. Today, I printed out the depth() meber of a QPixmap (X11 version), and it was 24. For an 8 bit RGB implementation, that means ther is no alpha channel. In practice, you have a 1 bit alpha cahnnel (the pixel is transparent or not), which is probably implemented by a special constant, or a bit map (1 bit for each pixel), I don't really know. 1 bit alpha channel is not enough for SVG effects. I.e: the lighting effects use the alpha channel as height vector.
One more interesting thing. Here is a code snippet:
w, h -> width height, anything > 0 will do
QPixmap pixmap(w, h);
pixmap.fill(Qt::transparent);
QPainter p(&pixmap);
p.fillRect(0, 0, w, h, Qt::red)
p.end();
pixmap.save("save.png");
On X11, you will see an empty image
If you comment out the .fill line, you will see a red box.

> The problem on combining the both effects is on changing the effects (by script
> or animation). We'll recreate the complete filter on changing one effect, but
> this should change in the future and only effects that need updates should be
> rerendered. So we might leave the FEDiffuseLighting.cpp.
That was exactly my thought. Ok, so I will keep the current classes. This apprach is more rbust for changes.
> For the graphic side of implementation, you should ask a graphic guy. Oliver
> Hunt and others have experience here.
CC'ing him. No matter where I am heading in WebKit, we always meet :)

Attachment 55111[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGLightSource.cpp:26: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
WebCore/svg/graphics/filters/SVGLightSource.cpp:60: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/svg/graphics/filters/SVGLightSource.cpp:77: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.cpp:34: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 6 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 55116[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 55119[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 55696[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Thank you. I only have a few questions, where the answer is not obvious (for me at :) )
> 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.
There is no change here, isn't it? It has already been defined as "const Color&". Passing pointers is faster, of course.
> 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..
The inline functions are not friend functions of FELighting, thus the protected members are not accessible. There are several possible workaround for that, and I chose the simplest one (without header modification). Shall I make them a member function? Or friend function (moving LightingData inside FELighting)? Or something else?
> 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.
You mentioned this before, but "static_cast<unsigned char>(0xff)" is a compile time constant, and there is no runtime casting. But I could move it out anyway.
>
> WebCore/svg/graphics/filters/SVGFELighting.h:41
> + class CanvasPixelArray;
> Indention is wrong in this file, code inside namespace should not be indented.
The style checker mention this as well, but most classes in WebKit is indented in the Header file, including other headers in the same directory.
> Hm, could you explain again why x/y are int and float is z? I may have overlooked the obvious reason.
I will put a comment there. z = alpha value (opaque treated as 1.0) scaled by a user specified constant. That constant is a float.
Next time I will add a ChangeLog and update mac pixel tests as well.

(In reply to comment #29)
> Thank you. I only have a few questions, where the answer is not obvious (for me at :) )
>
> > 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.
>
> There is no change here, isn't it? It has already been defined as "const Color&". Passing pointers is faster, of course.
Right, just wanted to mention that you shouldn't change that one :-)
> > 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..
>
> The inline functions are not friend functions of FELighting, thus the protected members are not accessible. There are several possible workaround for that, and I chose the simplest one (without header modification). Shall I make them a member function? Or friend function (moving LightingData inside FELighting)? Or something else?
Friendship sounds the fastest solution to me, choose whatever you like, if it avoids duplicating memory.
>
> > 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.
>
> You mentioned this before, but "static_cast<unsigned char>(0xff)" is a compile time constant, and there is no runtime casting. But I could move it out anyway.
Oh that's true of course. Hm, I still find it easier to read.
>
> >
> > WebCore/svg/graphics/filters/SVGFELighting.h:41
> > + class CanvasPixelArray;
> > Indention is wrong in this file, code inside namespace should not be indented.
>
> The style checker mention this as well, but most classes in WebKit is indented in the Header file, including other headers in the same directory.
Yeah it's a "new rule" compared to when this code appeared in trunk. We're fixing them over the time whenver we see the style bot complain.
>
> > Hm, could you explain again why x/y are int and float is z? I may have overlooked the obvious reason.
>
> I will put a comment there. z = alpha value (opaque treated as 1.0) scaled by a user specified constant. That constant is a float.
Good.
>
>
> Next time I will add a ChangeLog and update mac pixel tests as well.
Excellent!