[webkit-reviews] review denied: [Bug 52200] Small filter primitive renderer improvements : [Attachment 79729] patch again

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 52200: Small filter primitive renderer improvements
https://bugs.webkit.org/show_bug.cgi?id=52200

Attachment 79729: patch again
https://bugs.webkit.org/attachment.cgi?id=79729&action=review

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

(In reply to comment #27)
> > Yes, like I wrote it in https://bugs.webkit.org/show_bug.cgi?id=52200#c18.
This is the common way like  we do it for all elements.
> 
> ??? still I don't undestand you.
> 
> You said:
> 
> * SVGFEDiffuseLightingElement::svgAttributeChanged calls
renderer()->primitiveAttributeChanged(attrName);
> * RenderSVGResourceFilterPrimitive::primitiveAttributeChanged(const
QualifiedName& attrName) calls parent()->primitveAttributeChanged(this, const
QualifiedName& attrName);
> 
> That is exactly what the code snippet do I copied in my previous comment.
> 
> SVGFEDiffuseLightingElement calls its
render()->primitiveAttributeChanged(...) but renderer() requires a static_cast
to RenderSVGResourceFilterPrimitive, so I made an inline function in the base
class to make the code more readable.

Thats true, I was more thinking about consistency across other elements and
avoid as much calls of functions from other files as possible to have a better
overview. But in this case, and also because you create the renderer in
SVGFilterPrimitiveAttribute) it might be better to use your code as is. The
patch already looks great. Still some notes.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:151
> -	       return false;
> +	       return true;

This is difficult. If you return true, the filtered object continues drawing on
to the context: the real context! Means we'll draw the target on to the context
and on top of it we draw the filter result. Thats bad because of couple of
reasons. At first, because we draw the result of the filter with conpositeOver
to the context we may see both drawings. Second, the repaintRect of the target
depends on the filter area. If the filter area is outside of the
objectBoundingBox from the target, we won't clear the results on animations or
what ever. Third, we do unnecessary drawings. The filtered target element might
be the root element of thounds of childs, that is inefficient. Why do you have
to return true here? Shouldn't it work with false as well? We would enter
postApply() even it we return false here.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:300
> +	       filterData->builded = true;
> +	   }
> +
> +	   if (!lastEffect->hasResult()) {
> +	       // Always true if filterData is just built.

Why did you move 'filterData->builded = true;' atop of 'if
(!lastEffect->hasResult())' Isn't it build after we called apply() the first
time? and the 'if' doesn't even check filterData->builded so it shouldn't
matter here, but makes more sense in general to set it as true, if we really
built it.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:363
> +	   repaintRectangle(filterData->absoluteDrawingRegion);

Interesting, but you couldn't check it on your mac?

what if you have another element that is drawn on top of the filtered object?
will it get repainted as well? (Unsure right now, a test case would help) Also
isn't the parent renderer hierarchy RenderSVGResourceContainer->
RenderSVGHiddenContainer? Does it even affect any redrawings? I'd move this to
another patch an invalidate like we did before. So you cann add more tests to
check the correct behavior of the code.

> Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.h:54
> +	  
static_cast<RenderSVGResourceFilter*>(parent())->primitiveAttributeChanged(this
, attribute);

please check if the parent exists and if it is really a
RenderSVGResourceFilter, IIRC it don't needs to be a RenderSVGResourceFilter.
We even create a renderer in this case:
<g>
<feOffset/>
</g>

parent() would be RenderSVGContainer.