[webkit-reviews] review denied: [Bug 19991] WebKit needs cross-platform filter system : [Attachment 30513] initial SVG Filter patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 19991: WebKit needs cross-platform filter system
https://bugs.webkit.org/show_bug.cgi?id=19991

Attachment 30513: initial SVG Filter patch
https://bugs.webkit.org/attachment.cgi?id=30513&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
 48 FloatRect SVGResourceFilter::filterBBoxForItemBBox(const FloatRect&
itemBBox) const

really takes an objectBoundingBox in spec terms.  We really should make it
impossible to pass it something that isn't an objectBoundingBox, but that's for
a later patch.

Seems:
m_filterBuilder->lastFilter()
belongs in a local here:
128	    if (m_filterBuilder->lastFilter()->resultImage())
 129		
context->drawImage(m_filterBuilder->lastFilter()->resultImage()->image(), 
 130				 m_filterBuilder->lastFilter()->subRegion());

Don't we already have a conversion function for this?
     Color color = Color(floodColor().red() / 255.0f,
 83			    floodColor().green() / 255.0f,
 84			    floodColor().blue() / 255.0f,
 85			    floodColor().alpha() / 255.0 * floodOpacity());

If you transform the rect, you don't need to save/restore (which is expensive):

 80	filterContext->save();
 81	filterContext->translate(-subRegion().x(), -subRegion().y());
 82	Color color = Color(floodColor().red() / 255.0f,
 83			    floodColor().green() / 255.0f,
 84			    floodColor().blue() / 255.0f,
 85			    floodColor().alpha() / 255.0 * floodOpacity());
 86	filterContext->fillRect(subRegion(), color);
 87	filterContext->restore();

This would have been easier to review if the:
 virtual void apply();
60	    virtual void dump();
 60	    void apply(SVGResourceFilter*);
 61	    void dump();
changes could have been done in a first-pass.

In general this looks great!  So glad you're taking this up.  I think we should
see this one more round before landing though.	r- for the nits above.