[webkit-reviews] review granted: [Bug 19991] WebKit needs cross-platform filter system : [Attachment 31157] subRegion Code

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted 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 31157: subRegion Code
https://bugs.webkit.org/attachment.cgi?id=31157&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
 7	   <!-- Hier mit flood? -->
 :)  Most of the team doesn't speak german.

No argument name needed:
 60	    FloatRect calculateUniteEffectRect(Filter* filter);

It's sad that more of the calculateUniteEffectRect code can't be shared.  It
seems like there are two variants of that code.  I wonder if we should put it
in a base class instead of copying it to every subclass?  Subclasses could
implement some other method which returns a bool to control which of the two
common implementations to use?

This if is not needed:
 62	if (m_mergeInputs.size() > 1) {

The for already takes care of that.

This code:
 84	filterContext->fillRect(FloatRect(0.f, 0.f, subRegion().width(),
subRegion().height()), color);

would be clearer as:
FloatRect(FloatPoint(), subregion.size())

The explicit color construction is not needed here:
 82	Color color = Color(colorWithOverrideAlpha(floodColor().rgb(),
floodOpacity()));

 76	IntSize bufferSize = IntSize(subRegion().width(),
subRegion().height());

why not just subRegion().size()?
or enclosingIntSize(...) if necessary?

No need to call .get(), OwnPtr will convert to a bool automatically:
0     if (!filterGraphic.get())
 61	    return;

I woudl really like to see a way to get rid of all the calculateUniteEffectRect
copy/paste code.

r- for the nits above.	I'd like to see this again... ideally with the
calculateUniteEffectRect copy/paste gone if possible. :)