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
Product: WebKit
Classification: Unclassified
Component: SVG
: 525.x (Safari 3.1)
: All All
: P2 Major
Assigned To: Nobody
:
Depends on:
Blocks: 45453 68469 26389
  Show dependency treegraph
 
Reported: 2010-07-14 02:48 PDT by Nikolas Zimmermann
Modified: 2014-05-12 05:54 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-07-14 02:48:00 PDT
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 Zoltan Herczeg 2010-07-15 02:00:45 PDT
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 Nikolas Zimmermann 2010-07-15 06:25:47 PDT
(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 Zoltan Herczeg 2010-07-19 05:52:47 PDT
Created attachment 61942 [details]
draft patch
Comment 4 Zoltan Herczeg 2010-07-19 05:58:10 PDT
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 Dirk Schulze 2010-07-19 06:14:15 PDT
(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 Zoltan Herczeg 2010-07-19 06:43:13 PDT
> 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 Nikolas Zimmermann 2010-07-19 13:25:06 PDT
(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 Nikolas Zimmermann 2010-07-20 00:50:57 PDT
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 Zoltan Herczeg 2010-07-20 00:54:59 PDT
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 Nikolas Zimmermann 2010-07-20 00:58:08 PDT
(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 Zoltan Herczeg 2010-07-20 06:05:19 PDT
Created attachment 62060 [details]
Patch for FilterElement properties
Comment 12 Nikolas Zimmermann 2010-07-20 06:29:53 PDT
Comment on attachment 62060 [details]
Patch for FilterElement properties

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 Zoltan Herczeg 2010-07-20 06:47:34 PDT
(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 Nikolas Zimmermann 2010-07-20 06:54:11 PDT
(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 Zoltan Herczeg 2010-07-21 02:05:45 PDT
Created attachment 62156 [details]
Patch for FilterElement properties (v2)
Comment 16 Zoltan Herczeg 2010-07-21 02:09:19 PDT
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 Nikolas Zimmermann 2010-07-21 02:28:37 PDT
Comment on attachment 62156 [details]
Patch for FilterElement properties (v2)

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 Zoltan Herczeg 2010-07-21 03:06:05 PDT
FilterElement patch landed in http://trac.webkit.org/changeset/63809
Comment 19 WebKit Review Bot 2010-07-21 03:16:07 PDT
http://trac.webkit.org/changeset/63809 might have broken Qt Linux Release
Comment 20 Zoltan Herczeg 2010-07-22 04:54:52 PDT
Created attachment 62287 [details]
Patch for StandardAttributes properties
Comment 21 Nikolas Zimmermann 2010-07-22 05:16:58 PDT
Comment on attachment 62287 [details]
Patch for StandardAttributes properties

Looks good, r=me.
Comment 22 Zoltan Herczeg 2010-07-22 05:52:09 PDT
StandardAttributes patch landed in http://trac.webkit.org/changeset/63883
Comment 23 WebKit Review Bot 2010-07-22 06:02:17 PDT
http://trac.webkit.org/changeset/63883 might have broken Qt Linux Release
Comment 24 Zoltan Herczeg 2010-07-27 02:00:12 PDT
Created attachment 62655 [details]
Patch for DiffuseLighting properties
Comment 25 Zoltan Herczeg 2010-07-27 05:44:38 PDT
Created attachment 62684 [details]
 Patch for feOffset properties
Comment 26 Nikolas Zimmermann 2010-07-27 06:29:56 PDT
Comment on attachment 62655 [details]
Patch for DiffuseLighting properties

r=me, as discussed on IRC.
Comment 27 Nikolas Zimmermann 2010-07-27 06:31:00 PDT
Comment on attachment 62684 [details]
 Patch for feOffset properties

r=me.
Comment 28 Zoltan Herczeg 2010-07-28 01:37:58 PDT
feDiffuseLighting patch landed in: http://trac.webkit.org/changeset/64191
feOffset patch landed in: http://trac.webkit.org/changeset/64192
Comment 29 Zoltan Herczeg 2010-08-03 04:54:48 PDT
Created attachment 63325 [details]
Patch for feSpotLight properties
Comment 30 Zoltan Herczeg 2010-08-04 06:21:45 PDT
Created attachment 63445 [details]
Patch for feSpotLight properties (v2)

As discussed in irc
Comment 31 Nikolas Zimmermann 2010-08-04 06:49:15 PDT
Comment on attachment 63445 [details]
Patch for feSpotLight properties (v2)

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 Zoltan Herczeg 2010-08-04 06:55:37 PDT
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 Nikolas Zimmermann 2010-08-04 07:01:43 PDT
(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 Zoltan Herczeg 2010-08-05 06:20:53 PDT
feSpotLight patch landed in http://trac.webkit.org/changeset/64716.
Comment 35 Renata Hodovan 2010-09-07 02:28:31 PDT
Created attachment 66698 [details]
Patch for fePointLight properties
Comment 36 Dirk Schulze 2010-09-07 04:40:13 PDT
Comment on attachment 66698 [details]
Patch for fePointLight properties

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 Renata Hodovan 2010-09-07 06:25:33 PDT
Created attachment 66714 [details]
Patch for fePointLight properties
Comment 38 Dirk Schulze 2010-09-07 06:28:33 PDT
Comment on attachment 66714 [details]
Patch for fePointLight properties

Great tests!
Comment 39 WebKit Commit Bot 2010-09-07 07:07:48 PDT
Comment on attachment 66714 [details]
Patch for fePointLight properties

Clearing flags on attachment: 66714

Committed r66881: <http://trac.webkit.org/changeset/66881>
Comment 40 WebKit Commit Bot 2010-09-07 07:08:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Renata Hodovan 2010-09-08 03:01:49 PDT
Created attachment 66864 [details]
Patch for feDistantLight properties
Comment 42 Zoltan Herczeg 2010-09-08 03:03:42 PDT
CQ closed the bug
Comment 43 Dirk Schulze 2010-09-08 05:17:13 PDT
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 Renata Hodovan 2010-09-08 05:59:16 PDT
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 Renata Hodovan 2010-09-08 07:34:17 PDT
Created attachment 66896 [details]
Patch for feDistantLight properties
Comment 46 Dirk Schulze 2010-09-08 23:51:33 PDT
Comment on attachment 66896 [details]
Patch for feDistantLight properties

r=me
Comment 47 WebKit Commit Bot 2010-09-09 01:08:15 PDT
Comment on attachment 66896 [details]
Patch for feDistantLight properties

Clearing flags on attachment: 66896

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