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.
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)
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 :-()
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.
> > Would double the efforts of managing build systems. Do you insist of adding it later? > Put the Ctor into cpp, thats better anyway.
Created attachment 66627 [details] patch Full patch contains all features discussed in #webkit.
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.
Landed in http://trac.webkit.org/changeset/66823
(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
Adam or Brian, have you got any idea what caused this build fail on Windows? Did Zoltan miss something modification in Windows build system?
Comment on attachment 66627 [details] patch Remove r+, because it was landed and rolled-out.
(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.
> 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.
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.
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?
Created attachment 66717 [details] Patch 2 for Dirk
> 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.
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.
Thank you for the review. http://trac.webkit.org/changeset/66955 Seems Windows likes the patch this time. Closing bug.
Given that the pixel test result is actually now yellow, should the text in the test be changed as well?
> 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.
Created attachment 67027 [details] update the expected files
Comment on attachment 67027 [details] update the expected files r=me.