Bug 42244 - SVGFilterElement & SVGFE*Element don't support dynamic invalidation, when attributes change
: SVGFilterElement & SVGFE*Element don't support dynamic invalidation, when att...
Status: RESOLVED FIXED
: WebKit
SVG
: 525.x (Safari 3.1)
: All All
: P2 Major
Assigned To:
:
:
:
: 26389 45453
  Show dependency treegraph
 
Reported: 2010-07-14 02:48 PST by
Modified: 2010-09-09 04:56 PST (History)


Attachments
draft patch (6.25 KB, patch)
2010-07-19 05:52 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for FilterElement properties (1.05 MB, patch)
2010-07-20 06:05 PST, Zoltan Herczeg
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
Patch for FilterElement properties (v2) (1.13 MB, patch)
2010-07-21 02:05 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for StandardAttributes properties (725.36 KB, patch)
2010-07-22 04:54 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for DiffuseLighting properties (621.35 KB, patch)
2010-07-27 02:00 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for feOffset properties (318.44 KB, patch)
2010-07-27 05:44 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for feSpotLight properties (922.85 KB, patch)
2010-08-03 04:54 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for feSpotLight properties (v2) (927.04 KB, patch)
2010-08-04 06:21 PST, Zoltan Herczeg
no flags Review Patch | Details | Formatted Diff | Diff
Patch for fePointLight properties (507.22 KB, patch)
2010-09-07 02:28 PST, Renata Hodovan
krit: review-
Review Patch | Details | Formatted Diff | Diff
Patch for fePointLight properties (522.78 KB, patch)
2010-09-07 06:25 PST, Renata Hodovan
no flags Review Patch | Details | Formatted Diff | Diff
Patch for feDistantLight properties (769.16 KB, patch)
2010-09-08 03:01 PST, Renata Hodovan
no flags Review Patch | Details | Formatted Diff | Diff
Patch for feDistantLight properties (277.73 KB, patch)
2010-09-08 07:34 PST, Renata Hodovan
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-14 02:48:00 PST
Most filter related classes don't implement svgAttributeChanged / childrenChanged.
Stuff like <rect filter="url(#myfilter)" onclick="document.getElementById('myfilter').setAttribute('filterUnits', 'objectBoundingBox')" />  doesn't work because of that -> the filter is not invalidated.
It's just a matter of adding invalidateResourceClient() calls in the right places, see SVGClipPathElement as example.

Tests need to be added to svg/dynamic-updates, covering changing filter attributes through DOM / SVG DOM.
------- Comment #1 From 2010-07-15 02:00:45 PST -------
Can I use this as a template for other elements?

void SVGClipPathElement::childrenChanged(bool changedByParser, Node* beforeChange, Node* afterChange, int childCountDelta)
{
    SVGStyledTransformableElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);

    if (!changedByParser)
        invalidateResourceClients();
}

Why all base classes are tested? Isn't it should be done by SVGStyledTransformableElement::svgAttributeChanged? Do I need to check all base classes?

void SVGClipPathElement::svgAttributeChanged(const QualifiedName& attrName)
{
    SVGStyledTransformableElement::svgAttributeChanged(attrName);

    if (attrName == SVGNames::clipPathUnitsAttr ||
        SVGTests::isKnownAttribute(attrName) ||.
        SVGLangSpace::isKnownAttribute(attrName) ||
        SVGExternalResourcesRequired::isKnownAttribute(attrName) ||
        SVGStyledTransformableElement::isKnownAttribute(attrName))
        invalidateResourceClients();
}
------- Comment #2 From 2010-07-15 06:25:47 PST -------
(In reply to comment #1)
> Can I use this as a template for other elements?
> 
> void SVGClipPathElement::childrenChanged(bool changedByParser, Node* beforeChange, Node* afterChange, int childCountDelta)
> {
>     SVGStyledTransformableElement::childrenChanged(changedByParser, beforeChange, afterChange, childCountDelta);
> 
>     if (!changedByParser)
>         invalidateResourceClients();
> }

Yes you can, you just have to invalidate the parent filter, as the effects themselves don't have renderers.

> 
> Why all base classes are tested? Isn't it should be done by SVGStyledTransformableElement::svgAttributeChanged? Do I need to check all base classes?

Only SVGFooElement inherits from SVGTests/SVGURIRef, etc.. not SVGStyledTRanformableElement itself, so you have to do the check there.
------- Comment #3 From 2010-07-19 05:52:47 PST -------
Created an attachment (id=61942) [details]
draft patch
------- Comment #4 From 2010-07-19 05:58:10 PST -------
SVGFilterElement has svgAttributeChanged, but it seems there is no test case for it. My qestions:

1) Shall I create a test case for each attribute, or some aggrageted test case is enough? "x, y, width, height, filterUnits, primitiveUnits, filterRes" and "dom, svgdom", total of 7 * 2 = 14 test cases?

