Summary: | FEComponentTransfer element doesn't support dynamic invalidation | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||
Component: | SVG | Assignee: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | krit, simon.fraser, thorton, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | http://granite.sru.edu/~ddailey/svg/feComponentTransfer2.svg | ||||||||||||
Attachments: |
|
Description
Renata Hodovan
2011-10-10 02:38:26 PDT
Created attachment 110532 [details]
Proposed patch
Chromium ews is yellow 'cos the new tests need plaform specific expecteds on chromium. The rest failing tests don't contain any SVG elements, so I guess they are unrelated or flaky. Comment on attachment 110532 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=110532&action=review Tests look great, I checked each of them. I wish the changes would be more easily visible (did the filter apply really work?) I have to trust you all tests work manually fine and produce this result which is different to no filter applied :-) r- because of abusing SVGElement to share code. Please rework this part, everything else is great. > Source/WebCore/svg/SVGElement.cpp:141 > + return; Unnecessary. > Source/WebCore/svg/SVGElement.h:120 > + void invalidateParents(); This does not belong in SVGElement - it has a very generic name and leads to confusion. SVGElement should be kept as small as possible. Why don't you introduce a free-function in eg. SVGFilterPrimitiveStandardAttributes.h or another filter-related class? Created attachment 110679 [details] Propsed patch > Tests look great, I checked each of them. I wish the changes would be more easily visible (did the filter apply really work?) I have to trust you all tests work manually fine and produce this result which is different to no filter applied :-) Yeah, tests work manually fine. Perhaps you don't see it on the pngs, because the second pricture tries to approach the input image (e.g. the second parameter set contains the default values) and it's printed to the png. Furthermore I renamed invalidateParents() to invalidateFilterPrimitiveParent() and moved to SVGFilterPrimitveStandardAttributes. As we talked on IRC it's declared as a free function. Created attachment 110680 [details]
Proposed patch
One unnecessary comment is removed.
Comment on attachment 110680 [details]
Proposed patch
Looks great, r=me.
Comment on attachment 110680 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=110680&action=review > Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.cpp:174 > +{ > + ContainerNode* parent = element->parentNode(); Ah forgot to say. You have to either add an early return if element can be null or add a ASSERT(element); Comment on attachment 110680 [details]
Proposed patch
Clearing patch flag hoping that cr-linux EWS will stop processing the patch over and over.
Comment on attachment 110680 [details]
Proposed patch
Heh, obvious return missing in
+ if (attrName == SVGNames::inAttr)
+ invalidateFilterPrimitiveParent(this);
..
ASSERT_NOT_REACHED();
Created attachment 110851 [details]
Propsed patch
Missing returns are added both in SVGComponentTransferFunction and SVGFEMergeNodeElement.
FIXME about the missing svgAttrChanged() is also removed from SVGComponentTransferFunction.
Comment on attachment 110851 [details] Propsed patch View in context: https://bugs.webkit.org/attachment.cgi?id=110851&action=review r=me, please fix this before landing: > Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:144 > + if (attrName == SVGNames::typeAttr > + || attrName == SVGNames::tableValuesAttr > + || attrName == SVGNames::slopeAttr > + || attrName == SVGNames::interceptAttr > + || attrName == SVGNames::amplitudeAttr > + || attrName == SVGNames::exponentAttr > + || attrName == SVGNames::offsetAttr) { This is useless, isSupportedAttribute() checks the same, if that returned true, it's one of these attributes :-). > Source/WebCore/svg/SVGComponentTransferFunctionElement.cpp:149 > + invalidateFilterPrimitiveParent(this); > + return; > + } > + > + ASSERT_NOT_REACHED(); Just make this: invalidateFilterPrimitiveParent(this); It looks to me like this was landed in http://trac.webkit.org/changeset/97369 and can be closed, no? |