[webkit-reviews] review denied: [Bug 113040] [Texmap] TextureMapperImageBuffer should not use rendering code for filters. : [Attachment 194597] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Allan Sandfeld Jensen
<allan.jensen at digia.com>'s request for review:
Bug 113040: [Texmap] TextureMapperImageBuffer should not use rendering code for
filters.
https://bugs.webkit.org/show_bug.cgi?id=113040

Attachment 194597: Patch
https://bugs.webkit.org/attachment.cgi?id=194597&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194597&action=review


> Source/WebCore/ChangeLog:9
> +	   Split FilterEffectRenderer in a simple filter graphics part and
leave only
> +	   the parts requiring access to the DOM or Render tree in
FilterEffectRenderer.

Can you add more information why you are doing this? It would help ti
understand the patch more. After all you are adding another inheritance
hierarchy for the general use case (CSS filters on web pages) to reuse the
filter logic. Maybe a static function or moveing build() up to Filter.h makes
more sense? Depends on the use case.

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:217
> +    default:
> +	   break;

There should probably be an ASSERTION_NOT_REACHED as well here. To be honest, I
would like to have all filters listed here and remove the default.

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:233
> +	   switch (filterOperation->getOperationType()) {

I am not sure if that even needs a switch. A simple if should be sufficient
enough to catch the three cases.

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:247
> +	   if (effect) {

should be more

if (!effect)
    continue;

The question is, do you really just want to skip the operations you do not want
to handle? Shouldn't you rather stop the process? The user does not expect what
you do here anyway. But it also depends on the use case. So again, why do you
split FilterEffectsRenderer?

> Source/WebCore/platform/graphics/filters/ImageBufferFilter.cpp:259
> +    if (!m_effects.size())
> +	   return false;

You probably return false in the case of custom shaders and and references
above as well.