[Webkit-unassigned] [Bug 52200] Small filter primitive renderer improvements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52200


Dirk Schulze <krit at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #79729|review?                     |review-
               Flag|                            |




--- Comment #28 from Dirk Schulze <krit at webkit.org>  2011-01-21 13:21:55 PST ---
(From update of attachment 79729)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.