[webkit-reviews] review denied: [Bug 5192] WebKit+SVG is missing support for many Filter Elements : [Attachment 4626] Lighting patch v3

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 5192: WebKit+SVG is missing support for many Filter Elements
http://bugzilla.opendarwin.org/show_bug.cgi?id=5192

Attachment 4626: Lighting patch v3
http://bugzilla.opendarwin.org/attachment.cgi?id=4626&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
Ok, my raft of comments on your patch:

GREAT WORK.  Amazing screen shot you sent around earlier.

There are a bunch of little style issues to fix:
if( -- space between if and (
foo(){} -- space between ) and {
foo() {}; -- no need for ;
if (foo) bar();  -- No one-line ifs,  bar() needs to have its' own line,
indented.

KCanvasPoint3F makePoint() should be part of KCanvas3F, not a static function
in ksvg2.

Long constructor functions should be moved into the .cpp files, instead of the
headers.

Constructors should use c++ style inits m_foo(foo) instead of m_foo = foo;


Some functional issues too:

<feDiffuseLighting> and <feSpecularLighting> can have other nodes than just a
single lightsource in them.  So you need to walk the child nodes, looking for
the first source and use that.

Your custom filter code should take CIImage inputs instead of CISamplers and
should make the appropriate CISamplers from CIFilters in the apply function.

Don't call +initialize directly.  That's done for you at the first mention of
the class.  Call something innocous like [MyFilter class]; instead.

Yeah, in general, don't pass around CISamplers... think of that as an
"internal" class which CIFilters use.

It's better not to use convenience functions like:
outputImageWithNormals, but rather to lookup filters by name, reset them to
defaults, set each default, and then grab the output image (a CIImage) to pass
to the next filter.  Folowing this format makes for easier to read code, and
code to which it's easier to add things like Filter sub-region support.

We should consider removing FE_POINT_LIGHT from the constants for filter types,
if they are not ever going to be constructed as filters by the device.

We still need to come up with a slick solution for either loading the kernel
files at runtime, or compiling the kernel source into WebKit as a static
string.  Writing a simple perl script to take in cikernel file and output a
header file with a single static string defined would be a slick solution.

convolve still should be changed to use 3 3-vectors instead of 9 individual
float lookups.

New files you created should have the same Apple-style BSD license header,
except with your name on them.	Files you copied from ksvg, should carry the
ksvg style LGPL header w/ your name (and the names from the file you copied). 
All files need a up-to-date copyright header however.

All and all, the patch looks GREAT!  There are just a few little nits which we
should clean up before we land this one.

Thanks again for all your hard work!