Bug 43954 - An individual renderer should be assigned to each SVGFE*Element class
Summary: An individual renderer should be assigned to each SVGFE*Element class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 45266
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-13 00:57 PDT by Zoltan Herczeg
Modified: 2010-09-10 04:11 PDT (History)
7 users (show)

See Also:


Attachments
Draft patch (186.36 KB, patch)
2010-09-03 04:36 PDT, Zoltan Herczeg
krit: review-
Details | Formatted Diff | Diff
patch (192.12 KB, patch)
2010-09-06 04:43 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
Path for Dirk (192.63 KB, patch)
2010-09-07 03:20 PDT, Zoltan Herczeg
krit: review-
Details | Formatted Diff | Diff
Patch 2 for Dirk (192.71 KB, patch)
2010-09-07 06:46 PDT, Zoltan Herczeg
krit: review+
Details | Formatted Diff | Diff
update the expected files (96.91 KB, patch)
2010-09-09 06:16 PDT, Zoltan Herczeg
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 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.
Comment 1 Zoltan Herczeg 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)
Comment 2 Dirk Schulze 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 :-()
Comment 3 Zoltan Herczeg 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.
Comment 4 Dirk Schulze 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.
Comment 5 Zoltan Herczeg 2010-09-06 04:43:45 PDT
Created attachment 66627 [details]
patch

Full patch contains all features discussed in #webkit.
Comment 6 Dirk Schulze 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.
Comment 7 Zoltan Herczeg 2010-09-06 07:09:16 PDT
Landed in http://trac.webkit.org/changeset/66823
Comment 8 Csaba Osztrogonác 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
Comment 9 Csaba Osztrogonác 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?
Comment 10 Csaba Osztrogonác 2010-09-06 08:11:31 PDT
Comment on attachment 66627 [details]
patch

Remove r+, because it was landed and rolled-out.
Comment 11 Dirk Schulze 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.
Comment 12 Zoltan Herczeg 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.
Comment 13 Zoltan Herczeg 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.
Comment 14 Dirk Schulze 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?
Comment 15 Zoltan Herczeg 2010-09-07 06:46:43 PDT
Created attachment 66717 [details]
Patch 2 for Dirk
Comment 16 Zoltan Herczeg 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.
Comment 17 Dirk Schulze 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.
Comment 18 Zoltan Herczeg 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.
Comment 19 Dirk Pranke 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?
Comment 20 Zoltan Herczeg 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.
Comment 21 Zoltan Herczeg 2010-09-09 06:16:48 PDT
Created attachment 67027 [details]
update the expected files
Comment 22 Nikolas Zimmermann 2010-09-10 04:11:29 PDT
Comment on attachment 67027 [details]
update the expected files

r=me.