2) How can I create pixel test under mac (leopard)? -p does not work for svg/dynamic-updates :(
------- Comment #5 From 2010-07-19 06:14:15 PST -------
(In reply to comment #4)
> SVGFilterElement has svgAttributeChanged, but it seems there is no test case for it. My qestions:
There should be at least one test for filters. Not sure about it's name.
> 
> 1) Shall I create a test case for each attribute, or some aggrageted test case is enough? "x, y, width, height, filterUnits, primitiveUnits, filterRes" and "dom, svgdom", total of 7 * 2 = 14 test cases?
Yes, like you can see, we did it for other elements as well.
> 
> 2) How can I create pixel test under mac (leopard)? -p does not work for svg/dynamic-updates :(
Update to trunk. Should be possible now.
------- Comment #6 From 2010-07-19 06:43:13 PST -------
> There should be at least one test for filters. Not sure about it's name.

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

The patch focusing on relative lengths.

> Update to trunk. Should be possible now.

Updated this morning. And I don't see any pixel test related changes so far. Anmyway, I will try.
------- Comment #7 From 2010-07-19 13:25:06 PST -------
(In reply to comment #6)
> > There should be at least one test for filters. Not sure about it's name.
> 
> http://trac.webkit.org/changeset/62488
> 
> The patch focusing on relative lengths.
> 
> > Update to trunk. Should be possible now.
> 
> Updated this morning. And I don't see any pixel test related changes so far. Anmyway, I will try.

No, it's not enabled yet, still missing a small patchlet from my side. Will do this tomorrow, and check your patch! Sorry, very tired atm.
------- Comment #8 From 2010-07-20 00:50:57 PST -------
Hi Zoltan, patch looks just fine! You'll probably run into problems though, as you also need to implement svgAttributeChanged - I doubt the other attribute tests will work w/o it.

Just landed the svg/dynamic-updates pixel test generation patch, and rebaseline all of them - in r63730.
If you update now and use "run-webkit-tests --tolerance 0 -p svg/dynamic-updates" it will generate pixel test results for you.

(Sidenote: We have some regressions, since the pixel tests weren't activated for a long time for those tests, tracked by bug 42618 -- though I rebaselined all of them, even with errors, so you should see them pass all, with tolerance 0, at least on Leopard, what I use to develop).
------- Comment #9 From 2010-07-20 00:54:59 PST -------
Thanks Niko.

Test cases work under Qt, hopefully only the pixel expectations remain for mac. I will create all 14 test cases now, and update the patch today.
------- Comment #10 From 2010-07-20 00:58:08 PST -------
(In reply to comment #9)
> Thanks Niko.
> 
> Test cases work under Qt, hopefully only the pixel expectations remain for mac. I will create all 14 test cases now, and update the patch today.

Excellent!
------- Comment #11 From 2010-07-20 06:05:19 PST -------
Created an attachment (id=62060) [details]
Patch for FilterElement properties
------- Comment #12 From 2010-07-20 06:29:53 PST -------
(From update of attachment 62060 [details])
Looks great, I have some suggestions though:
It would be great, if all results would end up with the _same_ image. It's much easier to spot breakages, if the expected pngs would all look the same, if the invalidation worked properly - can you change that?
(Are you aware of make-script-test-wrappers, btw? That automatically creates the .html files for you)

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterRes-call.js:45
 +      filterElement.setFilterRes("400", "400");
These take numbers, you can omit the ".

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterResX-prop.js:44
 +      filterElement.filterResX.baseVal = "400";
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterResY-prop.js:44
 +      filterElement.filterResY.baseVal = "400";
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-height-prop.js:43
 +      filterElement.height.baseVal.value = "200";
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-width-prop.js:43
 +      filterElement.width.baseVal.value = "200";
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-x-prop.js:43
 +      filterElement.x.baseVal.value = "10";
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-y-prop.js:43
 +      filterElement.y.baseVal.value = "10";
Ditto.
------- Comment #13 From 2010-07-20 06:47:34 PST -------
(In reply to comment #12)
> It would be great, if all results would end up with the _same_ image.

Actually I intentionally made different images, because sometimes copy-paste programming could lead to the same result when somebody forget to change some things.

> (Are you aware of make-script-test-wrappers, btw? That automatically creates the .html files for you)

Never heard about it. And the script has no help.

Ok I will do these changes tomorrow.
------- Comment #14 From 2010-07-20 06:54:11 PST -------
(In reply to comment #13)
> (In reply to comment #12)
> > It would be great, if all results would end up with the _same_ image.
> 
> Actually I intentionally made different images, because sometimes copy-paste programming could lead to the same result when somebody forget to change some things.
Okay. For example SVGRectElement has tests that change x/y/width/height. We want to draw a 100x100 rectangle, from x=0/y=0. In the width test, I'd start with with="50" and let it grow to "100". In the x-test I'd start with x=10 and let it shrink to x=0. etc..

It's hard to tell just from the visual result, whether the test worked, that's all. We should have discussed this before, sorry that you were on the wrong track! :(

> 
> > (Are you aware of make-script-test-wrappers, btw? That automatically creates the .html files for you)
> 
> Never heard about it. And the script has no help.
Yeah, it's unfortuante. Whenever you place foo.js in script-tests/foo.js, it will generate foo.html for you from the TEMPLATE.html file - so you don't need to write them on your own.

> 
> Ok I will do these changes tomorrow.
Thanks a lot, hope you're not bored now :(
------- Comment #15 From 2010-07-21 02:05:45 PST -------
Created an attachment (id=62156) [details]
Patch for FilterElement properties (v2)
------- Comment #16 From 2010-07-21 02:09:19 PST -------
Thanks for the info, Niko.

Seems to me pointLight does not affected by the primitiveUnits of the filter, only its parent, the feDiffuseLighting effect. This is probably a bug.
------- Comment #17 From 2010-07-21 02:28:37 PST -------
(From update of attachment 62156 [details])
r=me, looks much better! Some quick fixes needed before landing:

LayoutTests/ChangeLog:5
 +          Testing SVGFilterElement dynamic property changes
The title in the ChangeLog does not correspond to the bug report, either change the bug title or the ChangeLog, whatever you like.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-filterRes-attr.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Name is wrong, please update.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-filterUnits-attr.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-height-attr.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-primitiveUnits-attr.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-width-attr.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-y-attr.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterRes-call.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterResX-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterResY-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-filterUnits-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-height-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-primitiveUnits-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-width-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-x-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.

LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-y-prop.js:1
 +  // [Name] SVGFilterElement-dom-x-attr.js
Ditto.
------- Comment #18 From 2010-07-21 03:06:05 PST -------
FilterElement patch landed in http://trac.webkit.org/changeset/63809
------- Comment #19 From 2010-07-21 03:16:07 PST -------
http://trac.webkit.org/changeset/63809 might have broken Qt Linux Release
------- Comment #20 From 2010-07-22 04:54:52 PST -------
Created an attachment (id=62287) [details]
Patch for StandardAttributes properties
------- Comment #21 From 2010-07-22 05:16:58 PST -------
(From update of attachment 62287 [details])
Looks good, r=me.
------- Comment #22 From 2010-07-22 05:52:09 PST -------
StandardAttributes patch landed in http://trac.webkit.org/changeset/63883
------- Comment #23 From 2010-07-22 06:02:17 PST -------
http://trac.webkit.org/changeset/63883 might have broken Qt Linux Release
------- Comment #24 From 2010-07-27 02:00:12 PST -------
Created an attachment (id=62655) [details]
Patch for DiffuseLighting properties
------- Comment #25 From 2010-07-27 05:44:38 PST -------
Created an attachment (id=62684) [details]
 Patch for feOffset properties
------- Comment #26 From 2010-07-27 06:29:56 PST -------
(From update of attachment 62655 [details])
r=me, as discussed on IRC.
------- Comment #27 From 2010-07-27 06:31:00 PST -------
(From update of attachment 62684 [details])
r=me.
------- Comment #28 From 2010-07-28 01:37:58 PST -------
feDiffuseLighting patch landed in: http://trac.webkit.org/changeset/64191
feOffset patch landed in: http://trac.webkit.org/changeset/64192
------- Comment #29 From 2010-08-03 04:54:48 PST -------
Created an attachment (id=63325) [details]
Patch for feSpotLight properties
------- Comment #30 From 2010-08-04 06:21:45 PST -------
Created an attachment (id=63445) [details]
Patch for feSpotLight properties (v2)

As discussed in irc
------- Comment #31 From 2010-08-04 06:49:15 PST -------
(From update of attachment 63445 [details])
r=me, with one comment:
WebCore/svg/SVGFilterElement.h:65
 +              if (parent->hasTagName(SVGNames::filterTag)) {

I'd prefer another way to write this function:

static void invalidateFilter(SVGElement* element)
{
    ASSERT(element);
    if (!element->inDocument())
        return;
    Node* parent = element->parentNode();
    while (parent && !parent->hasTagName(SVGNames::filterTag))
        parent = parent->parentNode();

    if (!parent)
        return;

    ASSERT(parent->hasTagName(SVGNames::filterTag));
    if (RenderObject* renderer = parent->renderer())
        renderer->setNeedsLayout(true);
}

Please modify to use more early returns before landing :-)
------- Comment #32 From 2010-08-04 06:55:37 PST -------
I don't see any reason for the second assert (as if the compiler could generate wrong code), but since it is an assert, it is ok.
------- Comment #33 From 2010-08-04 07:01:43 PST -------
(In reply to comment #32)
> I don't see any reason for the second assert (as if the compiler could generate wrong code), but since it is an assert, it is ok.

Right, remove it.
------- Comment #34 From 2010-08-05 06:20:53 PST -------
feSpotLight patch landed in http://trac.webkit.org/changeset/64716.
------- Comment #35 From 2010-09-07 02:28:31 PST -------
Created an attachment (id=66698) [details]
Patch for fePointLight properties
------- Comment #36 From 2010-09-07 04:40:13 PST -------
(From update of attachment 66698 [details])
It looks like you forgot to add the scripts. Or at least I can't see them :-)

Please add the scripts to the patch and update the ChangeLog. I would like to read more about what you're doing in the ChangeLog. Please add a comment that you added dynamic update tests for feLight effects.
------- Comment #37 From 2010-09-07 06:25:33 PST -------
Created an attachment (id=66714) [details]
Patch for fePointLight properties
------- Comment #38 From 2010-09-07 06:28:33 PST -------
(From update of attachment 66714 [details])
Great tests!
------- Comment #39 From 2010-09-07 07:07:48 PST -------
(From update of attachment 66714 [details])
Clearing flags on attachment: 66714

Committed r66881: <http://trac.webkit.org/changeset/66881>
------- Comment #40 From 2010-09-07 07:08:04 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #41 From 2010-09-08 03:01:49 PST -------
Created an attachment (id=66864) [details]
Patch for feDistantLight properties
------- Comment #42 From 2010-09-08 03:03:42 PST -------
CQ closed the bug
------- Comment #43 From 2010-09-08 05:17:13 PST -------
How many new patches should be inserted into this bug report? Can't we create new bug reports and take this one as master bug? Just set 'blocks 42244'. Can make it much easier to traverse trac.webkit.org, if every bug has it's own bug report.
------- Comment #44 From 2010-09-08 05:59:16 PST -------
Good idea. Every SVGElement need new test cases. It'll result a lot of patches. But it may be better to create a new master bug and link this one to that as well.


(In reply to comment #43)
> How many new patches should be inserted into this bug report? Can't we create new bug reports and take this one as master bug? Just set 'blocks 42244'. Can make it much easier to traverse trac.webkit.org, if every bug has it's own bug report.
------- Comment #45 From 2010-09-08 07:34:17 PST -------
Created an attachment (id=66896) [details]
Patch for feDistantLight properties
------- Comment #46 From 2010-09-08 23:51:33 PST -------
(From update of attachment 66896 [details])
r=me
------- Comment #47 From 2010-09-09 01:08:15 PST -------
(From update of attachment 66896 [details])
Clearing flags on attachment: 66896

Committed r67068: <http://trac.webkit.org/changeset/67068>
------- Comment #48 From 2010-09-09 01:08:32 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #49 From 2010-09-09 01:42:01 PST -------
http://trac.webkit.org/changeset/67068 might have broken GTK Linux 32-bit Release and Qt Linux Release