RESOLVED FIXED 43954
An individual renderer should be assigned to each SVGFE*Element class
https://bugs.webkit.org/show_bug.cgi?id=43954
Summary An individual renderer should be assigned to each SVGFE*Element class
Zoltan Herczeg
Reported 2010-08-13 00:57:45 PDT
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.
Attachments
Draft patch (186.36 KB, patch)
2010-09-03 04:36 PDT, Zoltan Herczeg
krit: review-
patch (192.12 KB, patch)
2010-09-06 04:43 PDT, Zoltan Herczeg
no flags
Path for Dirk (192.63 KB, patch)
2010-09-07 03:20 PDT, Zoltan Herczeg
krit: review-
Patch 2 for Dirk (192.71 KB, patch)
2010-09-07 06:46 PDT, Zoltan Herczeg
krit: review+
update the expected files (96.91 KB, patch)
2010-09-09 06:16 PDT, Zoltan Herczeg
zimmermann: review+
Zoltan Herczeg
Comment 1 2010-09-03 04:36:54 PDT
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 2 2010-09-03 06:37:59 PDT
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
Comment 3 2010-09-03 06:52:16 PDT
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
Comment 4 2010-09-03 07:07:46 PDT
> > 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
Comment 5 2010-09-06 04:43:45 PDT
Created attachment 66627 [details] patch Full patch contains all features discussed in #webkit.
Dirk Schulze
Comment 6 2010-09-06 05:37:26 PDT
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
Comment 7 2010-09-06 07:09:16 PDT
Csaba Osztrogonác
Comment 8 2010-09-06 08:08:46 PDT
(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
Comment 9 2010-09-06 08:09:51 PDT
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 10 2010-09-06 08:11:31 PDT
Comment on attachment 66627 [details] patch Remove r+, because it was landed and rolled-out.
Dirk Schulze
Comment 11 2010-09-06 11:03:50 PDT
(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
Comment 12 2010-09-06 11:38:24 PDT
> 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
Comment 13 2010-09-07 03:20:57 PDT
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 14 2010-09-07 03:31:51 PDT
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
Comment 15 2010-09-07 06:46:43 PDT
Created attachment 66717 [details] Patch 2 for Dirk
Zoltan Herczeg
Comment 16 2010-09-07 07:00:32 PDT
> 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 17 2010-09-07 07:52:48 PDT
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
Comment 18 2010-09-08 00:41:59 PDT
Thank you for the review. http://trac.webkit.org/changeset/66955 Seems Windows likes the patch this time. Closing bug.
Dirk Pranke
Comment 19 2010-09-08 18:05:16 PDT
Given that the pixel test result is actually now yellow, should the text in the test be changed as well?
Zoltan Herczeg
Comment 20 2010-09-08 23:23:51 PDT
> 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
Comment 21 2010-09-09 06:16:48 PDT
Created attachment 67027 [details] update the expected files
Nikolas Zimmermann
Comment 22 2010-09-10 04:11:29 PDT
Comment on attachment 67027 [details] update the expected files r=me.
Note You need to log in before you can comment on or make changes to this bug.