[webkit-reviews] review denied: [Bug 5860] feComponentTransfer doesn't work : [Attachment 6496] fixed up patch

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Wed Feb 15 01:42:15 PST 2006


Eric Seidel <macdome at opendarwin.org> has denied Oliver Hunt
<ojh16 at student.canterbury.ac.nz>'s request for review:
Bug 5860: feComponentTransfer doesn't work
http://bugzilla.opendarwin.org/show_bug.cgi?id=5860

Attachment 6496: fixed up patch
http://bugzilla.opendarwin.org/attachment.cgi?id=6496&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
So we talked about this on irc.

A couple things I'd really like to see in this patch before we finally put it
to rest (now that I've read it more carefully).

1.  There were one or two style issues foo=bar; instead of the recomended foo =
bar;

2.  KCComponentTransferFunction really should have a default initializer,
instead of making KCanvasFEComponentTransfer() do the dirty work.

3. [filter setValue:[getFilterForFunc(alphaFunction(), inputImage, [CIVector
vectorWithX:0.0 Y:0.0 Z:0.0 W:1.0]) valueForKey:@"outputImage"]  would be much
simpler if it was made into a function getImageForFunc(alpha, inputImage)  It
doesn't even need to pass "inputImage" if it's a function on the class...

4.  We're really trying to kill all of the List classes (we basically have 0
need for a less effiicient list structure) and instead use Vector<T> for
everything.  In this case, you may still use Q3ValueList for tableValues, given
other existing code.. but if you can change to Vector<float> that's
recommended.

5.  A nit: filterForComponentFunc might be cleaner if you switched on the name
first, then called CIFilter filterWithName: on the resulting name... just a
thought, it's fine either way.

6.  An empty:
kcanvas/device/quartz/filters/WKDisplacementMapFilter.cikernel
 found its way into this patch.

7.  Given the awful inadequacy of the W3C SVG test suite, it would be best if
we could land at least a couple more test cases with this patch.

Given all those comments, I think it's best if we push this one through one
more round.  I hope you don't mind too much.  It's a wonderful work as always!



More information about the webkit-reviews mailing list