Bug 43954 – An individual renderer should be assigned to each SVGFE*Element class

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Zoltan Herczeg]

This renderer should capture the drawing and layout events. The filter elements should be clever enough to repaint the necessary areas when an attribute changes, and not the whole filter region.

[Zoltan Herczeg]

Created attachment 66482 [details]
Draft patch

Dirk, Nico, I upload the current state of my patch to the bugzilla. Actually it fixes one layout test. Is this the soultion you talked about?

The main reason of seting r? to test the build system updates through the EWS. (Changelog is missing)

[Dirk Schulze]

Comment on attachment 66482 [details]
Draft patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66482&action=prettypatch

Realy great patch! Some notes:

> WebCore/rendering/RenderSVGFilterPrimitive.cpp:37
> +namespace WebCore {
> +
> +
> +
> +} // namespace WebCore
> +
Ok, this file is useless at the moment. It should be added once the logic gets added.

> WebCore/rendering/RenderSVGFilterPrimitive.h:37
> +class RenderSVGFilterPrimitive : public RenderSVGHiddenContainer {
We may should call it RenderSVGResourceFilterPrimitive, because it needs to be controlled by RenderSVGResourceFilter later.

> WebCore/rendering/RenderSVGFilterPrimitive.h:41
> +     RenderSVGFilterPrimitive(SVGStyledElement* filterElement, FilterEffect* filterEffect)
> +         : RenderSVGHiddenContainer(filterElement)
> +         , m_filterEffect(filterEffect)
Why don't you use SVGFilterPrimitiveStandardAttributes* primitiveElement here? I also wouldn't set the FilterEffect here.

> WebCore/rendering/RenderSVGFilterPrimitive.h:46
> +     RefPtr<FilterEffect> m_filterEffect;
We don't set the RefPtr with your patch, I would let it out here. We have to specify some kind of struct anyway here (see RenderSVGResource*.h). One SVGFE*Element can be used by different targetElements, because the root filterElement can be used by different targetElements. That means we have to store all values depending on the targetElement.

> WebCore/svg/SVGFELightElement.cpp:82
> +        if (parent() && parent()->renderer())
> +            RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent()->renderer());
This needs of course more testing. Is the parent realy a filterEffect? Don't call renderer() multiple times, store the pointer in a variable.

> WebCore/svg/SVGFELightElement.cpp:131
> +    if (!changedByParser && parent() && parent()->renderer())
> +        RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent()->renderer());
dito

> WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:142
> +    return new (arena) RenderSVGFilterPrimitive(this, 0);
talked about the CTor some lines before.

> WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:55
> +        if (renderer())
> +            RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer());
if (RenderObject* renderer = renderer())

> WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:61
> +    virtual bool rendererIsNeeded(RenderStyle*) { return true; }
This line is not neaded. true is the default.

An additional note. We should think about naming everything filterPrimitive instead of filterEffect to be compatible with the spec. This is more a note for me than an advice for you.

Again, the patch looks realy great. I wounder and am happy that this patch fixes tests instead of breaking tests. Did you run all SVG-tests? Great job so far.
(Reviewed the patch earlier, because the win-bots seem to take ages :-()

[Zoltan Herczeg]

Thanks for the review.

> > WebCore/rendering/RenderSVGFilterPrimitive.cpp:37
> > +namespace WebCore {
> > +
> > +
> > +
> > +} // namespace WebCore
> > +
> Ok, this file is useless at the moment. It should be added once the logic gets added.

Would double the efforts of managing build systems. Do you insist of adding it later?

> Again, the patch looks realy great. I wounder and am happy that this patch fixes tests instead of breaking tests. Did you run all SVG-tests? Great job so far.

Yeah. Actually that test was working on Qt but not on mac. After discussing the issue with Niko, I just landed it until we could fix it somehow. I am also happy this patch fixes it, and no need to debug why that is wrong :)

Other things are ok, I will do the changes.

[Dirk Schulze]

> 
> Would double the efforts of managing build systems. Do you insist of adding it later?
> 
Put the Ctor into cpp, thats better anyway.

[Zoltan Herczeg]

Created attachment 66627 [details]
patch

Full patch contains all features discussed in #webkit.

[Dirk Schulze]

