| Summary: | SVGFilterElement & SVGFE*Element don't support dynamic invalidation, when attributes change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||||||||||||||||||
| Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Major | CC: | abarth, commit-queue, eric, jeffschiller, krit, rhodovan.u-szeged, webkit.review.bot, zherczeg, zimmermann | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 525.x (Safari 3.1) | ||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||
| Bug Blocks: | 68469, 26389, 45453 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
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();
}
(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. Created attachment 61942 [details]
draft patch
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 :( (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. > 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. (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. 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). 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. (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! Created attachment 62060 [details]
Patch for FilterElement properties
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.
(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. (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 :( Created attachment 62156 [details]
Patch for FilterElement properties (v2)
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 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.
FilterElement patch landed in http://trac.webkit.org/changeset/63809 http://trac.webkit.org/changeset/63809 might have broken Qt Linux Release Created attachment 62287 [details]
Patch for StandardAttributes properties
Comment on attachment 62287 [details]
Patch for StandardAttributes properties
Looks good, r=me.
StandardAttributes patch landed in http://trac.webkit.org/changeset/63883 http://trac.webkit.org/changeset/63883 might have broken Qt Linux Release Created attachment 62655 [details]
Patch for DiffuseLighting properties
Created attachment 62684 [details]
Patch for feOffset properties
Comment on attachment 62655 [details]
Patch for DiffuseLighting properties
r=me, as discussed on IRC.
Comment on attachment 62684 [details]
Patch for feOffset properties
r=me.
feDiffuseLighting patch landed in: http://trac.webkit.org/changeset/64191 feOffset patch landed in: http://trac.webkit.org/changeset/64192 Created attachment 63325 [details]
Patch for feSpotLight properties
Created attachment 63445 [details]
Patch for feSpotLight properties (v2)
As discussed in irc
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 :-)
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. (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. feSpotLight patch landed in http://trac.webkit.org/changeset/64716. Created attachment 66698 [details]
Patch for fePointLight properties
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.
Created attachment 66714 [details]
Patch for fePointLight properties
Comment on attachment 66714 [details]
Patch for fePointLight properties
Great tests!
Comment on attachment 66714 [details] Patch for fePointLight properties Clearing flags on attachment: 66714 Committed r66881: <http://trac.webkit.org/changeset/66881> All reviewed patches have been landed. Closing bug. Created attachment 66864 [details]
Patch for feDistantLight properties
CQ closed the bug 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. 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. Created attachment 66896 [details]
Patch for feDistantLight properties
Comment on attachment 66896 [details]
Patch for feDistantLight properties
r=me
Comment on attachment 66896 [details] Patch for feDistantLight properties Clearing flags on attachment: 66896 Committed r67068: <http://trac.webkit.org/changeset/67068> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/67068 might have broken GTK Linux 32-bit Release and Qt Linux Release |
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.