Comment on attachment 66627 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66627&action=prettypatch

> WebCore/rendering/RenderObject.h:324
>      virtual bool isSVGResourceContainer() const { return false; }
>      virtual bool isSVGShadowTreeRootContainer() const { return false; }
> +    virtual bool isRenderSVGResourceFilterPrimitive() const { return false; }
Maybe we should name this isSVGResourceFilterPrimitive to be consistent with the other function names.

Please change the name everywhere you're using this function. Otherwise looks great. r=me with this change.

[Zoltan Herczeg]

Landed in http://trac.webkit.org/changeset/66823

[Csaba Osztrogonác]

(In reply to comment #7)
> Landed in http://trac.webkit.org/changeset/66823
And rolled out by http://trac.webkit.org/changeset/66830 .
See: https://bugs.webkit.org/show_bug.cgi?id=45266

error message on Windows bot:
Creating library C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\lib\WebKit.lib and object C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\lib\WebKit.exp
WebCore.lib(SVGAllInOne.obj) : error LNK2001: unresolved external symbol "public: __thiscall WebCore::RenderSVGResourceFilterPrimitive::RenderSVGResourceFilterPrimitive(class WebCore::SVGFilterPrimitiveStandardAttributes *)" (??0RenderSVGResourceFilterPrimitive@WebCore@@QAE@PAVSVGFilterPrimitiveStandardAttributes@1@@Z)
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals

[Csaba Osztrogonác]

Adam or Brian, have you got any idea what caused this build fail on Windows?
Did Zoltan miss something modification in Windows build system?

[Csaba Osztrogonác]

Comment on attachment 66627 [details]
patch

Remove r+, because it was landed and rolled-out.

[Dirk Schulze]

(In reply to comment #10)
> (From update of attachment 66627 [details])
> Remove r+, because it was landed and rolled-out.

I don't know the error message, but mybe the new renderer has to be added to RenderSVGAllInOne.

[Zoltan Herczeg]

> I don't know the error message, but mybe the new renderer has to be added to RenderSVGAllInOne.

Thought about it myself, but no other renderer was added to that file, and the renderer is in WebCore/render no in WebCore/svg. Tomorrow the other Zoltan promised me some help if he can make his Windows build system up again.

[Zoltan Herczeg]

Created attachment 66699 [details]
Path for Dirk

Adding #include "RenderSVGResourceFilterPrimitive.cpp" to SVGAllInOne.cpp fixed the Windows build, although I am not quite happy with solution since no other files are needed from WebCore/rendering. However I cannot see any other solution at the moment.

[Dirk Schulze]

Comment on attachment 66699 [details]
Path for Dirk

View in context: https://bugs.webkit.org/attachment.cgi?id=66699&action=prettypatch

> WebCore/svg/SVGAllInOne.cpp:29
> +#include "RenderSVGResourceFilterPrimitive.cpp"
You added it to the wrong list. Take http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGAllInOne.cpp. This should work as well. Can you test it again please?

[Zoltan Herczeg]

Created attachment 66717 [details]
Patch 2 for Dirk

[Zoltan Herczeg]

> You added it to the wrong list. Take http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGAllInOne.cpp. This should work as well. Can you test it again please?

Fixed. & working under Windows as well. Thanks.

[Dirk Schulze]

Comment on attachment 66717 [details]
Patch 2 for Dirk

View in context: https://bugs.webkit.org/attachment.cgi?id=66717&action=prettypatch

> WebCore/ChangeLog:3
> +        Reviewed by Dirk Schulze.
Haha. You already added me as reviewer? :-)

Great patch! r=me

Take care of win, seems like the win-bot couldn't apply your patch.

[Zoltan Herczeg]

Thank you for the review.

http://trac.webkit.org/changeset/66955

Seems Windows likes the patch this time. Closing bug.

[Dirk Pranke]

Given that the pixel test result is actually now yellow, should the text in the test be changed as well?

[Zoltan Herczeg]

> Given that the pixel test result is actually now yellow, should the text in the test be changed as well?

Wow, forgot to change the text :( Thanks for notifing me.

[Zoltan Herczeg]

Created attachment 67027 [details]
update the expected files

[Nikolas Zimmermann]

Comment on attachment 67027 [details]
update the expected files

r=me.